Page MenuHomePhabricator

Release taint-check 2.0.0
Closed, ResolvedPublic

Description

The 2.x branch is now ready for release. It supports PHP7.0+ and php-ast 1.0.0+ (1.0.1 is strongly recommended on PHP7.1+).
Please release 2.0.0 from the content of the 2.x branch.

Event Timeline

Daimona created this task.Jul 3 2019, 7:45 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 3 2019, 7:45 AM
Daimona renamed this task from Release taint-check 2.0 to Release taint-check 2.0.0.Jul 3 2019, 8:21 AM
Daimona updated the task description. (Show Details)

Change 520381 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Merge remote-tracking branch 'origin/2.x'

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

Change 520381 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Merge remote-tracking branch 'origin/2.x'
https://gerrit.wikimedia.org/r/520381

In case the 2.x branch must be merged into master first.

So here's what I'd propose

  • Create the new release:
git clone "https://gerrit.wikimedia.org/r/mediawiki/tools/phan/SecurityCheckPlugin"
git checkout 2.x
git tag -a v2.0.0 -m "Releasing version 2.0.0 - see 2.x development branch"
  • [any further testing]
  • merge r520381

I think this is all we can do until the composer-docker ast issues are resolved.

sbassett triaged this task as Normal priority.Jul 3 2019, 8:07 PM

So here's what I'd propose

  • Create the new release:
git clone "https://gerrit.wikimedia.org/r/mediawiki/tools/phan/SecurityCheckPlugin"
git checkout 2.x
git tag -a v2.0.0 -m "Releasing version 2.0.0 - see 2.x development branch"
  • [any further testing]
  • merge r520381

I think this is all we can do until the composer-docker ast issues are resolved.

Fine by me. Will you do the actual release, or should I? Then I'll let you know the results of the final testing on a bunch of extensions. And then yeah, we'll have to wait for ast in containers.

Of note, there are a few +2 patches coming next in 2.x. If it's fine by you, I'd like to merge them in 2.x after the release (letting jenkins test them possibly), and then move everything unmerged/unreviewed to master.

sbassett added a comment.EditedJul 3 2019, 8:25 PM

Done: https://gerrit.wikimedia.org/r/admin/projects/mediawiki/tools/phan/SecurityCheckPlugin,tags

Feel free to merge the +2 patch sets to 2.x now. And we might want to have a discussion on Phab as to which ones make the most sense for a 2.1.0 or whatever release. And when that might happen.

Done: https://gerrit.wikimedia.org/r/admin/projects/mediawiki/tools/phan/SecurityCheckPlugin,tags
Feel free to merge the +2 patch sets to 2.x now. And we might want to have a discussion on Phab as to which ones make the most sense for a 2.1.0 or whatever release. And when that might happen.

Great, thanks! I'm going to do the final testing tomorrow (UTC+2).
As for commits still in 2.x, most of them should be just cleanups or minor changes. I'll look again tomorrow and will let you know. It could also make sense to release a 2.0.1 version with +2ed commits, if they're all minor changes and ast isn't ready in the next couple of days.

As for commits still in 2.x, most of them should be just cleanups or minor changes. I'll look again tomorrow and will let you know. It could also make sense to release a 2.0.1 version with +2ed commits, if they're all minor changes and ast isn't ready in the next couple of days.

Works for me, thanks a ton (again) for all of your work on this. There's absolutely no way this would've gotten done this summer without your efforts.

Extension testing results

Using mwext-fast-config, on master. Using PHP 7.3.4 with php-ast 1.0.1.

AbuseFilter
includes/AbuseFilterModifyLogFormatter.php:34 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)
includes/AbuseFilterModifyLogFormatter.php:35 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)
includes/AbuseFilterModifyLogFormatter.php:42 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)
includes/AbuseFilterModifyLogFormatter.php:43 SecurityCheck-DoubleEscaped Calling method \AbuseFilterModifyLogFormatter::makePageLink() in \AbuseFilterModifyLogFormatter::extractParameters that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Message::escaped)

Notes: These seem to be false positives, and need some investigation.
Summary: 4 issues, 4 false positives

CheckUser
includes/specials/SpecialCheckUser.php:1600 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1602 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1607 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1611 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1613 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1617 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1619 SecurityCheck-DoubleEscaped Calling method \Xml::label() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +367) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCheckUser.php:1625 SecurityCheck-DoubleEscaped Calling method \Xml::submitButton() in \SpecialCheckUser::getBlockForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +473) (Caused by: Builtin-\Message::escaped)

Notes: I checked the first one for Xml::label, and in fact it's double escaped (so the other should be DE too). The submitButton is double escaped, too.
Summary: 8 issues, all correct

