Page MenuHomePhabricator

Security review of Ex:DoubleWiki
Closed, ResolvedPublic

Related Objects

StatusSubtypeAssignedTask
Resolvedsbassett

Event Timeline

csteipp created this task.Mar 29 2016, 9:39 PM
csteipp created this object with visibility "Custom Policy".
Krenair added a subscriber: Krenair.
Dzahn removed a subscriber: Dzahn.Apr 11 2016, 6:58 PM

I'm not entirely done yet. So far though, its basically vulnerable to the same thing as language converter is - that is, if you can put tags inside attributes and have the angled brackets be unescaped, then you can trick it into doing XSS.

For example, if you have the uncommon configuration $wgExperimentalHtmlIds = true;

(Sorry, this isn't entirely minimized)

And have the right side of the match be

==foo==
<span id="dw-foo" title="title of span"/>
Sad paragraph

==bar==
<span id="dw-foo2" title="title of bar span"/>

Some random text. Some more text.

==&lt;p&gt;baz&lt;/p&gt;&lt;script&gt;alert(4)&lt;/script&gt;==

<span id="dw-foo2" title="next"/>

This random text is different.

And the left side be:

<div id="mw-content-text">foo</div>

[[de:Foo]]

==foo==
<span id="dw-foo" title="title of span"/>
Happy paragraph

==bar==
<span id="dw-foo2" title="title of bar span"/>

Some random text. Some more text.

<span id="dw-foo2" title="next"/>

Less text. Fee fa fo fum!

You get an XSS. Similar things might be possible anywhere else < and > are not escaped to entities in attributes (We've removed most of the examples I know about)

Ok. So the situation is actually worse than i initially thought:

  • attribute breaking if you can put <p> in an attribute (see comment above)
  • attribute insertion using allignment pre's (including putting an onload on an img tag)
  • cache pollution of per user parser content. Possibly with no interaction from target user as doublewiki works from edit preview (thus via a different origin xhr). Can leak rollback tokens
  • possibly also attribute insertion on right side text from text breaking in middle of attribute (I was mistaken, this isn't really an issue)
  • attribute insertion using allignment pre's (including putting an onload on an img tag)

To be clear, I mean wikitext like:

[[de:Foo]]
<div id="align-de" style="display:none;"><pre>
TRIGGER=" onload=alert(217) d=<p>
</pre></div>


[[File:0001c.jpg|20px|alt=TRIGGER]]
dpatrick updated the task description. (Show Details)May 31 2016, 8:43 PM
dpatrick updated the task description. (Show Details)May 31 2016, 8:46 PM
  • cache pollution of per user parser content. Possibly with no interaction from target user as doublewiki works from edit preview (thus via a different origin xhr). Can leak rollback tokens

To clarify, by this, I mean an attacker tricks the victim web browser into loading (via ajax or whatever) something like http://localhost/w/git/index.php?title=fdadfa&action=edit&preloadtitle={{special:listusers/bawolff}}[[de:foo]]&section=new&preview=yes&match=de Which then gets into the shared cache, which the attacker can retrieve at a later date. The version in the cache is per-user, and can have sensitive data on it (in that example, it will have suppressed usernames if the vicitim is allowed to see that info.)

This will prevent the issues I noticed, mostly by ensuring that DoubleWiki is not used on pages which have edge cases in the html. Extension is still kind of scary, but I believe is "safe" with these changes.

Bawolff removed Bawolff as the assignee of this task.Jun 9 2016, 7:37 PM

I'll review/test the patch.

Aklapper removed dpatrick as the assignee of this task.Feb 19 2018, 11:19 AM
sbassett added a subscriber: sbassett.

Note: re-test exploitable XSS, pre-conditions may not be true. Assign soon for review/testing.

A few follow-up notes:

'wmgUseDoubleWiki' => [
	'default' => false,
	'wikisource' => true,
	'sourceswiki' => true,
	'frwiktionary' => true,
	'frwikiversity' => true,
	'fawiki' => true,
	'testwiki' => true,
]
sbassett lowered the priority of this task from High to Medium.Jul 2 2019, 5:39 PM
sbassett claimed this task.Jul 9 2019, 6:10 PM
sbassett lowered the priority of this task from Medium to Low.
sbassett moved this task from Frozen to In Progress on the deprecated-security-team-reviews board.
sbassett moved this task from Backlog to In Progress on the user-sbassett board.Jul 10 2019, 9:10 PM
sbassett added a comment.EditedJul 26 2019, 9:31 PM

Update:

  1. $wgExperimentalHtmlIds was deprecated in 1.32.
  2. 1.31 is still in LTS until 2021.
  3. DoubleWiki master currently gets a thumbs up from both mwext-php72-phan-docker and mwext-php72-phan-seccheck-docker (see here). While this is reassuring it, of course, is hardly a definitive blessing, especially when looking at some of the more obscure issues found by @Bawolff above.
  4. @Bawolff's patch from T131199#2347343 doesn't apply to REL1_31, so it'll need a rebase, and certainly would for newer release branches and master. I'll have to look into this a bit more as I'm not entirely sure the patch is relevant outside of $wgExperimentalHtmlIds being set to true, which is a non-issue for 1.32+. Still, we (I) probably need to attempt a rebase for 1.31 since that's still supported for 2 more years.

As a note, in theory mw should be better at escaping < and > in attributes now a days as remexhtml is a lot less tolerant of that (possibly in security relavent ways) than tidy was

Ah, ok, thanks @Bawolff. Probably why the phan/seccheck dockers are happy in CI.

sbassett added a subscriber: Reedy.Jul 31 2019, 4:19 PM

Update re: RemexHtml: looks like it became the standard in 1.31. Which checks out as $wgTidyConfig is indeed set to [ 'driver' => 'RemexHtml' ]; and $wgUseTidy is set to false in DefaultSettings.php for even early release candidates of 1.31, as well as later versions. The same is true for REL1_31. I'd like to play around with this a bit more just to confirm that things look fine and that I'm not seeing any other security issues with DoubleWiki. Though chatting with @Reedy, DoubleWiki isn't in widespread use (greatest impact for us are the wikisources) and given when RemexHtml became the default, backporting shouldn't even be necessary (unless other issues were found) now given the current version lifecycle.

sbassett added a comment.EditedAug 19 2019, 9:47 PM

Update: I built out and tested against MediaWiki and ex:DoubleWiki REL1_31, REL1_32, REL1_33 and master, all of which are now using RemexHtml and for which $wgExperimentalHtmlIds was either disabled or no longer exists. The SecurityCheckPlugin found nothing for master but did return SecurityCheck-DoubleEscaped issues for the htmlspecialchars( $left_url ) and htmlspecialchars( $right_url ) calls within matchColumns() for REL1_31, REL1_32, REL1_33. However I believe these to be false positives given how they are quasi-self-assigning and the SecurityCheck-DoubleEscaped issue is not found in master, where they are no longer quasi-self-assigned.

I could not get the XSS examples from T131199#2337832 or T131199#2342458 to render as it appears that RemexHtml is indeed much more strict in what it allows within html attributes. I'm not certain I fully understand the cache pollution issue at T131199#2347325, so I'll have to investigate that a bit further. But for now, I think DoubleWiki seems fairly safe with respect to the XSS issues previously found.

sbassett moved this task from Postponed to In Progress on the user-sbassett board.Oct 15 2019, 4:22 PM
sbassett lowered the priority of this task from Low to Lowest.Oct 22 2019, 6:25 PM
sbassett moved this task from In Progress to Done on the user-sbassett board.Dec 6 2019, 9:18 PM
sbassett moved this task from Done to Postponed on the user-sbassett board.Dec 10 2019, 9:31 PM
chasemp moved this task from Incoming to Waiting on the secscrum board.Mar 10 2020, 8:13 PM

@Bawolff - I was never able to reproduce the XSS issues within newer versions of MediaWiki as noted within T131199#5423140. I'm also not certain how risky the cache pollution issue is. Thinking about just resolving and making this task public.

sbassett moved this task from Postponed to Waiting on the user-sbassett board.May 28 2020, 9:21 PM
sbassett moved this task from Waiting to Postponed on the user-sbassett board.Jun 29 2020, 3:41 PM
sbassett closed this task as Resolved.Jul 1 2020, 9:04 PM
sbassett moved this task from Waiting to Our Part Is Done on the secscrum board.
sbassett moved this task from Postponed to Done on the user-sbassett board.
sbassett removed a project: Patch-For-Review.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".