In PhonosButton.js, the unescaped message text is appended to the page content:
this.getPopup().$body.append( $( '<p>' ).append( mw.message( 'phonos-purge-needed-error' ).text() + ' ', $link ) );
This is vulnerable to an i18n-based XSS because the phonos-purge-needed-error message is user-controlled. You can reproduce the vulnerability by putting something unsafe in the message content (e.g., by using the x-xss language), and then triggering the handleMissingFile() function.
Intentionally vague public patch at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Phonos/+/967269
I think it's fine for it to be public as there's no malicious overrides and even if there were, the conditions under which you could encounter this code path in production is unlikely to occur.
Nonetheless we'll get the fix deployed today. Thanks for identifying this exploit!
@Daimona just curious if you found this by circumstance, or if you have some tooling we should be aware of? I know uselang=x-xss is relatively new, but even using that you can't repro this under normal conditions.
I found it by pure chance :) I got the phonos-purge-needed-error error for a page in production, and I wanted to see what could've caused it. I came across the code in question, and immediately realized that it's vulnerable because I had found similar XSS vulnerabilities just a couple weeks ago (T348343).
BTW, I was looking at the fix in r967269 but that doesn't work here, because " " gets escaped by .text(), and the second argument ($link) is ignored. An alternative would be to replace .text() with .escaped(), but keep using .append(). I don't want to write this on gerrit in case it gives away the real purpose of the patch :)
BTW, I was looking at the fix in r967269 but that doesn't work here, because " " gets escaped by .text(), and the second argument ($link) is ignored. An alternative would be to replace .text() with .escaped(), but keep using .append(). I don't want to write this on gerrit in case it gives away the real purpose of the patch :)
Thanks for catching that! I guess that's why we used append() in the first place :)
Follow-up patch (now merged) at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Phonos/+/967311/
The first patch does fix the exploit and has already been deployed, so this task is safe to be open now.
I've made the task public now that this is patched in production.
As this is very tricky to reproduce, I'm going to be bold and skip QA.
Thanks all for the help!
Change 991335 had a related patch set uploaded (by Mmartorana; author: MusikAnimal):
[mediawiki/extensions/Phonos@REL1_40] PhonosButton: use text() instead of append()
Change 991335 merged by jenkins-bot:
[mediawiki/extensions/Phonos@REL1_40] PhonosButton: use text() instead of append()