CentralAuth
includes/specials/SpecialCentralAuth.php:343 SecurityCheck-DoubleEscaped Calling method \Xml::fieldset() in \SpecialCentralAuth::showInfo that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +613) (Caused by: Builtin-\Message::escaped)
includes/specials/SpecialCentralAuth.php:404 SecurityCheck-DoubleEscaped Calling method \Xml::fieldset() in \SpecialCentralAuth::showWikiLists that outputs using tainted argument $legend. (Caused by: ../../includes/Xml.php +613) (Caused by: includes/specials/SpecialCentralAuth.php +400)
includes/specials/SpecialCentralAuth.php:633 SecurityCheck-DoubleEscaped Calling method \SpecialCentralAuth::foreignLink() in \SpecialCentralAuth::formatBlockStatus that outputs using tainted argument $text. (Caused by: includes/specials/SpecialCentralAuth.php +731) (Caused by: includes/specials/SpecialCentralAuth.php +630)
includes/specials/SpecialCentralAuth.php:801 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:802 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:809 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:816 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:817 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:824 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:825 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAuth.php:833 SecurityCheck-DoubleEscaped Calling method \Xml::radioLabel() in \SpecialCentralAuth::showStatusForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/Xml.php +449) (Caused by: Builtin-\Message::parse)
includes/specials/SpecialCentralAutoLogin.php:594 SecurityCheck-DoubleEscaped Calling method \Xml::encodeJsCall() in \SpecialCentralAutoLogin::execute that outputs using tainted argument $[arg #2]. (Caused by: Builtin-\Xml::encodeJsCall) (Caused by: includes/CentralAuthHooks.php +1194)

Notes: The ones for Xml::fieldset are genuine; ditto for radioLabel. Couldn't test the others due to troubles with CentralAuth on my wiki.
Summary: 12 issues, 10 genuine, the other 2 likely genuine

Echo
includes/formatters/EchoModelFormatter.php:17 SecurityCheck-DoubleEscaped Calling method \wfExpandUrl() in \EchoModelFormatter::formatModel that outputs using tainted argument $[arg #1]. (Caused by: includes/formatters/EchoHtmlEmailFormatter.php +156) (Caused by: includes/formatters/EchoModelFormatter.php +13)
includes/formatters/EchoModelFormatter.php:22 SecurityCheck-DoubleEscaped Calling method \wfExpandUrl() in \EchoModelFormatter::formatModel that outputs using tainted argument $[arg #1]. (Caused by: includes/formatters/EchoHtmlEmailFormatter.php +156) (Caused by: includes/formatters/EchoModelFormatter.php +20)
scripts/generatecss.php:26 SecurityCheck-XSS Echoing expression that was not html escaped (Caused by: scripts/generatecss.php +25; scripts/generatecss.php +6)

Notes: The first two are a bit cryptic, they seem false positives, but I'm unsure. The third one is a false positive for CLI script, something already known.
Summary: 3 issues, 1 FP, 2 likely FPs

Flow
includes/Formatter/FeedItemFormatter.php:49 SecurityCheck-DoubleEscaped Calling method \FeedItem::__construct() in \Flow\Formatter\FeedItemFormatter::format that outputs using tainted argument $[arg #2]. (Caused by: ../../includes/changes/FeedItem.php +145) (Caused by: includes/Formatter/FeedItemFormatter.php +22)
includes/Import/OptInController.php:393 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::archiveExistingTalkpage that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Import/OptInController.php +388; includes/Import/OptInController.php +389; includes/Import/OptInController.php +390)
includes/Import/OptInController.php:519 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editBoardDescription that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +518; includes/Import/OptInController.php +519)
includes/Import/OptInController.php:583 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/OptInController.php +578; includes/Import/OptInController.php +583)
includes/Import/OptInController.php:588 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::removeArchiveTemplateFromWikitextTalkpage that outputs using tainted argument $[arg #2]. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Conversion/Utils.php +67; includes/Conversion/Utils.php +74; includes/Import/OptInController.php +588)
includes/Import/OptInController.php:636 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $newContent. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +635)
includes/Import/OptInController.php:636 SecurityCheck-DoubleEscaped Calling method \Flow\Import\OptInController::createRevision() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $[arg #2]. (Caused by: includes/Import/OptInController.php +313) (Caused by: includes/Conversion/Utils.php +67; includes/Conversion/Utils.php +74; includes/Import/OptInController.php +588; includes/Import/OptInController.php +635)
includes/Import/OptInController.php:638 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Import\OptInController::editWikitextContent that outputs using tainted argument $newContent. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Import/OptInController.php +635)
includes/Import/TemplateHelper.php:76 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/TemplateHelper.php +76)
includes/Import/Wikitext/ImportSource.php:73 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Import/Wikitext/ImportSource.php +73)
includes/Model/AbstractRevision.php:423 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Model\AbstractRevision::getContent that outputs using tainted argument $raw. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Model/AbstractRevision.php +401)
includes/Model/AbstractRevision.php:546 SecurityCheck-DoubleEscaped Assigning a tainted value to a variable that later does something unsafe with it (Caused by: includes/Model/AbstractRevision.php +546)
includes/Model/AbstractRevision.php:568 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \Flow\Model\AbstractRevision::setContent that outputs using tainted argument $content. (Caused by: includes/Import/OptInController.php +588) (Caused by: includes/Model/AbstractRevision.php +546; includes/Model/AbstractRevision.php +562; includes/Model/AbstractRevision.php +568)
includes/View.php:87 SecurityCheck-DoubleEscaped Calling method \Flow\View::renderApiResponse() in \Flow\View::show that outputs using tainted argument $apiResponse. (Caused by: includes/View.php +367) (Caused by: includes/View.php +71)
maintenance/FlowReserializeRevisionContent.php:103 SecurityCheck-DoubleEscaped Calling method \Flow\Conversion\Utils::convert() in \FlowReserializeRevisionContent::execute that outputs using tainted argument $wikitext. (Caused by: includes/Import/OptInController.php +588) (Caused by: maintenance/FlowReserializeRevisionContent.php +102)

