It seems that manual text alignment is only used in about two dozen of pages across all Wikisources:

• https://global-search.toolforge.org/?q=%5C<div+id%5C%3D%5C"align%5C-&regex=1&namespaces=&title=
• https://global-search.toolforge.org/?q=%5C%7B%5C%7B%28Align%7CĜisrandigo%7Cهم%E2%80%8Cترازکردن%7C정렬%7CJämka%29%5C%7C%5Ba-z%5D%7B2%7D%5C%7C&regex=1&ignorecase=1&namespaces=&title=

So removing the alignment feature (not side-by-side rendering entirely) might be a reasonable mitigation.

@matmarex that does sound like a reasonable mitigation to me. If possible though, it would be nice to keep the feature and disable the javascript popups. Do you think that would be possible from what you have reviewed?

No doubt it's possible, but ensuring that there are no bugs is much more work than finding just one bug, so I did not try. Maybe we could put some sanitizer on top of the buggy code instead, but I don't know how that will interact with all other parsing code.

@matmarex sanitizer sounds like the best option

@matmarex following up to see if you had a chance to look into the sanitizer. I don't see any other maintainers for this project who could chime in

also adding platform engineering and @Tpt in case they have some insight on this issue

Sorry, I didn't get a chance to work on this.

What a solution to this issue likely involves is a bit of a rewrite of any functions within DoubleWiki.php (the vast bulk of this extension's functionality) that manipulate and/or output html, likely the list @matmarex provided within the task description. There are probably a few ways to do this:

1. Parser->parse() - likely too expensive and overkill for this
2. Sanitizer::removeSomeTags() - also a bit expensive, but possibly more appropriate
3. Use HTMLPurifier or similar - a few extensions already use this as a dep (and there's literally an extension just for using it) but nothing that's been reviewed or pushed to Wikimedia production as far as I can tell
4. Review and rewrite the problematic regular expressions - this seems like a waste of time and effort given the previous options

Anyhow, whatever option is pursued, it seems like some basic unit tests should be a part of that effort, as there currently are none. And I suppose there is one final option, though I'd imagine we'd like to avoid it: un-deploy the extension from Wikimedia production.

@sbassett thanks for looking into this....it seems like the HTMLPurifier is our best bet, but should probably be reviewed before being used on WMF production if I'm reading that correctly. I definitely think undeploying the extension would harm some translation efforts and other popular uses of this extension, so I would only consider that as a last resort.

the security team would like to start the process to get this extension undeployed as there has been no movement on fixing this high priority security issue by community members or WMF staff. I'll submit this extension to the code stewardship process, but that has been paused for the moment, so there will probably be no activity there for a while.

for reference, the code stewardship request is https://phabricator.wikimedia.org/T332850

In T323651#8720153, @Mstyles wrote:

as there has been no movement on fixing this high priority security issue by community members or WMF staff.

Community members cannot fix security issues if they don't know about / cannot access security issues...
Which steps have been performed to find WMF staff to look into this?

@Aklapper the platform engineering team has been added along with any maintainers mentioned. I don't think there's any other staff members who could be tagged here

The PoC here sounds the same as the one on T131199#2342458 so possibly the never applied patch at F4100931 might help. [have not verified myself. Going solely on the PoC looking sort of similar].

That said, this extension is all sorts of sketch. Disabling is not an unreasonable course of action.

In T323651#8720357, @Aklapper wrote:

Community members cannot fix security issues if they don't know about / cannot access security issues...
Which steps have been performed to find WMF staff to look into this?

Sadly, there is only one listed maintainer for this extension, and they were added to this bug in January of this year. And there simply aren't really any other active maintainers of the DoubleWiki extension. And having a look at the recent master log, most commits are from libraryupgrader or l10n-bot, with a handful of coding standard updates, but no significant development or maintenance.

In T323651#8723568, @Bawolff wrote:

The PoC here sounds the same as the one on T131199#2342458 so possibly the never applied patch at F4100931 might help. [have not verified myself. Going solely on the PoC looking sort of similar].

I couldn't reproduce those issues at the time, though my testing may have been flawed. I've had a look at the old patch from that bug/paste and it's definitely a non-trivial review to get it up-to-date. I've been working through it a bit this morning, but a lot of maintenance work has been performed on DoubleWiki_body.php since that patch was first written (including moving it to includes/DoubleWiki.php), so it will probably take me a little while to properly walk through the conflicts and merge it. That being said, if we can get a sane, updated patch out of this, then it's probably worthwhile deploying it to production to see if it addresses most or all of the injection issues present.

That said, this extension is all sorts of sketch. Disabling is not an unreasonable course of action.

Agreed, especially since it apparently has no active maintainers and isn't incredibly widely-deployed.

There's a patch out to disable DoubleWiki extension for now until we have a fix for this XSS.

Since we're considering undeploying the extension now, I'd like to again suggest removing just the vulnerable feature (aligning content between languages), which basically no one is using anyway, and keeping the rest. I would work on that if anyone is willing to merge such a patch.

In T323651#8813113, @matmarex wrote:

I would work on that if anyone is willing to merge such a patch.

Sure, happy to review.

In T323651#8813472, @Legoktm wrote:

In T323651#8813113, @matmarex wrote:

I would work on that if anyone is willing to merge such a patch.

Sure, happy to review.

This would be a decent temporary solution IMO, but it still doesn't solve the issue of an active maintainer to support future security (and other) bugs for this extension. If that piece cannot be solved within a reasonable timeframe, then I think T332850 is still likely the best path forward.

Security patch to remove the feature, plus three hardening patches to clean up the remaining code (not known to be vulnerable) that was doing regexp replacements on HTML or questionable HTML escaping:

I can submit the latter three for review on Gerrit later, or we may review here and apply them as security patches if you'd like.

@matmarex thank you so much for submitting these patches! is there anyone we can find as a reviewer to ensure we're moving in the right direction?

@matmarex we would prefer to keep these as patches and not review on Gerrit for security purposes

In T323651#8817770, @Mstyles wrote:

@matmarex thank you so much for submitting these patches! is there anyone we can find as a reviewer to ensure we're moving in the right direction?

I haven't reached out to anyone, other than the folks who subscribed themselves to this task and might be willing. Do you want me to advertise it on the WMF Slack or something?

In T323651#8820970, @Mstyles wrote:

@matmarex we would prefer to keep these as patches and not review on Gerrit for security purposes

Sure, no problem.

In T323651#8817770, @Mstyles wrote:

is there anyone we can find as a reviewer to ensure we're moving in the right direction?

Errr, I already volunteered to do the review? (T323651#8813472)

I did a visual review just now, they look fine, will do a bit of testing tomorrow before +2ing.

@Legoktm did you get a chance to do some testing? this could possibly be deployed today during the security deploy window

CR+2 for the first security patch from T323651#8814793. It should substantially reduce the risk of matchColumns(). There's still some sketchy regexp stuff happening within getMangledTextAndTranslation(), but we don't have any reported issues there, yet. I think we can security-deploy this and see how it goes, while maybe creating a corresponding public bug explaining that the match columns functionality for DoubleWiki will be indefinitely disabled. If the security patch seems stable after a day or two, we can likely push @matmarex' remaining code improvement patches through gerrit.

In T323651#8814793, @matmarex wrote:

Security patch to remove the feature, plus three hardening patches to clean up the remaining code (not known to be vulnerable) that was doing regexp replacements on HTML or questionable HTML escaping:

0001-SECURITY-Remove-vulnerable-column-alignment-feature.patch8 KBDownload

0002-Remove-broken-link-processing-for-the-matched-text.patch1 KBDownload

0003-Modernize-HTML-output.patch5 KBDownload

0004-Use-HtmlHelper-modifyElements-instead-of-regexps-to-.patch3 KBDownload

I can submit the latter three for review on Gerrit later, or we may review here and apply them as security patches if you'd like.

The Security patch ONLY has been deployed

In T323651#8852650, @Mstyles wrote:

The Security patch ONLY has been deployed

Thanks! It would be nice if we could test that the reported payload no longer works in production. While testwiki seems to have ext:DoubleWiki enabled, it does not appear to be working there, which isn't really surprising. I have OS rights on the wikisources with my staff rights, apparently, but I hesitate to edit something like Bible_(King_James)/Genesis there, even if I could be rather quick about testing and then suppressing the edit.

Seems like it works on test https://test.wikipedia.org/wiki/Main_Page?match=en

In T323651#8855905, @Bawolff wrote:

Seems like it works on test https://test.wikipedia.org/wiki/Main_Page?match=en

Huh, interesting. On some fairly obvious pages (or what I think are fairly obvious) it does not?

https://test.wikipedia.org/wiki/Dog?match=en
https://test.wikipedia.org/wiki/Barack_Obama?match=en

etc.

Anyhow, I tested against Main_page: https://test.wikipedia.org/w/index.php?title=Main_Page&diff=570072&oldid=570071. I think I did that correctly and could not get the XSS payload within the task description to execute when attempting a ?match=en.

It requires the interlanguage link to be present (i just added one to dog so that works now)

In T323651#8814793, @matmarex wrote:

Security patch to remove the feature, plus three hardening patches to clean up the remaining code (not known to be vulnerable) that was doing regexp replacements on HTML or questionable HTML escaping:

0001-SECURITY-Remove-vulnerable-column-alignment-feature.patch8 KBDownload

0002-Remove-broken-link-processing-for-the-matched-text.patch1 KBDownload

0003-Modernize-HTML-output.patch5 KBDownload

0004-Use-HtmlHelper-modifyElements-instead-of-regexps-to-.patch3 KBDownload

I can submit the latter three for review on Gerrit later, or we may review here and apply them as security patches if you'd like.

I submitted the second patch to Gerrit, it can be found here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DoubleWiki/+/924136
The last two patches did not apply when I tried. Not sure what happened

In T323651#8889596, @Mstyles wrote:

I submitted the second patch to Gerrit, it can be found here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DoubleWiki/+/924136
The last two patches did not apply when I tried. Not sure what happened

The final two patches depend upon the changes made within the security patch. So they'll have to wait to be merged until that patch is publicly released. Which we could do now (since it's already in Wikimedia production: T276237) or wait until the end of the quarter, when it gets mentioned within the next supplemental security release (T333626).

Change 932825 had a related patch set uploaded (by Mstyles; author: Bartosz Dziewoński):

[mediawiki/extensions/DoubleWiki@master] Remove vulnerable column alignment feature

https://gerrit.wikimedia.org/r/932825

Change 933666 had a related patch set uploaded (by Mstyles; author: Bartosz Dziewoński):

[mediawiki/extensions/DoubleWiki@master] Modernize HTML output

https://gerrit.wikimedia.org/r/933666

Change 933667 had a related patch set uploaded (by Mstyles; author: Bartosz Dziewoński):

[mediawiki/extensions/DoubleWiki@master] Use HtmlHelper::modifyElements() instead of regexps to modify links

https://gerrit.wikimedia.org/r/933667

Change 932825 merged by jenkins-bot:

[mediawiki/extensions/DoubleWiki@master] Remove vulnerable column alignment feature

https://gerrit.wikimedia.org/r/932825

Change 933666 merged by jenkins-bot:

[mediawiki/extensions/DoubleWiki@master] Modernize HTML output

https://gerrit.wikimedia.org/r/933666

Change 933667 merged by jenkins-bot:

[mediawiki/extensions/DoubleWiki@master] Use HtmlHelper::modifyElements() instead of regexps to modify links

https://gerrit.wikimedia.org/r/933667