r103738 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r103737‎ | r103738 | r103739 >
Date:08:50, 20 November 2011
Author:aaron
Status:resolved (Comments)
Tags:core 
Comment:
image_auth.php cleanups:
* Factored main code into wfImageAuthMain()
* Made preg_match() for $name account for "page3-" type specifiers in the thumb name
* Code style cleanups
Modified paths:
  • /trunk/phase3/img_auth.php (modified) (history)

Diff [purge]

Index: trunk/phase3/img_auth.php
——@@ -10,14 +10,15 @@
1111 *
1212 * Optional Parameters
1313 *
14 - * - Set $wgImgAuthDetails = true if you want the reason the access was denied messages to be displayed
15 - * instead of just the 403 error (doesn't work on IE anyway), otherwise will only appear in error logs
 14+ * - Set $wgImgAuthDetails = true if you want the reason the access was denied messages to
 15+ * be displayed instead of just the 403 error (doesn't work on IE anyway),
 16+ * otherwise it will only appear in error logs
1617 * - Set $wgImgAuthPublicTest false if you don't want to just check and see if all are public
1718 * must be set to false if using specific restrictions such as LockDown or NSFileRepo
1819 *
19 - * For security reasons, you usually don't want your user to know *why* access was denied, just that it was.
20 - * If you want to change this, you can set $wgImgAuthDetails to 'true' in localsettings.php and it will give the user the reason
21 - * why access was denied.
 20+ * For security reasons, you usually don't want your user to know *why* access was denied,
 21+ * just that it was. If you want to change this, you can set $wgImgAuthDetails to 'true'
 22+ * in localsettings.php and it will give the user the reason why access was denied.
2223 *
2324 * Your server needs to support PATH_INFO; CGI-based configurations usually don't.
2425 *
——@@ -33,75 +34,93 @@
3435 }
3536 wfProfileIn( 'img_auth.php' );
3637
 38+# Set action base paths so that WebRequest::getPathInfo()
 39+# recognizes the "X" as the 'title' in ../image_auth/X urls.
3740 $wgActionPaths[] = $_SERVER['SCRIPT_NAME'];
38 -// See if this is a public Wiki (no protections)
39 -if ( $wgImgAuthPublicTest
40 - && in_array( 'read', User::getGroupPermissions( array( '*' ) ), true ) )
41 -{
42 - wfForbidden('img-auth-accessdenied','img-auth-public');
43 -}
4441
45 -$matches = WebRequest::getPathInfo();
46 -$path = $matches['title'];
 42+wfImageAuthMain();
 43+wfLogProfilingData();
4744
48 -// Check for bug 28235: QUERY_STRING overriding the correct extension
49 -$dotPos = strrpos( $path, '.' );
50 -$whitelist = array();
51 -if ( $dotPos !== false ) {
52 - $whitelist[] = substr( $path, $dotPos + 1 );
53 -}
54 -if ( !$wgRequest->checkUrlExtension( $whitelist ) ) {
55 - return;
56 -}
 45+function wfImageAuthMain() {
 46+ global $wgImgAuthPublicTest, $wgRequest, $wgUploadDirectory;
5747
58 -$filename = realpath( $wgUploadDirectory . $path );
59 -$realUpload = realpath( $wgUploadDirectory );
 48+ // See if this is a public Wiki (no protections).
 49+ if ( $wgImgAuthPublicTest
 50+ && in_array( 'read', User::getGroupPermissions( array( '*' ) ), true ) )
 51+ {
 52+ // This is a public wiki, so disable this script (for private wikis only)
 53+ wfForbidden( 'img-auth-accessdenied', 'img-auth-public' );
 54+ return;
 55+ }
6056
61 -// Basic directory traversal check
62 -if( substr( $filename, 0, strlen( $realUpload ) ) != $realUpload ) {
63 - wfForbidden('img-auth-accessdenied','img-auth-notindir');
64 -}
 57+ // Get the requested file path (source file or thumbnail)
 58+ $matches = WebRequest::getPathInfo();
 59+ $path = $matches['title']; // path with leading '/'
6560
66 -// Extract the file name and chop off the size specifier
67 -// (e.g. 120px-Foo.png => Foo.png)
68 -$name = wfBaseName( $path );
69 -if( preg_match( '!\d+px-(.*)!i', $name, $m ) ) {
70 - $name = $m[1];
71 -}
 61+ // Check for bug 28235: QUERY_STRING overriding the correct extension
 62+ $whitelist = array();
 63+ $dotPos = strrpos( $path, '.' );
 64+ if ( $dotPos !== false ) {
 65+ $whitelist[] = substr( $path, $dotPos + 1 );
 66+ }
 67+ if ( !$wgRequest->checkUrlExtension( $whitelist ) ) {
 68+ return;
 69+ }
7270
73 -// Check to see if the file exists
74 -if( !file_exists( $filename ) ) {
75 - wfForbidden('img-auth-accessdenied','img-auth-nofile',$filename);
76 -}
 71+ // Get the full file path
 72+ $filename = realpath( $wgUploadDirectory . $path );
 73+ $realUpload = realpath( $wgUploadDirectory );
7774
78 -// Check to see if tried to access a directory
79 -if( is_dir( $filename ) ) {
80 - wfForbidden('img-auth-accessdenied','img-auth-isdir',$filename);
81 -}
 75+ // Basic directory traversal check
 76+ if ( substr( $filename, 0, strlen( $realUpload ) ) != $realUpload ) {
 77+ wfForbidden( 'img-auth-accessdenied', 'img-auth-notindir' );
 78+ return;
 79+ }
8280
83 -$title = Title::makeTitleSafe( NS_FILE, $name );
 81+ // Check to see if the file exists
 82+ if ( !file_exists( $filename ) ) {
 83+ wfForbidden( 'img-auth-accessdenied','img-auth-nofile', $filename );
 84+ return;
 85+ }
 86+
 87+ // Check to see if tried to access a directory
 88+ if ( is_dir( $filename ) ) {
 89+ wfForbidden( 'img-auth-accessdenied','img-auth-isdir', $filename );
 90+ return;
 91+ }
8492
85 -// See if could create the title object
86 -if( !$title instanceof Title ) {
87 - wfForbidden('img-auth-accessdenied','img-auth-badtitle',$name);
88 -}
 93+ // Extract the file name and chop off the size specifier
 94+ // (e.g. 120px-Foo.png => Foo.png or page2-120px-Foo.png => Foo.png).
 95+ // This only applies to thumbnails, and all thumbnails have a -px specifier.
 96+ $name = wfBaseName( $path );
 97+ if ( preg_match( '!(?:[^-]*-)*?\d+px-(.*)!i', $name, $m ) ) {
 98+ $name = $m[1]; // this file is a thumbnail
 99+ }
89100
90 -// Run hook
91 -if (!wfRunHooks( 'ImgAuthBeforeStream', array( &$title, &$path, &$name, &$result ) ) ) {
92 - wfForbidden($result[0],$result[1],array_slice($result,2));
93 -}
 101+ $title = Title::makeTitleSafe( NS_FILE, $name );
 102+ if ( !$title instanceof Title ) { // files have valid titles
 103+ wfForbidden( 'img-auth-accessdenied', 'img-auth-badtitle', $name );
 104+ return;
 105+ }
94106
95 -// Check user authorization for this title
96 -// UserCanRead Checks Whitelist too
97 -if( !$title->userCanRead() ) {
98 - wfForbidden('img-auth-accessdenied','img-auth-noread',$name);
 107+ // Run hook for extension authorization plugins
 108+ if ( !wfRunHooks( 'ImgAuthBeforeStream', array( &$title, &$path, &$name, &$result ) ) ) {
 109+ wfForbidden( $result[0], $result[1], array_slice( $result, 2 ) );
 110+ return;
 111+ }
 112+
 113+ // Check user authorization for this title
 114+ // UserCanRead Checks Whitelist too
 115+ if( !$title->userCanRead() ) {
 116+ wfForbidden( 'img-auth-accessdenied', 'img-auth-noread', $name );
 117+ return;
 118+ }
 119+
 120+ // Stream the requested file
 121+ wfDebugLog( 'img_auth', "Streaming `".$filename."`." );
 122+ StreamFile::stream( $filename, array( 'Cache-Control: private', 'Vary: Cookie' ) );
99123 }
100124
101 -// Stream the requested file
102 -wfDebugLog( 'img_auth', "Streaming `".$filename."`." );
103 -StreamFile::stream( $filename, array( 'Cache-Control: private', 'Vary: Cookie' ) );
104 -wfLogProfilingData();
105 -
106125 /**
107126 * Issue a standard HTTP 403 Forbidden header ($msg1-a message index, not a message) and an
108127 * error message ($msg2, also a message index), (both required) then end the script
——@@ -111,24 +130,29 @@
112131 */
113132 function wfForbidden( $msg1, $msg2 ) {
114133 global $wgImgAuthDetails;
 134+
115135 $args = func_get_args();
116136 array_shift( $args );
117137 array_shift( $args );
118 - $MsgHdr = htmlspecialchars( wfMsg( $msg1 ) );
119 - $detailMsg = ( htmlspecialchars( wfMsg( ( $wgImgAuthDetails ? $msg2 : 'badaccess-group0' ), $args ) ) );
120 - wfDebugLog('img_auth', "wfForbidden Hdr:".wfMsgExt( $msg1, array( 'language' => 'en' ) )." Msg: ".
121 - wfMsgExt( $msg2, array('language' => 'en' ), $args ) );
 138+
 139+ $msgHdr = htmlspecialchars( wfMsg( $msg1 ) );
 140+ $detailMsgKey = $wgImgAuthDetails ? $msg2 : 'badaccess-group0';
 141+ $detailMsg = htmlspecialchars( wfMsg( $detailMsgKey, $args ) );
 142+
 143+ wfDebugLog( 'img_auth',
 144+ "wfForbidden Hdr:" . wfMsgExt( $msg1, array( 'language' => 'en' ) ). " Msg: ".
 145+ wfMsgExt( $msg2, array( 'language' => 'en' ), $args )
 146+ );
 147+
122148 header( 'HTTP/1.0 403 Forbidden' );
123149 header( 'Cache-Control: no-cache' );
124150 header( 'Content-Type: text/html; charset=utf-8' );
125151 echo <<<ENDS
126152 <html>
127153 <body>
128 -<h1>$MsgHdr</h1>
 154+<h1>$msgHdr</h1>
129155 <p>$detailMsg</p>
130156 </body>
131157 </html>
132158 ENDS;
133 - wfLogProfilingData();
134 - exit();
135159 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r104216FU r103738: redid the method of getting the source file name for thumbnails a...aaron02:21, 25 November 2011
r105208FU r104216: keep the r103738 regex check for thumbnails but use the parent di...aaron19:17, 5 December 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   02:02, 23 November 2011

The stripping of the page2 prefix is wrong, and allows files to masquerade as other files.

I uploaded a file as 'Fake-stuff-200px-Main_Page.jpg'. URL is like http://stormcloud.local/trunk/img_auth.php/5/53/Fake-stuff-200px-Main_Page.jpg 

$wgHooks['ImgAuthBeforeStream'][] = function (&$title, &$path, &$name, &$result) {
	wfDebug("XXX t " . $title->getPrefixedText() . "\n");
	wfDebug("XXX p " . $path. "\n");
	wfDebug("XXX n " . $name. "\n");
	return true;
};

Shows: 

XXX t File:Main Page.jpg
XXX p /5/53/Fake-stuff-200px-Main_Page.jpg
XXX n Main_Page.jpg

the name/title being used for auth checks here are 'Main_Page.jpg' instead of 'Fake-stuff-200px-Main_Page.jpg'.

Looks like the old check would have come up with the same bad response, though; it wasn't anchored at the start of the filename.

Probably better to check for the 'thumb' directory, and use the thumbnail image's immediate parent directory's name for thumbs (which will match the actual orig filename)

Status & tagging log