Notes: All of them are a bit cryptic and I didn't really investigate. All I can say is that they seem to be sharing the same root cause, which has to do with Utils::convert.
Summary: 15 issues, unknown validity

SpamBlacklist
includes/SpamBlacklistLogFormatter.php:7 SecurityCheck-DoubleEscaped Calling method \htmlspecialchars() in \SpamBlacklistLogFormatter::getMessageParameters that outputs using tainted argument $[arg #1]. (Caused by: includes/SpamBlacklistLogFormatter.php +6)

Notes: Should be the same as AbuseFilter
Summary: 1 issue, 1 FP

Thanks

No issues

ContentTranslation

No issues

UserMerge

No issues

Conclusions

IMHO, this is acceptably good. Echo and Flow would need some more investigation (especially Flow), but that's something that can be delayed I guess. Now I'm going to investigate the issue in LogFormatters, to see if it can be resolved easily. Aside from that, I guess we can move on, merging r520381 and starting to update extensions as soon as php-ast is ready.

Now I'm going to investigate the issue in LogFormatters, to see if it can be resolved easily. Aside from that, I guess we can move on, merging r520381 and starting to update extensions as soon as php-ast is ready.

Found it. Once again, it has to do with the param taints specified in the plugin config. To put it simply, the AbuseFilter code is calling LinkRenderer::makeKnownLink passing in an escaped second argument. makeKnownLink's second argument has ESCAPES_HTML in the plugin config, so we have escaped+escaped => DoubleEscaped warning.
Now, the annotation for makeKnownLink is partly correct. More specifically, it really escapes HTML (via HtmlArmor as the comment says), *but* just as long as you pass it the HTML as string. Instead, if you pass it an HtmlArmor, it will be output as-is. Which is to say, you have to escape the content.
The code above is calling an intermediary function (makePageLink) with an escaped string, and that function puts it in an HtmlArmor and calls makeKnownLink, and we get the warning.

So, the solution.

We'd need a way to tell the plugin that makeKnownLink [1] only escapes HTML if it's passed a string, and not an HtmlArmor. Or maybe tweak param taint for MW functions. Or fix it somewhere else in the code. But in any case, I don't see anything too easy.

[1] - Note that everything I'm saying also stands for related functions: makeLink, makePreloadedLink and buildAElement

I can't see any easy solution that we can implement now. Moreover, it should come together with better handling of HtmlArmor (as a sort of follow-up of https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/508312/), so I'd like to wait.

I sent https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520789/ on top of the 2.x tip and will continue the work there in the next days.

@sbassett If you're fine with the current state, I'll leave you the honor to merge https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520381/-1..2.

Change 520381 merged by SBassett:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Merge remote-tracking branch 'origin/2.x'

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

@Daimona:

IMHO, this is acceptably good.

Agreed - tests above look good, thanks. I think we're probably good with keeping the 2.x branch around for a while, possibly indefinitely? Even after we merge to master. Let me know if you feel otherwise for any reason.

And... just merged https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520381/-1..2

Daimona closed this task as Resolved.Jul 6 2019, 7:51 AM
Daimona assigned this task to sbassett.

@Daimona:

IMHO, this is acceptably good.

Agreed - tests above look good, thanks. I think we're probably good with keeping the 2.x branch around for a while, possibly indefinitely? Even after we merge to master. Let me know if you feel otherwise for any reason.
And... just merged https://gerrit.wikimedia.org/r/#/c/mediawiki/tools/phan/SecurityCheckPlugin/+/520381/-1..2

Sure, I think keeping the branch alive won't hurt; actually, I see it as a quite common practice to leave development branches in place. And now we're done with this task.

In the next days, I'm going to merge +2ed patches and cherry pick 'em to master. I still have to check what could make sense to add in a minor release, and will provide updates in a separate task. I'm also going to cherry pick and abandon the remaining patches, to empty the 2.x branch. And of course, I'll have to look at fixing a couple of known issues introduced with the upgrade, although for convenience I may delay that until we upgrade to phan 2.2, which shouldn't happen as part of the next minor release due to PHP70 incompatibility.