Last modified: 2009-09-10 21:18:22 UTC
Created attachment 6320 [details] Updated img_auth.php This started with localizing the messages with img_auth.php but expanded. 1. Localize img_auth.php using img_auth.i18.php and current EN language 2. Reorder checks to make sense (and eliminate redundancy). New order of checks should be: * See if this is a public Wiki (no protections) * See if server allows PATH_INFO * Basic directory traversal check * See if could create the title object * Check to see if the file exists * Check to see if tried to access a directory * See if could create the title object * Run hook for extended checks * Check user authorization for this title (UserCan) * Whitelist check was deprecated - redundant to UserCan 3. Add a hook to allow custom checking for Custom FileRepo(s) 4. Add a global variable wgImgAuthDetails (img_auth only) that defaults to minimum info, but allows Details if set to true 5. Move all "wfDebugLog" into the rejection functions - since they should never get there anyway. This really cleans up the script file and with the hook and localization should minimize future maintenance. I've tested to the extent that I can in my environment, but really need code check and other testers. Would also do documentation.
Created attachment 6321 [details] Localization file (en only) for img_auth
Installation Instructions for testers: 1. Copy img_auth.php into ($IP) dir (overwrites existing) 2. Copy img_auth.i18.php into ($IP) dir (where img_auth.php) resides Note: First "* See if could create the title object" in check order of description is redundant, and a typo. Real check is right before hook.
Couple quick notes: First, the first parameter to wfForbidden() seems to always be the "access forbidden" message; it's probably cleaner to just call that from the function. :) The debug log messages shouldn't be localized; those are internal messages which should be consistently readable by site administrators in a multilingual environment so they can debug issues. With $wgImgAuthDetails on, input filenames are being passed into HTML error messages without validation or escaping; this is a script injection vuln. wfMsgHTML() escapes the text of the message, then replaces in your parameters -- the expectation being that your parameters are formatted HTML such as links. Also we'd generally want config vars like this defined in DefaultSettings.php so they can be consistently located. I'm a little vague on what the hook accomplishes; if meant for alternate file repository types, it'll fail as we've already dropped out a 403 result due to the file not existing in $wgUploadDirectory... It looks like the only thing it could do is reject access to local files which would otherwise have been allowed. Probably if alternate source backends are desired here (say, database storage or a WebDAV storage backend), they'd need their own implementation on the repository class for checking path validity and doing the output streaming.
(In reply to comment #3) > Couple quick notes: > First, the first parameter to wfForbidden() seems to always be the "access > forbidden" message; it's probably cleaner to just call that from the function. > :) There are two parts to the HTML 403 message, the header and the body. * msg1 is header * msg2 is body Would be glad to change, but many gov sites have a standard header text such as: * We're big bad mothers so don't mess with us Followed by standard body text * The knock you're hearing on the door is our black suited, dark eyeglass goon squad who are going to take you to a dark place and do terrible things to you. I'd be glad to reduce the customization, but thought, what the heck. > The debug log messages shouldn't be localized; those are internal messages > which should be consistently readable by site administrators in a multilingual > environment so they can debug issues. When I updated the messaging, I thought, hey, some people may want to give out more detailed messages than "you can't get here", so I added that capability with the localization. As long as that message was being sent to the user, thought it might make sense to send the exact same message to the log file. > With $wgImgAuthDetails on, input filenames are being passed into HTML error > messages without validation or escaping; this is a script injection vuln. > wfMsgHTML() escapes the text of the message, then replaces in your parameters > -- the expectation being that your parameters are formatted HTML such as links. Good point about the script vunerability. Don't know how I missed that one. Unless you have a better approach, I think I'll just take out the "$1" capability in all the messages. Should I be using straight wfMsg instead of wfMsgHTML too? Other suggestions (or someone who might help without bothering you?) > Also we'd generally want config vars like this defined in DefaultSettings.php > so they can be consistently located. I think that img_auth.php isn't used in the majority of MediaWiki installations. Adding that parameter to the generic is something that would only be used if img_auth (rewriting) is used. If you think it's better there, I'm all for it. This way, only used when needed. > I'm a little vague on what the hook accomplishes; if meant for alternate file > repository types, it'll fail as we've already dropped out a 403 result due to > the file not existing in $wgUploadDirectory... It looks like the only thing it > could do is reject access to local files which would otherwise have been > allowed. > Probably if alternate source backends are desired here (say, database storage > or a WebDAV storage backend), they'd need their own implementation on the > repository class for checking path validity and doing the output streaming. You saw straight through me on this one. Yes, I was intending to use it on NSFileRepo, which does namespace validation. That implementation, based on Tim Starling's recommendation, is an extension and alternate "backend" source - and yes, I used a new repository class to do it. With this hook, would be able to use this extension and possibly future acess rejections without branching img_auth.php. See http://www.mediawiki.org/wiki/Extension:NSFileRepo
(In reply to comment #3) > With $wgImgAuthDetails on, input filenames are being passed into HTML error > messages without validation or escaping; this is a script injection vuln. > wfMsgHTML() escapes the text of the message, then replaces in your parameters Revisiting this one. I used wfMsgHTML() which has htmlspecialchars() escaping in it. I may be displaying my ignorance here, but wouldn't that avoid any injection by displaying it as a string versus allowing the injection of html links, javascript, etc. This would actually allow the admin to view what the injection attack was, rather than allow it to proceed. I'll admit I'm no expert here, so this might be dead wrong. Would also need to inform hook users to do same in hook documentation.
(In reply to comment #3) > With $wgImgAuthDetails on, input filenames are being passed into HTML error > messages without validation or escaping; this is a script injection vuln. > wfMsgHTML() escapes the text of the message, then replaces in your parameters Ah, stupid me - disregard previous comment. Would this solve that problem (wherever used)? wfForbidden(wfMsgHTML('image_auth-accessdenied'),wfMsgHTML('image_auth-noread',htmlspecialchars($name)));
Created attachment 6515 [details] Patch File of final Tested and ready for code review if acceptable
Created attachment 6516 [details] Complete Tested code - ready for code review
Had an idea that would allow late loading of 18n file. Instead of invoking wfMsgHTML before calling forbidden, could wait til after then add the 18n require_once in wfForbidden and wfPublicError. That way the only time the 18n file would load would be when an error has occured. Down-side would be that hook extensions would have to use only message ID's, not actual messages - but I'm not sure that's bad.
You could use a wrapper function, eg: function wfImgAuthMsg() { static $fileloaded = false; if (!$fileloaded) { require_once( dirname( __FILE__ ) . '/img_auth.i18n.php' ); $fileloaded = true; } return call_user_func('wfMsgHTML', func_get_args() ); }
Created attachment 6517 [details] Updated img_auth.php using suggestions from code review 1) Only loads message file if error encountered (should be rare) 2) All logged messages are in english - user messages localized. 3) Errors return message index instead of error messages - actual interpretation of message in wfForbidden. 4) Eliminated all caching, compressed both error messages routines into a single routine.
Created attachment 6518 [details] Complete Tested code - ready for final code review before commit to trunk
Created attachment 6519 [details] Additional Messages rqd for img_auth script
Created attachment 6520 [details] Message Instructions for localization
Created attachment 6521 [details] Moved messages and default settings to language and DefaultSettings.php respectively Will still have to add the following to DefaultSettings.php $wgImgAuthDetails = false; ///< defaults to false - only set to true if you use img_auth and want the user to see details on why access failed $wgImgAuthPublicTest = true; ///< defaults to true - if public read is turned on, no need for img_auth, config error unless other access is used
Created attachment 6522 [details] Complete Tested Code ready for commit to trunk
Created attachment 6523 [details] Unified Diff for bug patches Unified Diff for all required patches
(In reply to comment #13) > Created an attachment (id=6519) [details] > Additional Messages rqd for img_auth script Is there some reason to use ` instead of " what is used elsewhere in MediaWiki? (In reply to comment #17) > Created an attachment (id=6523) [details] +if (!wfRunHooks( 'ImgAuthBeforeStream', array( &$title, &$path, &$name, &$result ) ) ) + call_user_func_array('wfForbidden',merge_array(array($result[0],$result[1]),array_slice($result,2))); Maybe a short comment here about the type of $result wouldn't hurt. If this is a new hook, it should be documented in docs/hooks.txt. +'img-auth-accessdenied' => "[[Image Authorization]] Access Denied", Is the link supposed to go somewhere? Documentation is also wikitext. It looks like you did not update maintenance/language/messages.inc, but hey, other developers forget to update it too. I'm happy that you provided message documentation and that you pass around message keys instead of strings. It is a bit more flexible if you ever want to change something, like support magic words in the translations. +'img-auth-nopathinfo' => "Missing PATH_INFO. Your server is not set up to pass this information - may be CGI-based and can't support img_auth. See `Image Authorization` on MediaWiki.", Manual:x on MediaWiki.org? +'img-auth-notindir' => "Requested path not in upload directory.", is not in the upload directory? +'img-auth-badtitle' => "Unable to construct a valid Title from `$1`.", Title with lowercase, users don't need to care if it is Object for us or not. +'img-auth-nologinnWL' => "Not logged in and `$1` not in whitelist.", "You are not logged in ... is not in the whitelist" +'img-auth-nofile' => "`$1` does not exist.", Maybe prefix with File, to give a hint what is expected +'img-auth-isdir' => "`$1` is a directory.", Same here, you could say we except a file, but directory was given.
Created attachment 6529 [details] Diff Patches for bug 1. Added to messages.inc,used miscellaneous3 block. 2. Followed Niklas's suggestions and modified messages If all is OK, will Commit. On Commit, will document: 1) ImgAuthBeforeStream hook 2) $wgImgAuthDetails 3) wgImgAuthPublicTest
Hrm... this code is *really* nasty looking and needs to be reverted or cleaned up. What's with the call_user_func_array() stuff everywhere? If you need to pass more variable parameters, just have it take an array... Don't try to squish random params onto wfMsgHtml() when you can simply use wfMsgExt() which will take an array of options and an array of message arguments.
Reverted in r56051.
Created attachment 6533 [details] Cleaned up code from revert Cleaned up code: 1) Removed all call_user_func_array(), used $arg array 2) Removed all htmlspecialchars() for file name escaping 3) Reapplied DefaultSettings.php 4) Reapplied messages.inc 5) Reapplied MessagesEn.php and MessagesQqq.php 6) Reapplied RELEASE-NOTES 7) Tested Don't know how to handle reapplying all the internationalization patches - whether that is automatic or whether I should do it manually, but will find out on IRC before committing.
Much cleaner code. :) The one additional recommendation (posted this in IRC, not sure if you saw): my one other recommendation is to change the wgMsgHtml() here to wfMsg() and do an htmlspecialchars() on the outside yourself (or possibly this can be done with one of the escaping options on wfMsgExt() -- we want to HTML-escape after argument replacement, whereas wfMsgHtml() does the escaping before argument replacement) The rest of the localizations got reverted in r56053; you can re-merge them like so: svn merge -r56053:56052 . then do a commit as normal.
Created attachment 6535 [details] Final Touches and localisation files 1) Replaced wgMsgHtml() with htmlspecialchars(wfMsg()). Did not change wfMsgExt() because it is only used to output to log file - which cannot be injected. 2) Merged all localisation files back in. 3) Tested
