From ce8edcc5657835811554ebb29186d3c6fc3de3e1 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 15 May 2006 09:45:14 +0000 Subject: [PATCH] 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. --- RELEASE-NOTES | 2 ++ includes/EditPage.php | 4 ++-- includes/OutputPage.php | 4 ++-- includes/SkinTemplate.php | 2 +- includes/SpecialPreferences.php | 41 ++++++++++++++++----------------- includes/User.php | 19 +++++++++++++++ 6 files changed, 46 insertions(+), 26 deletions(-) diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 68966d73bf..d1c9370611 100644 --- 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 a8a72853ef..24c9b4f100 100644 --- 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 de27ed9953..c155d380c4 100644 --- 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"; diff --git a/includes/SkinTemplate.php b/includes/SkinTemplate.php index 771004df5e..ae392a0159 100644 --- 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 8b972b01a6..bccbc6e4b5 100644 --- 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( "" ); + $wgOut->addHtml( wfHidden( 'wpQuickbar', $this->mQuickbar ) ); } # Skin @@ -816,10 +816,11 @@ class PreferencesForm { # global $wgLivePreview, $wgUseRCPatrol; $wgOut->addHTML( '
' . wfMsg( 'textboxsize' ) . ' -
- mRows}\" size='3' /> - mCols}\" size='3' /> -
" . +
' . + wfInputLabel( wfMsg( 'rows' ), 'wpRows', 'wpRows', 3, $this->mRows ) . + ' ' . + wfInputLabel( wfMsg( 'columns' ), 'wpCols', 'wpCols', 3, $this->mCols ) . + "
" . $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( '
' . htmlspecialchars(wfMsg('prefs-rc')) . '' . - ' 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( '
' . wfMsgHtml( 'prefs-watchlist' ) . '' ); - $wgOut->addHTML( ' ' ); - $wgOut->addHTML( '' ); + $wgOut->addHTML( wfInputLabel( wfMsg( 'prefs-watchlist-days' ), + 'wpWatchlistDays', 'wpWatchlistDays', 3, $this->mWatchlistDays ) ); $wgOut->addHTML( '

' ); # Spacing $wgOut->addHTML( $this->getToggles( array( 'watchlisthideown', 'watchlisthidebots', 'extendwatchlist' ) ) ); - $wgOut->addHTML( ' ' ); - $wgOut->addHTML( '' ); + $wgOut->addHTML( wfInputLabel( wfMsg( 'prefs-watchlist-edits' ), + 'wpWatchlistEdits', 'wpWatchlistEdits', 3, $this->mWatchlistEdits ) ); $wgOut->addHTML( '
' ); # Search $wgOut->addHTML( '
' . wfMsg( 'searchresultshead' ) . '' . $this->addRow( - '', - "mSearch\" size='4' />" + wfLabel( wfMsg( 'resultsperpage' ), 'wpSearch' ), + wfInput( 'wpSearch', 4, $this->mSearch, array( 'id' => 'wpSearch' ) ) ) . $this->addRow( - '', - "mSearchLines\" size='4' />" + wfLabel( wfMsg( 'contextlines' ), 'wpSearchLines' ), + wfInput( 'wpSearchLines', 4, $this->mSearchLines, array( 'id' => 'wpSearchLines' ) ) ) . $this->addRow( - '', - "mSearchChars\" size='4' />" + wfLabel( wfMsg( 'contextchars' ), 'wpSearchChars' ), + wfInput( 'wpSearchChars', 4, $this->mSearchChars, array( 'id' => 'wpSearchChars' ) ) ) . "
" . wfMsg( 'defaultns' ) . "$ps
" ); # Misc # $wgOut->addHTML('
' . wfMsg('prefs-misc') . ''); - $wgOut->addHTML( - '' . - " 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 b109e803f2..60a1bbed34 100644 --- 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(); } -- 2.20.1