@Bawolff - as the main author of Page Forms, I'm very interested to hear any feedback about the extension. I don't know if it's outside the scope of this particular "task", but personally I'd welcome any "review", official or unofficial.

In T149869#3006167, @dpatrick wrote:

@Dereckson I'd like to schedule this soon but we need more information. Can you update the description? You can find a template for Phabricator at https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Security_reviews#Requesting_a_review.

Done.

@Yaron_Koren could you check the information in task description is now correct?

In T149869#2767152, @Reedy wrote:

Per https://www.mediawiki.org/wiki/Review_queue a security review is so needed to deploy PageForms on the Wikimedia cluster.

Well, it's already deployed.. And we're tracking master. Depends how much it has changed.

SemanticForms seems to have been deployed without a full review, as T23602 was declined when individual content projects required it, then deployed without review to Wikitech.

Since this is an extension that various groups actively use, I'm marking this bug private before posting my review in the interest of responsible disclosure.

PageForms is a very large extension. Due to time constraints, I wasn't able to look at the entire thing in full detail. I concentrated on the form generating code. Some parts, such as integration with SMW and Cargo and some of the JS libraries I only looked at superficially. Overall, PageForms could be improved by more clearly separating escaped and non-escaped variables. Currently it can be unclear in certain places whether a variable is actually escaped or not. Some of the methods related to creating the form (e.g. PFFormPrinter::formHTML, PFAutoeditAPI) could benefit from splitting into smaller functions and having a more clear code-flow.

Security (major):

• CSRF in pf_autoedit. This is serious because it could be used to add malicious code to MediaWiki:Common.js or a user js page.
  • #autoedit (and forms in general) should not be allowed to edit pages in NS_MEDIAWIKI. There is very little use case that I see, and lots of risk. (c7de3e36)
  • Perhaps #autoedit also should not be allowed to edit protected pages to mitigate the risk of CSS-tricks (See comment in normal section).
