Thank you for working on this. The patch you merged seems to fix the issue with passing it in as a url parameter, it seems to leave open the option of self XSS-ing by putting the string into the search box though. I don't know if this is something that is important to fix, but it mightbe nice.

Can you give an example of how to replicate this?

When going to https://sandbox.semantic-mediawiki.org/wiki/Sp%C3%A9cial:RunQuery/Tokens and putting <img/src=="x onerror=alert(1)//"> into the box, the code also gets executed.

I am aware that this is essentially self harming as this is about similar to pasting things into the console of your developer tools, however there are good reasons why companies like facebook display huge warnings there not to do such things. In the interest of protecting non-technical users this might still be a nice thing to do.

Okay, I see... this one is less of a big deal, as you point out, and it might be harder to fix. But it's good to know about this problem - thanks.

I'm back, because I realized that your fix works in 1.34, but does not seem to work in 1.31.8.

In the older version I still run into this issue.

Are you sure about that? That doesn't make any sense.

https://liquipedia.net/commons/Special:RunQuery/Find_images?pfRunQueryFormName=Find+images&Find+images[person]=%3C!--%3Cimg%20src=%22--%3E%3Cimg%20src=x%20onerror=alert(1)//%22%3E&Find+images[event]=&Find+images[date][day]=&Find+images[date][month]=&Find+images[date][year]=&Find+images[description]=&pf_free_text=&wpRunQuery=Run+query

This wiki uses MW 1.31.8 and PF 4.9.5, but I can still get an alert from a URL parameter. Maybe I am missing something?

Version details can bee seen here: https://liquipedia.net/commons/Special:Version?detail

So the above (T259433#6378990) was obnoxious to figure out, but I think I know what's going on. Basically, select2's default escapeMarkup function (which should work at least ok at sanitizing) is being overriden by an odd old/cached copy within ext.pf.select2.tokens.js. Note that the escapeMarkup function override in 4.9.5 is:

function (m) { return m; };

But if you go to the liquidpedia.net url above and open up a javascript debugger, you can see this version within ext.pf.select2.tokens.js loaded via RL:

opts.escapeMarkup = function(m) {
    return m.replace('<', '&lt;').replace('>', '&gt;').replace('"', '&quot;');
}

Playing around in Chrome's debugger:

var testStr = "<!--<img%20src=\"--><img%20src=x%20onerror=alert(1)//\">"
console.log( testStr.replace('<', '&lt;').replace('>', '&gt;').replace('"', '&quot;') );

the same generated html (as produced by select2 during various DOM manipulations) occurs, as the chained replace functions only perform one round of replacement on the angle brackets, and thus do not sanitize any proceeding, arbitrary html. This is why the img tag is rendered and the resultant onerror handler fires. I had a quick look through PageForms' git history but couldn't find where this change was introduced, so I'm not entirely sure how or why it's there on liquidpedia.net.

Hmm yeah I wanted to patch in a full replace there, but forgot that JavaScript does not replace all occurrences without using a regex.

On my dev wiki at http://fonttax.wiki.tldev.eu/commons/Special:RunQuery/Find_images?pfRunQueryFormName=Find+images&Find+images[person]=%3C!--%3Cimg%20src=%22--%3E%3Cimg%20src=x%20onerror=alert(1)//%22%3E&Find+images[event]=&Find+images[date][day]=&Find+images[date][month]=&Find+images[date][year]=&Find+images[description]=&pf_free_text=&wpRunQuery=Run+query&debug=true (User: dev, PW: tldev) I have a pristine copy though and the issue is still there

I think substituting replaceAll with replace would probably work as well, which can take a regex or a string for the search.

However this isn't the problem on your dev server. The escapeMarkup override of function (m) { return m; }; present within both ext.pf.select2.tokens.js's and ext.pf.select2.comboboxjs's setOptions definition simply return the variable m, unsanitized. I still believe the culprit is the override within ext.pf.select2.tokens.js as it is present within ext.pf.select2.comboboxjs for the original sandbox.semantic-mediawiki.org example within the task description but not within ext.pf.select2.tokens.js, and that issue appears resolved. I think this makes sense given the presence of the pfTokens css class within the relevant rendered multi-selection form input. But this could be an issue for any select2 elements which are (perhaps unintentionally) referencing config from ext.pf.select2.tokens.js or ext.pf.select2.comboboxjs.

Is this still an issue in Page Forms? It's not clear to me from all the discussion what the actual problem is.

As far as I can see, there is still an issue here, as I can still create URL parameters that are XSS exploitable

@Yaron_Koren - yes, both ext.pf.select2.tokens.js and ext.pf.select2.combobox.js are JS libraries for the PageForms extension. Given the minimal doc/comments, these appear to be helper libs for form input autocompletion for the two respective select2 form input types. The problem is that both appear to be overriding select2's Utils.escapeMarkup function in certain scenarios with what is basically a noop function: function (m) { return m; }. This facilitates XSSes in certain cases. The liquipedia.net issue appears related to a fork of ext.pf.select2.tokens.js on their end, as it does not ever appear to be a part of PageForms' git history (that I could find). But the current versions of ext.pf.select2.tokens.js and ext.pf.select2.combobox.js found in PageViews 4.9.5 should likely be fixed to either remove the override of Utils.escapeMarkup within their setOptions functions (here and here) or find some other way for those not to override the essential security functionality of Utils.escapeMarkup.

In T259433#6406933, @sbassett wrote:

The liquipedia.net issue appears related to a fork of ext.pf.select2.tokens.js on their end, as it does not ever appear to be a part of PageForms' git history

I also had the issue in a pristine version on my dev though. That one has the noop version function (m) { return m; } that you mentionned.

In T259433#6408003, @FO-nTTaX wrote:

I also had the issue in a pristine version on my dev though. That one has the noop version function (m) { return m; } that you mentionned.

Correct. function (m) { return m; } doesn't effectively sanitize either. In fact it does nothing and I'm not sure why it exists within the token and combobox autocomplete helper libraries. Although there also seems to be an issue with the fix in rEPFM1b67fc6ced342237efe227af2ccab9a93272c88a. That seems to work on parameter values which have the is_list value defined and are of that type. That's what fixed the original sandbox.semantic-mediawiki.org issue within the task description. But within the other examples, Find+images[person] does not have an is_list value set, and is likely bypassing the relevant sanitization code within includes/PF_FormPrinter.php. That code is pretty convoluted and I'm not very familiar with it, but I believe the remaining issues could be addressed in two ways: 1) stop overriding Utils.escapeMarkup for the token and combobox select2 elements or 2) determine every parameter value's path through includes/PF_FormPrinter.php and ensure they are being sanitized by htmlentities (likely with the ENT_QUOTES flag enabled) or similar.

