Possible DoS vulnerability in MediaWiki
Closed, ResolvedPublic

Description

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


Version: 1.20.x
Severity: normal

Attached: 0001-Optimized-regex-in-Linker.php.patch

bzimport added a subscriber: wikibugs-l.
bzimport set Reference to bz41400.
csteipp created this task.Via LegacyOct 25 2012, 7:08 PM
csteipp added a comment.Via ConduitOct 25 2012, 10:21 PM

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.

tstarling added a comment.Via ConduitOct 29 2012, 5:33 AM

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.

PleaseStand added a comment.Via ConduitOct 31 2012, 10:41 PM

Created attachment 11274
patch to comment new regex

Attached: 0002-Add-comments-to-Linker-formatLinksInComment.patch

PleaseStand added a comment.Via ConduitOct 31 2012, 10:48 PM

Created attachment 11275
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?

attachment 0001-Remove-the-throw-from-Message-extractParam.patch ignored as obsolete

Krinkle added a comment.Via ConduitNov 5 2012, 7:16 AM

See also bug 38566.

tstarling added a comment.Via ConduitNov 5 2012, 8:55 AM

(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].

attachment 0001-Remove-the-throw-from-Message-extractParam.patch ignored as obsolete

PleaseStand added a comment.Via ConduitNov 8 2012, 4:38 PM

Created attachment 11334
patch to remove the throw (2)

Attached: 0001-Remove-the-throw-from-Message-extractParam.patch

csteipp added a comment.Via ConduitNov 20 2012, 12:17 AM

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?

tstarling added a comment.Via ConduitNov 20 2012, 12:20 AM

(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.

csteipp added a comment.Via ConduitNov 20 2012, 6:05 PM

Created attachment 11396
Combined patch for 1.21

Adds all three of Kevin's patches in one file.

Attached: 41400m.patch

Krenair added a comment.Via ConduitNov 29 2012, 10:54 PM
  • This bug has been marked as a duplicate of bug 38566 ***
csteipp added a project: Security.Via WebMar 26 2015, 8:39 PM

Add Comment

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.