Last modified: 2012-11-29 22:54:29 UTC
Created attachment 11241 [details] patch from Kevin I'm the "Wikipedia user PleaseStand" who reported the clickjacking vulnerability in 2010, and I think I have another security issue to report. On the English Wikipedia's technical village pump, user Kelisi reported that a certain article's history page would show a "Fatal exception of type MWException" only when he was logged in.[1] I debugged the issue and found that the cause was an inefficient regex in includes/Linker.php in core MediaWiki. Specifically, the regex hit the PCRE backtrack limit while processing an edit summary,[2] so preg_replace_callback() returned null. The fatal exception occurred when the null value was to be inserted as a message parameter. I'm reporting this issue through private e-mail, as I know such exponential behavior can allow for denial of service attacks, and I'm sure the vandals would enjoy making it hard for people to revert their edits :) They just have to put something like this in their edit summaries: [[aaaa]|aaaa]|aaaa]| ... (repeated at least 13 times more) and that would break recent changes, watchlists, and history pages all at once. The API, by the way, just returns null for the "parsedcomment". I have attached a patch that should fix the regex. Feel free to make any improvements. I've tested it against the recent histories (newest 1000 revisions) of three English Wikipedia articles. Keep in mind it's possible that someone might inadvertently add information to the village pump thread that makes it easier to find the vulnerability (e.g. a broken diff link). Also, if I would like to report a vulnerability in a user-created Wikipedia gadget, where should I send the report to? Here? I think I may have found an unrelated vulnerability in Twinkle, although it might be tricky to exploit. Thanks, Kevin Israel (Wikipedia user PleaseStand) [1] https://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28technical%29#Problem_with_.22Africville.22_article [2] https://en.wikipedia.org/w/index.php?title=Africville&oldid=360541408 - try clicking on one of the diff links
Kevin, I was able to reproduce the issue, and the patch seems to fix the problem. Thank you! We have just finalized our next security release, but we'll see if we can include this patch too.
The patch looks fine, although it would have been nice to see some comments embedded in the regex, maybe with /x. I would also like to see a patch to remove the throw from Message::extractParam(). This is not the first user-visible exception we've seen from it, and throwing an exception instead of replacing the parameter with a placeholder increases the severity from a minor annoyance to a DoS vulnerability. If the error message really needs to be highly visible to developers, trigger_error(..., E_USER_WARNING) could be used.
Created attachment 11274 [details] patch to comment new regex
Created attachment 11275 [details] patch to remove the throw Tim, do you have a specific reason for using trigger_error() directly rather than using wfWarn()? Also, do you have a particular placeholder message in mind that would work in all contexts, or is it fine to just leave $1 alone?
See also bug 38566.
(In reply to comment #4) > Created attachment 11275 [details] > patch to remove the throw > > Tim, do you have a specific reason for using trigger_error() directly rather > than using wfWarn()? Also, do you have a particular placeholder message in mind > that would work in all contexts, or is it fine to just leave $1 alone? wfWarn() only calls trigger_error() if $wgDevelopmentWarnings is true, which it usually isn't. For example, it is false on Wikimedia. I even have it switched off on my test instances, because otherwise I would be constantly spammed with deprecation notices from code I'm not developing. I was thinking of a more explicit placeholder than $1, like [INVALID].
Created attachment 11334 [details] patch to remove the throw (2)
I've run this set of patches locally, and it looks like it fixes the problem. Tim, does this look good enough to you to deploy?
(In reply to comment #8) > I've run this set of patches locally, and it looks like it fixes the problem. > > Tim, does this look good enough to you to deploy? Yes, it looks good.
Created attachment 11396 [details] Combined patch for 1.21 Adds all three of Kevin's patches in one file.
*** This bug has been marked as a duplicate of bug 38566 ***