• clickjacking in #autoedit. You should call $parser->getOutput()->preventClickjacking( true ) any time you have a parser function which adds a link or button to a page that can make a write without further interaction. (02a15b23)
• Unrecognized form elements aren't escaped properly. e.g. {{{<script>alert(document.domain)</script>}}} on form preview. (5c83f1580 and b01ba72ba6)
• <div data-bar="&#123;&#123;&#123;for template|Boo|"> }}} " onmouseover="alert('XSS On: ' + document.domain)" data-close=<div> will generate an XSS ( "PF_FormPrinter.php" line 846). There are similar problems with other field specifiers that get regexed out. Running regexes on html output is very dangerous and almost impossible to get right. I highly reccomend avoiding using regexes on any output after its been escaped. ( Attempt to address at 5c83f15802 , but not sufficient because not all of MediaWiki escapes { in attributes. Consider if you have a template named trp containing only {{{. Then the input {{#tag:pre|foo baz|class={{trp}}for template{{!}}boo{{!}}}} bar }}} " onmouseover=alert(3) will still XSS)
• SQL injection in PF_AutocompleteAPI.php:
  • substring LIKE clause has improper escaping. Its difficult to exploit this to dump user info directly due to the mangling of underscores as most columns have an underscore in their name, but you could certainly use it to dump the object cache table which may have sensitive information in it not to mention general mysql things like SELECT ... INTO OUTFILE. (Pre-mysql 5.7 Example: http://localhost:8080/w/api.php?action=pfautocomplete&substr=\%27)UNION(SELECT%09concat(keyname,%22:%22,value)%22page_title%22,0%22page_namespace%22from`objectcache`LIMIT%0920);%09--%09&category=Bn&formatversion=2&debug=1 ) (bb42c6f401)
• The exception handler in "PF_AutoeditAPI.php" assumes that exception messages are html escaped. This is a bad assumption. While true within the Autoedit api, its not generally true within MediaWiki at large. For example, try passing <script>alert(1)</script> as the value to an openlayers field. This will generate an alert both when using #autoedit and in normal form creation. 63237d
• If simpleupload is on: "PF_simpleupload.js" line 8 - $('<img class="simpleupload_prv" src="'+ mw.config.get('wgArticlePath').replace('$1', 'Special:Redirect/file/' + input.val() + '?width=100') +'">') - This appears to be an XSS (5cd1877341 - However, this would still be broken if $wgArticlePath has a query parameter, as is common when using php in CGI mode) - 7e77411d5385
• I couldn't actually get autocomplete via category to work, so I didn't test this but "includes/PF_ValuesUtils.php" line 225 combined with how autocompletion works in PageForms.js appears to be an XSS when $wgPageFormsUseDisplayTitle = true. For example, try a page named Foo with displaytitle: {{DISPLAYTITLE:<span data-foo="&quot;&gt;&lt;img src=x onerror=alert(2)&gt;">Foo</span>}} (The autocompletion part is fcbc0c4b. Displaytitle might not show formatting characters properly anymore)
• On a similar note, XSS when using values parameter - {{{field|d|input type=text with autocomplete|values=baz<img onerror=alert(53) src=x>}}} (fcbc0c4b)
• XSS in "PF_FormPrinter.php" line 338 - {{{for template|Baz12|multiple|minimum instances=1}}}Foo id="onmouseover=alert(1);//<div data-foo="baz">hover here</div>{{{end template}}}. Running Regexes over HTML almost always ends badly. I highly reccomend avoiding doing that whenever possible. (5c83f158 - Should be changed to use $matches[1] instead of [0]. Also should probably use ENT_QUOTES as future proofing against changes in html generation to use apostrophes instead of double quotes, for example like OOUI might do. Fixed in d0e7cc72dd7)
• Custom strip markers (e.g. "PF_FormUtils.php" line 411 and maybe "PF_FormPrinter.php" line 279) should include quote marks in them, to prevent html escaping bypass when strip markers for values with unescaped quotes are put inside attributes. (See core commit 939faea318d9) - 63237d
• I didn't actually test this, but "ext.pf.select2.base.js" line 81 appears to be vulnerable to an xss if the image url contains an apostrophe ('). (Attempted fix at c81949280a, but that's the wrong type of escaping. Need to use &quot; as the replacement string not \".) Second fix 8eb9b8c1a518.
• "ext.pf.select2.tokens.js" line 359 looks like an XSS (22dbbd50c9)
• PF_TreeInput.php - html not fully escaped {{{field|dog"><script>alert(1)</script>|input type=tree|structure=* foo}}} (474cb30)

Security (Normal)

• "PF_OpenLayersInput.php" line 59 - In a Wikimedia context, this is a violation of WMF privacy policy due to loading external scripts. Obviously outside of WMF, this is an ok thing for an extension to do, however it should be documented that the extension will do this, and an option should be provided to disable, for people who have high privacy requirements. (69354724)
• PF_GoogleMaps.php - ditto on the privacy policy thing. (69354724)
• {{{standard input|summary|style="background-image:url( 'http://example.com' )"}}} - "PF_FormPrinter.php" line 1178 - The style attribute is not run through Sanitizer::checkCss. This can lead to privacy violations, and in certain older browsers XSS.
• CSS tricks with #autoedit. Someone could use css such as <div style="position:static;font-size:200px;... to make the link be the entire page, and force the victim to accidentally click on it (For example, to make an admin edit a protected page, or just to cause general vandalism and mayham on a popular page). This is a difficult issue to address, while still having this feature, so it kind of becomes a trade-off as to whether the risk of this attack is worth it. Perhaps put #autoedit behind a config flag so folks can disable it if they want "high security". See also comments about #autoedit in the high section. Perhaps #autoedit should only be allowed to be used with pages anyone can edit, and protected pages require a second prompt to click through (or a "show changes" preview) before the edit goes through.
• "PF_TreeInput.php" line 134 getHTML() - width and height parameters not sanitized. Someone could use this to violate user privacy by loading external images. e.g. {{{field|foo|input type=tree|structure=* foo|width=10px; background: url(//example.com);}}} (fcf15d1e - Although it might make more sense to either be checking the combined css instead of the width/height parameter component, or if checking the width/height component to validate that it is an integer instead of validating that the numbers are valid css)

Security (minor)

• "PF_FormPrinter.php" line 792 $form_def = str_replace( 'standard input|free text', 'field|<freetext>', $form_def ); is kind of sketchy but afaict not exploitable. If you really need to do the replacement, it'd be better to replace it with something that doesn't use < and >. ( incomplete fix 7db3cd6076. Better fix in 9e7c2eb)
• the default value of current user could possibly be used to leak the viewing user's name to a third party, with a fair amount of effort. (MW already has this problem via {{REVISIONUSER}}.)
• In the interest of future proofing, its probably better to use Revision::FOR_THIS_USER instead of Revision::RAW in PF_AutoeditAPI.php.
• "PF_CreateForm.js" line 4 - Lack of urlencoding parameters.
• "includes/forminputs/PF_TextInput.php" line 151 - lack of quoting for input_id. Also wfMessage html escaping.
• PF_autoedit - DOMDocument::loadHTML() - I don't believe this is vulnerable since a doctype is specified. Nonetheless, please follow the procedures at https://www.mediawiki.org/wiki/XML_External_Entity_Processing when using DomDocument::loadHTML() (f95957aa0)

Raw system messages:

It is discouraged to have system messages directly output into the html (Unless you have a very good reason). Instead messages that need formatting should use ->parse(), potentially with ->rawParams() if there is something that doesn't have a wikitext equivalent.

• "PF_ParserFunctions.php" line 60 $inLinkStr (5c83f1580)
• "PF_FormPrinter.php" line 1509 - wfMessage( 'pf_formedit_formwarning' )(5c83f15802)
• $this->mPageTitle->getFullURL() )->text() - additionally, the url should be run through htmlspecialchars(). (e4dee73033)
• "PF_CreateForm.js" line 22 - pf_blank_error (594cc5f624 - although that's appending to the wrong DOM element possibly. Fixed in 380aafb54)
• "PF_checkboxes.js" line 45 - 'pf_forminputs_checkboxes_select_all', 'pf_forminputs_checkboxes_select_none'. (594cc5f624)

Double escaped system messages

• "PF_OpenLayersInput.php" line 78 - pf-maps-setmarker should probably be ->text() (d1a19e19d)

Non-security (major):

• "PF_ParserFunctions.php" line 30 - Don't call $parser->disableCache() unless you really need to. Instead use $parser->getOutput()->addModules instead of the equivalent method in OutputPage. (511e3cd414)
• The show preview button on Special:FormEdit seems broken (button type="button" instead of type="submit")? Nevermind, I appear to have been mistaken about this
• It appears that $wgPageFormsDatePickerSettings does not get loaded when installing from wfLoadExtension(). (I was mistaken about this. I think I must of grepped for wgPageFormsDatePickerSettings instead of PageFormsDatePickerSettings).
• "PF_CreateForm.js" line 4 - The other options ajax is totally broken on Special:CreateForm when using short urls or if the url has a #target fragment in it. (907bed119949 fixed it for short urls. Still broken if the url looks like http://localhost:8080/wiki/Special:CreateForm#foo )

Non-security (minor):

• Parts of this extension don't follow MW code style conventions. This doesn't really matter, and its totally reasonable for a third party developed extension to deviate from MW style conventions. However, if you want it consistent with current MW style conventions, you may want to consider enabling phpcs.
• Some uses of global state seem to assume only one form related page will be parsed in a given request. This is probably true, but not considered a best practise assumption. It would be better to store such state in the Parser object probably.
• "PF_FormUtils.php" line 233 cancelLinkHTML - Use of $wgTitle is strongly discouraged by modern MediaWiki conventions. The calling function should pass in a title instead.
• "PF_FormUtils.php" line 235 cancelLinkHTML - It would probably be better to detect MSIE in javascript, since then there is no chance of cache pollution (In the context of a special page, there should be no caching, so its unlikely to be an issue, but client side detection where possible is better).
• "PF_FormUtils.php" line 235 cancelLinkHTML - Its preferred in modern MediaWiki to not use Javascript: URLs for Javascript, but instead attach the javascript from a RL module. Javascript: urls would break if we ever manage to adopt Content-Security-Policy (In fairness, unlikely to happen any time soon).
• "PF_Hooks.php" line 45 - Should use $wgExtensionAssetPath
• "PF_ParserFunctions.php" line 155 - no guarantee that global state corresponds to right page, should be done via RL.
• "PF_FormUtils.php" line 54 - maxlength=200. Most MW has moved to using jquery.byteLimit so that its exactly 255 UTF-8 bytes.
• "PF_FormPrinter.php" line 426 - 'style' => "height: 13px; width: 13px; background: url($wgScriptPath/resources/src/mediawiki/images/question.png); border: none;" - This is probably not the most correct way to do this, but I don't know the correct way. Nevermind about this.
• Fatal error: Call to undefined method PFFormLinker::getFormsThatPagePointsTo() in /vagrant/mediawiki/extensions/PageForms/includes/PF_AutoeditAPI.php on line 271 - when going to http://localhost:8080/wiki/Special:FormEdit/Foo?target=Testdo (cd5ce3f580)
• "PF_FormEdit.php" line 145 - $targetTitle might be null. e.g. Special:FormEdit?form=Foo&target=%20
• Somewhere (not sure where) {{Special:RunQuery}} is calling the parser recurively when its being transcluded. You should use $wgParser->getFreshParser() in such situations to avoid UNIQ-... tags.
• [Not clear on the impact] "PF_AutocompleteAPI.php" line 316 - $whereStr = "$baseCargoTable.$baseCargoField = \"$baseValue\""; - The lack of escaping $baseValue seems concerning here. My understanding is that its processed by Cargo as untrusted input so this doesn't represent a security issue, but nonetheless, this seems kind of sketchy.
• "PF_ValuesUtils.php" line 199 - 'SORT BY cl_sortkey', - This is not a valid option name. Also, its significantly more efficient to order by cl_type, cl_sortkey. There is no index on just plain cl_sortkey. Also its kind of insane there is no LIMIT clause on this query. (8aa6d7724)
• "PF_CreatePageJob.php" line 44 - Should maybe use RequestContext::importScopedSession() instead.
• "PF_FormEditAction.php" line 230 - Not supplying method name is bad, especially for a query with unclear performance characteristics. (a5e7fec8)
• "PF_FormEditAction.php" line 231 - Adding the sort is rather pointless and just makes the db do unnessary work. (As of a5e7fec8, sort has a point)
• "PF_FormEditAction.php" line 231 - Why not group by pp_name, and select SUM( cat_pages )? (a5e7fec87)
• "PageForms.js" line 814 - Should probably use mw.config.get( 'wgUrlProtocols' ) so that validation matches wiki configuration. (73182b1dd. I'm not sure enough about context here to say for sure, but possibly the regex should also use ^ to be anchored at the beginning of the string?)
• "PageForms.js" line 1265 - async: false is bad...
• Invalid html "PF_CreateForm.php" line 704 (594cc5f624)
• Previewing direct from url (e.g. action=edit&preview=yes) a form page does not show the form.
• Missing message pf-datepicker-showweeknumbers (781eb4a97)
• "PF_popupform.js" line 752 - Looks like "one" should be "on". (d0cf4e36)
• When trying to do autocomplete for values from namespace=Template, I get js error TypeError: this.source is not a function[Learn More] jquery.ui.autocomplete.js:409:3 (I think 8df2a5656 was trying to fix this, but it still doesn't work if delimiter is null/undefined. Fixed in 2a9d716e68, 9dde7237d6)
• "PF_popupform.js" line 425 - escape is deprecated, should use encodeURIComponent instead. (84ce1f6315)
• wgPageFormsSimpleUpload is not in extension.json. (d7607f8807)

@Bawolff - wow! Thank you for this. It's a little bit ego-crushing to see this very long list, but still extremely helpful. (Though I should maybe note that not all of this code was written by me.) I have a new to-do list now.

I guess I'll mark these with a "strikethrough" as they get taken care of.

@Bawolff - I'm working my way through these; thanks again. I guess I can't check them off, because they're within your comment, which I can't edit. But I think I've now handled most of the major security issues, other than within Autoedit, which I now need to look into (I didn't write that part).

I'm also amazed to see that one feature in Special:CreateForm has apparently been broken for about a year. I guess it's not used that much!

I do have some comments and questions about some of these:

"PF_CreateForm.js" line 22 - pf_blank_error
"PF_checkboxes.js" line 45 - 'pf_forminputs_checkboxes_select_all', 'pf_forminputs_checkboxes_select_none'.

How should MW messages be escaped in JavaScript? I didn't know that this was an option.

"PF_CreatePageJob.php" line 44 - Should maybe use RequestContext::importScopedSession() instead.

I looked at RequestContext::importScopedSession() (I hadn't heard of it before), but I disagree with this - it seems like overkill to call this large function just to temporarily set $wgUser to another value.

"PageForms.js" line 1265 - async: false is bad...

There's an explanation above that line of why "async: false" is needed there (and an associated "TODO"). But by "bad", do you just mean bad for performance, or is there more than that?

Invalid html "PF_CreateForm.php" line 704

It's actually valid, when all the HTML is put together. I'm glad there's at least one case where I can fully defend the code. :)

In T149869#3064993, @Yaron_Koren wrote:

@Bawolff - I'm working my way through these; thanks again. I guess I can't check them off, because they're within your comment, which I can't edit. But I think I've now handled most of the major security issues, other than within Autoedit, which I now need to look into (I didn't write that part).

Oh huh. I didn't realize people could not edit other users comments. I always thought they could.

I'm also amazed to see that one feature in Special:CreateForm has apparently been broken for about a year. I guess it's not used that much!

I do have some comments and questions about some of these:

"PF_CreateForm.js" line 22 - pf_blank_error
"PF_checkboxes.js" line 45 - 'pf_forminputs_checkboxes_select_all', 'pf_forminputs_checkboxes_select_none'.

How should MW messages be escaped in JavaScript? I didn't know that this was an option.

You could do something like:

$('<span class="error" id="section_error"></span>' ).text( mediaWiki.msg( 'pf_blank_error' ) );

Or you could do mediaWiki.message('pf_blank_error' ).escaped() instead of .msg()

"PF_CreatePageJob.php" line 44 - Should maybe use RequestContext::importScopedSession() instead.

I looked at RequestContext::importScopedSession() (I hadn't heard of it before), but I disagree with this - it seems like overkill to call this large function just to temporarily set $wgUser to another value.

I mostly pointed it out because it looked similar to the existing code. If you think its not worth it, then don't worry about it, however importScopedSession() can help when other extensions make assumptions about the state of globals (In particular the CheckUser extension will probably record the wrong IP address unless its used)

"PageForms.js" line 1265 - async: false is bad...

There's an explanation above that line of why "async: false" is needed there (and an associated "TODO"). But by "bad", do you just mean bad for performance, or is there more than that?

Just for performance reasons (e.g. If user is doing some other interaction while the thing is loading, or something on the network times out and it ends up being in a loading state for like a minute, then no other js interaction on the page will work)

Invalid html "PF_CreateForm.php" line 704

It's actually valid, when all the HTML is put together. I'm glad there's at least one case where I can fully defend the code. :)

I mean, in <div style=\"clear: both\";> the semi-colon should come before the quote mark that ends the attribute, not after.

I mean, in <div style=\"clear: both\";> the semi-colon should come before the quote mark that ends the attribute, not after.

Aargh. Never mind, then!

If it makes you feel better, i was not able to figure out why i thought something was wrong with that code snippet until i looked at the html output in a syntax highlighter

Marginally better, I guess. :) Anyway, the set of non-security issues has turned out to be a good resource for "microtasks" for my planned GSoC project, so that's also good.

The "Fatal error: Call to undefined method PFFormLinker::getFormsThatPagePointsTo() in /vagrant/mediawiki/extensions/PageForms/includes/PF_AutoeditAPI.php on line 271 - when going to http://localhost:8080/wiki/Special:FormEdit/Foo?target=Testdo" problem was caused by

https://github.com/wikimedia/mediawiki-extensions-PageForms/commit/e3f2972d7fe74a5ebbbc586e0659cb9eb8f6d7e1#diff-bb0725ce4a9ff2588fd1339cf0c8194c

@Yaron_Koren would you know how to fix that please as I doint know what to replace that with?

I have a few more questions about some of these:

"PF_ParserFunctions.php" line 30 - Don't call $parser->disableCache() unless you really need to. Instead use $parser->getOutput()->addModules instead of the equivalent method in OutputPage.

This is actually line 309, but anyway - $parser->getOutput()->addModules() is already what's being called. Do you have any other suggestions? (If I understand what you're saying correctly.)

The show preview button on Special:FormEdit seems broken (button type="button" instead of type="submit")?

What does this mean - the "Show preview" button doesn't work for you? I just tested it, and it works fine for me.

"PF_FormPrinter.php" line 426 - 'style' => "height: 13px; width: 13px; background: url($wgScriptPath/resources/src/mediawiki/images/question.png); border: none;" - This is probably not the most correct way to do this, but I don't know the correct way.

What's wrong with this approach?

Actually, two more questions:

Custom strip markers (e.g. "PF_FormUtils.php" line 411 and maybe "PF_FormPrinter.php" line 279) should include quote marks in them, to prevent html escaping bypass when strip markers for values with unescaped quotes are put inside attributes. (See core commit 939faea318d9)

Sorry, I didn't understand this. What should I do in the code?

SQL injection in PF_AutocompleteAPI.php:
substring LIKE clause has improper escaping

How do I fix this? Would it be enough to escape quotation marks and parentheses?

"PF_ParserFunctions.php" line 30 - Don't call $parser->disableCache() unless you really need to. Instead use $parser->getOutput()->addModules instead of the equivalent method in OutputPage.

This is actually line 309, but anyway - $parser->getOutput()->addModules() is already what's being called. Do you have any other suggestions? (If I understand what you're saying correctly.)

Remove line 309, as addModules() already takes care of ensuring the js will be shown.

SQL injection in PF_AutocompleteAPI.php:
substring LIKE clause has improper escaping

How do I fix this? Would it be enough to escape quotation marks and parentheses?

Use Database::buildLike() ?

@Platonides - thanks, that's very helpful! It turns out that the $parser->disableCache() call in the code is from 2010 - I removed it and everything still works fine. And buildLike() is working well too; I didn't know about that one.

@Paladox - thanks for pinpointing the earlier commit! I just removed the call to getFormsThatPagePointsTo() - nothing replaced it.

@Bawolff (or anyone else) - actually, I have one more question:

In the interest of future proofing, its probably better to use Revision::FOR_THIS_USER instead of Revision::RAW in PF_AutoeditAPI.php.

Can you explain what that's about, briefly?

What does this mean - the "Show preview" button doesn't work for you? I just tested it, and it works fine for me.

Did you test on different browsers? I think firefox treats the button differently than IE

"PF_FormPrinter.php" line 426 - 'style' => "height: 13px; width: 13px; background: url($wgScriptPath/resources/src/mediawiki/images/question.png); border: none;" - This is probably not the most correct way to do this, but I don't know the correct way.

Whoops, that was partially meant as a comment for myself to come back to and figure out if its correct or not. I was thinking this should somehow be using resource loader instead, but I don't really know how to do that. Its probably fine as is, but you may want to ask a frontend dev what current best practise is. (As an aside though, it should use $wgResourceBasePath instead of $wgScriptPath)

Custom strip markers (e.g. "PF_FormUtils.php" line 411 and maybe "PF_FormPrinter.php" line 279) should include quote marks in them, to prevent html escaping bypass when strip markers for values with unescaped quotes are put inside attributes. (See core commit 939faea318d9)

Sorry, I didn't understand this. What should I do in the code?

So the problem with strip markers is if they get in an attribute, sometimes they can evade escaping. For example, if you have a strip marker abcd that gets replaced with the safe html <div class="onmouseover=alert(1) d">foo</div>, this becomes unsafe, if the strip marker is in an attribute (e.g. <span data-foo="abcd"> expands to <span data-foo="<div class="onmouseover=alert(1) d">foo</div>foo</div>">, which the browser will interpret as a script.

To fix this, just ensure that all strip marker names have quote characters in them (instead of strip markers named abcd, use '"abcd"'. This will ensure that strip markers are only allowed in places where quotes are ok,

In the interest of future proofing, its probably better to use Revision::FOR_THIS_USER instead of Revision::RAW in PF_AutoeditAPI.php.

Can you explain what that's about, briefly?

Revision::RAW means fetch the revision even if the current user is not allowed to view it (in case of revision delete). Revision::FOR_THIS_USER will refuse to load the revision if the current user is not allowed to view it. At the moment we don't have per -revision permissions for the top revision, so this doesn't matter, but it might be good to use Revision::FOR_THIS_USER in case of the unlikely event that ever changes.

Thanks for these responses. I just tested "Show preview" with Firefox, MS Edge and IE on Windows (and I had tested it with Chrome earlier) and it works with all of these browsers. The preview does take a little while to show up, which may be the problem. (A loading/spinning image might help.) But if you're seeing an actual problem with it on some browser, it's probably worth creating a separate bug report for it.

In T149869#3078560, @Yaron_Koren wrote:

Thanks for these responses. I just tested "Show preview" with Firefox, MS Edge and IE on Windows (and I had tested it with Chrome earlier) and it works with all of these browsers. The preview does take a little while to show up, which may be the problem. (A loading/spinning image might help.) But if you're seeing an actual problem with it on some browser, it's probably worth creating a separate bug report for it.

I can't reproduce that issue now. I must have been mistaken earlier. Sorry about that.

Ah! No need to apologize, it's great to hear that at least one of these issues does not need fixing. :)

On a more serious note, over half of these items have now been fixed. I don't know how best to indicate which ones are fixed, or whether you would want to check each one afterwards - let me know if you have any thoughts on that.

On a more serious note, over half of these items have now been fixed. I don't know how best to indicate which ones are fixed, or whether you would want to check each one afterwards - let me know if you have any thoughts on that.

I've edited my review comment to indicate in parenthesis the commits that fix the issue where you've made one + any comments I might have. I also left some other comments on some of the patches in gerrit.

It would also be good if phpunit regression tests could be added to ensure that once one of the issues is fixed, it won't accidentally be reintroduced again (You probably would want to keep these private until you announce the security fixes, so that example exploit code isn't public until everything is all fixed)

I agree about the testing - Page Forms could use a lot more unit tests, in general.

I'm not sure this one is true:

"ext.pf.select2.tokens.js" line 359 looks like an XSS

At least, I hope not, because that HTML can't be escaped - it's actual HTML (a text input) being placed there.

By the way, I've by now fixed a bunch more of these, or at least tried to.

In T149869#3126071, @Yaron_Koren wrote:

I agree about the testing - Page Forms could use a lot more unit tests, in general.

I'm not sure this one is true:

"ext.pf.select2.tokens.js" line 359 looks like an XSS

At least, I hope not, because that HTML can't be escaped - it's actual HTML (a text input) being placed there.

So the code is:

        function pfTokensTurnIntoInput( event ) {
                var currentSelect = $(event.target);
                var tokensInput = $(currentSelect).parent().parent().parent().parent();
                var itemValue = currentSelect.html();
                if ( itemValue.indexOf("<input type") === -1 ) {
                        currentSelect.html("<input type=\"text\" value=\"" + itemValue + "\">");
...

and pfTokensTurnIntoInput is a doubleclick handler for li.select2-search-choice

So this will be an XSS if itemValue contains a double quote (") character. I'm not exactly sure about the html where this function is applied to, but it seems very likely this is possible since itemValue is the result of $(event.target).html(), which is the normalized html (which means any &quot; will be turned back into " except inside of attributes) of whatever was double clicked. I assume that the user can add tokens with quote characters in them, so this is almost certainly an XSS. I don't see any reason why itemValue cannot be escaped here. its being inserted into an attribute value.

Ah, oops - I didn't think about quote escaping. Yes, you're right. I just checked in what I hope is a fix for this one.

We should probably also get CVE numbers for this issue

Do we have a specific documented process to request a CVE number for MediaWiki related issues? Or do we need to request them per vulnerabilities to MITRE?

In T149869#3161686, @Dereckson wrote:

Do we have a specific documented process to request a CVE number for MediaWiki related issues? Or do we need to request them per vulnerabilities to MITRE?

That is one way of doing it. We usually request them from RedHat, but as they are out of CVE numbers, and are waiting for some more from MITRE... So for this one, we asked the debian guys (who as they package mediawiki now, can help) for them, and @MoritzMuehlenhoff helped us out for T140591

He may be able to help issue them for PageForms too, but as they don't package/release it.... I don't know if they can/will.

He'd CC'd for this comment, so hopefully he can advise :)

For CVE IDs in extensions not bundled with Debian, better use https://cveform.mitre.org/, they'll usually assign these in less than 24 hours (and usually even on weekends).

@yaron Sorry I wasn't around recently. I've reformatted my comment to more clearly indicate what is fixed and what still remains to be done. I believe I've updated the comment to include all the commits made to PageForms up to this point in time.

@Bawolff - thanks for sticking with this, and for updating the entries. Adding the checkboxes and strikethroughs helps a lot with the readability. And it looks like you added a few new issues - hopefully I can fix them faster than you add them. :)

I have a few quibbles with the current list. I believe the following are not actual problems (though I'm less sure about the second one):

The show preview button on Special:FormEdit seems broken (button type="button" instead of type="submit")?

It appears that $wgPageFormsDatePickerSettings does not get loaded when installing from wfLoadExtension().

I believe the following three have been fixed:

Unrecognized form elements aren't escaped properly. e.g. {{{<script>alert(document.domain)</script>}}} on form preview. (I think 5c83f1580 aims to fix it. Not sure if it works, as when testing MW seems to freeze)

"PF_FormEditAction.php" line 230 - Not supplying method name is bad, especially for a query with unclear performance characteristics.

("__ METHOD __" is passed in to the select() call, which passes in the method name - no?)

"PF_CreateForm.js" line 4 - Lack of urlencoding parameters.

(There was at least an attempt to fix this, at 907bed119949)

And I believe this one is no longer valid:

"PF_FormEditAction.php" line 231 - Adding the sort is rather pointless and just makes the db do unnessary work.

For the "popular" forms, their names are now listed in the same order in which they were retrieved from the database, so now the sorting is necessary.

The show preview button on Special:FormEdit seems broken (button type="button" instead of type="submit")?

It appears that $wgPageFormsDatePickerSettings does not get loaded when installing from wfLoadExtension().

Looks like I was wrong about both these things. whoops.

Unrecognized form elements aren't escaped properly. e.g. {{{<script>alert(document.domain)</script>}}} on form preview. (I think 5c83f1580 aims to fix it. Not sure if it works, as when testing MW seems to freeze)

On my recent test, this seems to cause an infinite loop, so I'm not sure I'd call this fixed. The new code no longer updates $start_position, so I think it parses the invalid form element over and over again.

"PF_CreateForm.js" line 4 - Lack of urlencoding parameters.

Its better now, but still kind of broken if the url has a hash (#) in it.

I've checked off the other one's that you mentioned.

I just checked in some changes that I think fix the two "tree input" issues, as well as the other checkCSS() issue, and the infinite loop one. Thanks for diagnosing all of these.

Actually, one other quibble I should have noted earlier:

CSRF in pf_autoedit. This is serious because it could be used to add malicious code to MediaWiki:Common.js or a user js page.

In c7de3e3648b0, I changed #autoedit to only work on namespaces contained in $wgContentNamespaces - which, by default, does not include "MediaWiki:" or "User:". So while there are still potential issues, I don't think this counts as a major security risk any more.

In T149869#3202090, @Yaron_Koren wrote:

Actually, one other quibble I should have noted earlier:

CSRF in pf_autoedit. This is serious because it could be used to add malicious code to MediaWiki:Common.js or a user js page.

In c7de3e3648b0, I changed #autoedit to only work on namespaces contained in $wgContentNamespaces - which, by default, does not include "MediaWiki:" or "User:". So while there are still potential issues, I don't think this counts as a major security risk any more.

That significantly reduces the risk. However autoedit should still enforce having an edit token as csrf could be used as part of a vandalism attack or to target a protected page in the content namespace. Since the autoedit feature already requires javascript, it should be not very disruptive to instead use new mw.Api().postWithToken(...) (from the mediawiki.api module) to make the api request instead of $.ajax

@Yaron_Koren Ping on this? How's it going?

@Bawolff - thanks for the reminder on this. I haven't looked at this since April, pretty much, but I've wanted to start looking into it again, and this seems like a good time to do it.

Would it be possible to split off all the non-security issues into a separate bug report, to separate the two kinds of problems? Or do security reviews tend to also include non-security stuff?

Feel free to split it up anyway you want

Alright. Do you know if there's any easy way to copy over everything, including the checkboxes? A simple copy/paste doesn't do that.

In T149869#3847002, @Yaron_Koren wrote:

Alright. Do you know if there's any easy way to copy over everything, including the checkboxes? A simple copy/paste doesn't do that.

In the upper-right corner of the comment there is a drop down arrow, if you select "view remarkup" it will give you a thing you can copy and paste.

Okay, thanks. I put all the remaining non-security issues in T183213 - which will hopefully make all of this more manageable.

@Bawolff - I just checked in some changes, and I believe the following are fixed:

• "If simpleupload is on..." - I fixed the URL query string issue, although I believe this was no longer a security issue anyway
• "I didn't actually test this,..."

Also, it seems to me that this one was fixed a while ago...?

"includes/forminputs/PF_TextInput.php" line 151 - lack of quoting for input_id. Also wfMessage html escaping.

I also looked into this one:

In the interest of future proofing, its probably better to use Revision::FOR_THIS_USER instead of Revision::RAW in PF_AutoeditAPI.php."

...but it seems to me that that's not a good solution. In order to get the form to output the right content on the page, it has to know exactly what's on the page now - even if the current user is not allowed to view it. Perhaps a user not allowed to view the current contents of a page should not be allowed to call #autoedit on it either, but that would have to be a different solution.

This may also be done:

Parts of this extension don't follow MW code style conventions. This doesn't really matter, and its totally reasonable for a third party developed extension to deviate from MW style conventions. However, if you want it consistent with current MW style conventions, you may want to consider enabling phpcs.

Thanks to heroic efforts by Umherirrender, Page Forms now has phpcs enabled, and passes its tests. This may or may not fix this issue, depending on what exactly you meant by "MW code style conventions".

(By the way, this is a non-security issue, and I know I copied the remaining non-security issues to another bug report, but it still seems better now to check them off here.)

Bump, any more issues to sort out before closing this as resolved? :)

@Paladox - yes, this is far from resolved - there are, by my count, 12 security-related issues remaining for Page Forms. I believe most of them are minor at this point (even some of the ones listed as "major" or "normal" have been reduced in severity), but they still haven't been fully resolved.

ping. This bug has been open for a really long time now.

So this has now been marked as secret for 22 months. I appreciate that some of the lesser issues still aren't fixed, but security tasks are intended to only be kept secret for a limited amount of time to allow for fixes to be applied. Tasks like these are not supposed to be kept confidential indefinitely.

To that end, I intend to make this bug public in 3 weeks (on Jan 3). I picked this date rather arbitrarily and am open to modifying the timeframe if you desire as long as there is some sort of reasonable timeframe in place.

@Bawolff - sorry for neglecting this, and thanks for following up on it. I have no problem with the January 3 date - hopefully it will give me the impetus to at least fix the remaining "major" security bugs in that time.

Okay, I think four more of the "major security" issues can now be checked off:

• "I didn't actually test this, but..." - as noted earlier in the thread, this was actually fixed about a year ago, in 8eb9b8c1a518.

• "The exception handler..." and "Custom strip markers..." - I believe I fixed these two in 63237d1d63d1.

• "If simpleupload is on..." - the remaining issue was not a security issue, but there was indeed still a problem with the handling - I think I just fixed it in 7e77411d5385.

Also as noted earlier, "Parts of this extension don't follow...", though not a security issue, I think was also fixed about a year ago.

Ok, its Jan 7. Making this public

We've moved this task to "Frozen", where it will remain unless/until it is clear that work will continue.

Hi @Yaron_Koren -

If no further work is going to be done for this particular review we will mark this as resolved on July 30. After that date, you are welcome to submit a request for a new review, should it be needed. Thank you!

Jennifer

@Bawolff - well, it's been a long time. But I just checked in a change that I think fixes the 4th major security issue listed, the one that starts with "<div data-bar". The change can be found here:

https://phabricator.wikimedia.org/rEPFM7865d640d86bf17d76dfa1d4498478f09df669a5

It would be great if you could look at this fix and let me know if it does indeed solve the problem.... assuming you still do that sort of thing!

By the way, it's my belief that this is the last remaining major security issue, of the original review. Yes, there are two other issues listed in that section, but they were both reduced to the point where I don't think they're major any more, and possibly not even security-related...

It might be time to do a new security review, now that the possibly upcoming "WikiJournal" project has expressed some interest in having Page Forms installed on their site. But that's another story.

In T149869#5654801, @Yaron_Koren wrote:

It would be great if you could look at this fix and let me know if it does indeed solve the problem.... assuming you still do that sort of thing!

I believe @Bawolff is still around in a volunteer capacity, though I don't believe he's been very active with recent security review requests.

It might be time to do a new security review...

You could definitely submit a new review request (slightly updated process described here) however since PageForms does not appear to be deployed on any production Wikimedia wikis at this time, the Security-Team would most likely have to deprioritize the review request.