Fixes to input validation and output escaping for user preferences.

Inserting a newline into some improperly filtered option strings could be used to overwrite other pref values, bypassing their input validation. Any newlines now get filtered out at User::setOption as a final line of defence.
There were a few HTML injection bugs, but none appear to be exploitable, as prefs can only be set if you already control the account.
Bug found by gmaxwell.

diff --git a/RELEASE-NOTES b/RELEASE-NOTES
index 68966d7..d1c9370 100644 (file)
--- a/RELEASE-NOTES
+++ b/RELEASE-NOTES
@@ -271,6 +271,8 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN
 * (bug 5952) Improvement to German localisation (de)
 * Rename conflicting metadata help message to "metadata_help" (was "metadata")
   and treat it as wiki text
+* Improve preferences input filtering
+
 
 == Compatibility ==
 

diff --git a/includes/EditPage.php b/includes/EditPage.php
index a8a7285..24c9b4f 100644 (file)
--- a/includes/EditPage.php
+++ b/includes/EditPage.php
@@ -835,8 +835,8 @@ class EditPage {
                        $wgOut->addWikiText( wfMsg( 'longpagewarning', $wgLang->formatNum( $this->kblength ) ) );
                }
 
-               $rows = $wgUser->getOption( 'rows' );
-               $cols = $wgUser->getOption( 'cols' );
+               $rows = $wgUser->getIntOption( 'rows' );
+               $cols = $wgUser->getIntOption( 'cols' );
 
                $ew = $wgUser->getOption( 'editwidth' );
                if ( $ew ) $ew = " style=\"width:100%\"";

diff --git a/includes/OutputPage.php b/includes/OutputPage.php
index de27ed9..c155d38 100644 (file)
--- a/includes/OutputPage.php
+++ b/includes/OutputPage.php
@@ -849,8 +849,8 @@ class OutputPage {
                                        $source = wfMsg( $wgUser->isLoggedIn() ? 'noarticletext' : 'noarticletextanon' );
                                }
                        }
-                       $rows = $wgUser->getOption( 'rows' );
-                       $cols = $wgUser->getOption( 'cols' );
+                       $rows = $wgUser->getIntOption( 'rows' );
+                       $cols = $wgUser->getIntOption( 'cols' );
                        
                        $text = "\n<textarea name='wpTextbox1' id='wpTextbox1' cols='$cols' rows='$rows' readonly='readonly'>" .
                                htmlspecialchars( $source ) . "\n</textarea>";

diff --git a/includes/SkinTemplate.php b/includes/SkinTemplate.php
index 771004d..ae392a0 100644 (file)
--- a/includes/SkinTemplate.php
+++ b/includes/SkinTemplate.php
@@ -745,7 +745,7 @@ class SkinTemplate extends Skin {
                                $content_actions['varlang-' . $vcount] = array(
                                                'class' => $selected,
                                                'text' => $varname,
-                                               'href' => $this->mTitle->getLocalUrl( $actstr . 'variant=' . $code )
+                                               'href' => $this->mTitle->getLocalUrl( $actstr . 'variant=' . urlencode( $code ) )
                                        );
                                $vcount ++;
                        }

diff --git a/includes/SpecialPreferences.php b/includes/SpecialPreferences.php
index 8b972b0..bccbc6e 100644 (file)
--- a/includes/SpecialPreferences.php
+++ b/includes/SpecialPreferences.php
@@ -713,7 +713,7 @@ class PreferencesForm {
                } else {
                        # Need to output a hidden option even if the relevant skin is not in use,
                        # otherwise the preference will get reset to 0 on submit
-                       $wgOut->addHTML( "<input type='hidden' name='wpQuickbar' value='{$this->mQuickbar}' />" );
+                       $wgOut->addHtml( wfHidden( 'wpQuickbar', $this->mQuickbar ) );
                }
 
                # Skin