I just checked in the following change to Page Forms, which hopefully takes care of the URL query string vulnerability:

https://phabricator.wikimedia.org/rEPFM4ac1f9d4371974c823225da7273ddf2ce9b89dfd

I'm not aware of any more XSS issues with "combobox" or "tokens", but if there still are any, please let me know.

I pulled this version, but I can still recreate this with this url https://liquipedia.net/commons/Special:RunQuery/Find_images?pfRunQueryFormName=Find+images&Find+images[person]=%3Cimg%20src=%22x%22%20onerror=%22alert(1)%22%3E&Find+images[event]=&Find+images[date][day]=&Find+images[date][month]=&Find+images[date][year]=&Find+images[description]=&pf_free_text=&wpRunQuery=Run+query# (I use the git version now at 4ac1f9d4371974c823225da7273ddf2ce9b89dfd)

Feel free to check out https://liquipedia.net/commons/api.php?action=query&meta=siteinfo&siprop=extensions to verify

That page shows up fine for me, with the query string value properly escaped. Are you still seeing the problem?

At least in Firefox I am still seeing it. I've been wondering if I had a cached version of some script, but I've tried this on multiple PCs now

Yes, I see that same problem with Firefox. Which is very strange. Do you know if that new code I added to PF_FormField.php is getting called, on your wiki?

I added a call to error_log() like so

if ( !$form_submitted && $field_query_val != '' ) {
        error_log ("PageForms Test");
        if ( is_array( $field_query_val ) ) {
                $str = PFFormPrinter::getStringFromPassedInArray( $field_query_val, $delimiter );
        } else {
                $str = $field_query_val;
        }
        return htmlspecialchars( $str, ENT_QUOTES );

}

but to no avail. The error log stays empty when calling the URL as shown above.

@FO-nTTaX - I believe I've now checked in a fix for this problem; it escapes any HTML while still doing bolding on the results:

https://phabricator.wikimedia.org/rEPFM3d9d2fc601fdf690ef1b685b59448cc3ce32bfb4

Sorry that it took this long to fix... hopefully this fix works.

No worries, sometimes things take time, and that's ok. Thank you for your work on this :)

@FO-nTTaX - if you can confirm that the recent commit addresses the remaining issues on the wikis you've been testing, I think we can resolve this task, make it public and track it for the next supplemental security announcement (T270466). Thanks.

I've pulled this onto our wiki and as far as I can see it, it should be fine now. Again thank you for working on this :)

In T259433#6777234, @FO-nTTaX wrote:

I've pulled this onto our wiki and as far as I can see it, it should be fine now. Again thank you for working on this :)

Great, thanks. I've now made this task public and will include this issue within the next supplemental security announcement.

Change 658612 had a related patch set uploaded (by RhinosF1; owner: Yaron Koren):
[mediawiki/extensions/PageForms@REL1_35] Improve HTML-escaping in combobox and tokens inputs

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

Change 658613 had a related patch set uploaded (by RhinosF1; owner: Yaron Koren):
[mediawiki/extensions/PageForms@REL1_35] Add escaping of array query string params

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

Change 658613 merged by jenkins-bot:
[mediawiki/extensions/PageForms@REL1_35] Add escaping of array query string params

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

Change 658612 merged by jenkins-bot:
[mediawiki/extensions/PageForms@REL1_35] Improve HTML-escaping in combobox and tokens inputs

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

Another related change set, no bug id in the commit msg: https://gerrit.wikimedia.org/r/q/Ibe68b070ee791cd0c8e7f50eb04ac4e066b1512c