@@ -816,10 +816,11 @@ class PreferencesForm {
                #
                global $wgLivePreview, $wgUseRCPatrol;
                $wgOut->addHTML( '<fieldset><legend>' . wfMsg( 'textboxsize' ) . '</legend>
-                       <div>
-                               <label for="wpRows">' . wfMsg( 'rows' ) . "</label> <input type='text' name='wpRows' id='wpRows' value=\"{$this->mRows}\" size='3' />
-                               <label for='wpCols'>" . wfMsg( 'columns' ) . "</label> <input type='text' name='wpCols' id='wpCols' value=\"{$this->mCols}\" size='3' />
-                       </div>" .
+                       <div>' .
+                               wfInputLabel( wfMsg( 'rows' ), 'wpRows', 'wpRows', 3, $this->mRows ) .
+                               ' ' .
+                               wfInputLabel( wfMsg( 'columns' ), 'wpCols', 'wpCols', 3, $this->mCols ) .
+                       "</div>" .
                        $this->getToggles( array(
                                'editsection',
                                'editsectiononrightclick',
@@ -841,8 +842,8 @@ class PreferencesForm {
                $this->mUsedToggles['autopatrol'] = true; # Don't show this up for users who can't; the handler below is dumb and doesn't know it
 
                $wgOut->addHTML( '<fieldset><legend>' . htmlspecialchars(wfMsg('prefs-rc')) . '</legend>' .
-                                       '<label for="wpRecent">' . wfMsg ( 'recentchangescount' ) .
-                                       "</label> <input type='text' name='wpRecent' id='wpRecent' value=\"$this->mRecent\" size='3' />" .
+                                       wfInputLabel( wfMsg( 'recentchangescount' ),
+                                               'wpRecent', 'wpRecent', 3, $this->mRecent ) .
                        $this->getToggles( array(
                                'hideminor',
                                $wgRCShowWatchingUsers ? 'shownumberswatching' : false,
@@ -853,38 +854,36 @@ class PreferencesForm {
                # Watchlist
                $wgOut->addHTML( '<fieldset><legend>' . wfMsgHtml( 'prefs-watchlist' ) . '</legend>' );
 
-               $wgOut->addHTML( '<label for="wpWatchlistDays">' . wfMsgHtml( 'prefs-watchlist-days' ) . '</label> ' );
-               $wgOut->addHTML( '<input type="text" name="wpWatchlistDays" id="wpWatchlistDays" value="' . $this->mWatchlistDays . '" size="3" />' );
+               $wgOut->addHTML( wfInputLabel( wfMsg( 'prefs-watchlist-days' ),
+                       'wpWatchlistDays', 'wpWatchlistDays', 3, $this->mWatchlistDays ) );
                $wgOut->addHTML( '<br /><br />' ); # Spacing
                $wgOut->addHTML( $this->getToggles( array( 'watchlisthideown', 'watchlisthidebots', 'extendwatchlist' ) ) );
-               $wgOut->addHTML( '<label for="wpWatchlistEdits">' . wfMsgHtml( 'prefs-watchlist-edits' ) . '</label> ' );
-               $wgOut->addHTML( '<input type="text" name="wpWatchlistEdits" id="wpWatchlistEdits" value="' . $this->mWatchlistEdits . '" size="3" />' );
+               $wgOut->addHTML( wfInputLabel( wfMsg( 'prefs-watchlist-edits' ),
+                       'wpWatchlistEdits', 'wpWatchlistEdits', 3, $this->mWatchlistEdits ) );
 
                $wgOut->addHTML( '</fieldset>' );
 
                # Search
                $wgOut->addHTML( '<fieldset><legend>' . wfMsg( 'searchresultshead' ) . '</legend><table>' .
                        $this->addRow(
-                               '<label for="wpSearch">' . wfMsg( 'resultsperpage' ) . '</label>',
-                               "<input type='text' name='wpSearch' id='wpSearch' value=\"$this->mSearch\" size='4' />"
+                               wfLabel( wfMsg( 'resultsperpage' ), 'wpSearch' ),
+                               wfInput( 'wpSearch', 4, $this->mSearch, array( 'id' => 'wpSearch' ) )
                        ) .
                        $this->addRow(
-                               '<label for="wpSearchLines">' . wfMsg( 'contextlines' ) . '</label>',
-                               "<input type='text' name='wpSearchLines' id='wpSearchLines' value=\"$this->mSearchLines\" size='4' />"
+                               wfLabel( wfMsg( 'contextlines' ), 'wpSearchLines' ),
+                               wfInput( 'wpSearchLines', 4, $this->mSearchLines, array( 'id' => 'wpSearchLines' ) )
                        ) .
                        $this->addRow(
-                               '<label for="wpSearchChars">' . wfMsg( 'contextchars' ) . '</label>',
-                               "<input type='text' name='wpSearchChars' id='wpSearchChars' value=\"$this->mSearchChars\" size='4' />"
+                               wfLabel( wfMsg( 'contextchars' ), 'wpSearchChars' ),
+                               wfInput( 'wpSearchChars', 4, $this->mSearchChars, array( 'id' => 'wpSearchChars' ) )
                        ) .
                "</table><fieldset><legend>" . wfMsg( 'defaultns' ) . "</legend>$ps</fieldset></fieldset>" );
 
                # Misc
                #
                $wgOut->addHTML('<fieldset><legend>' . wfMsg('prefs-misc') . '</legend>');
-               $wgOut->addHTML(
-                       '<label for="wpStubs">' . htmlspecialchars ( wfMsg ( 'stubthreshold' ) ) . '</label>' .
-                       " <input type='text' name='wpStubs' id='wpStubs' value=\"$this->mStubs\" size='6' />"
-               );
+               $wgOut->addHTML( wfInputLabel( wfMsg( 'stubthreshold' ),
+                       'wpStubs', 'wpStubs', 6, $this->mStubs ) );
                $msgUnderline = htmlspecialchars( wfMsg ( 'tog-underline' ) );
                $msgUnderlinenever = htmlspecialchars( wfMsg ( 'underline-never' ) );
                $msgUnderlinealways = htmlspecialchars( wfMsg ( 'underline-always' ) );

diff --git a/includes/User.php b/includes/User.php
index b109e80..60a1bbe 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -1069,6 +1069,20 @@ class User {
        function getBoolOption( $oname ) {
                return (bool)$this->getOption( $oname );
        }
+       
+       /**
+        * Get an option as an integer value from the source string.
+        * @param string $oname The option to check
+        * @param int $default Optional value to return if option is unset/blank.
+        * @return int
+        */
+       function getIntOption( $oname, $default=0 ) {
+               $val = $this->getOption( $oname );
+               if( $val == '' ) {
+                       $val = $default;
+               }
+               return intval( $val );
+       }
 
        function setOption( $oname, $val ) {
                $this->loadFromDatabase();
@@ -1076,6 +1090,11 @@ class User {
                        # Clear cached skin, so the new one displays immediately in Special:Preferences
                        unset( $this->mSkin );
                }
+               // Filter out any newlines that may have passed through input validation.
+               // Newlines are used to separate items in the options blob.
+               $val = str_replace( "\r\n", "\n", $val );
+               $val = str_replace( "\r", "\n", $val );
+               $val = str_replace( "\n", " ", $val );
                $this->mOptions[$oname] = $val;
                $this->invalidateCache();
        }