Page MenuHomePhabricator

Add comment directive to includes/GlobalBlockingHooks.php to suppress SecurityCheck-XSS warning
Closed, ResolvedPublic

Description

As surfaced via mwext-php72-phan-seccheck-docker in https://gerrit.wikimedia.org/r/578566, the phan-taint-check-plugin finds a SecurityCheck-XSS warning within includes/GlobalBlockingHooks.php within the GlobalBlocking extension. This appears to be a false positive as both of the suspect Html::rawElement (lines 188, 191) seem fine:

  1. The strings on line 189 are hard-coded html class attributes
  2. The strings on line 190 are sent to parseAsBlock()
  3. The Html::rawElement call on line 191 accepts data from GlobalBlockListPager::formatRow() whose return value appears ok from a quick look at the function. (all string data sent to parse(), other string data sent to text() should might not be harmless and Linker::commentBlock()'s output should be fine here.)

Therefore, includes/specials/GlobalBlockListPager.php GlobalBlockingHooks::onSpecialContributionsBeforeMainOutput() should be safe and warrant the appropriate comment directive suppression likely has a lightweight XSS that should be mitigated by https://gerrit.wikimedia.org/r/579003.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 10 2020, 10:33 PM
sbassett triaged this task as Medium priority.
sbassett added subscribers: Urbanecm, DannyS712, Bawolff, Reedy.

Change 578634 had a related patch set uploaded (by SBassett; owner: SBassett):
[mediawiki/extensions/GlobalBlocking@master] Add @suppress SecurityCheck-XSS to GlobalBlockingHooks function

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

sbassett updated the task description. (Show Details)Mar 10 2020, 10:50 PM
sbassett updated the task description. (Show Details)
sbassett moved this task from Backlog to Wikimedia deployed on the phan-taint-check-plugin board.
sbassett moved this task from Backlog to In Progress on the user-sbassett board.

From a very quick look, it may be caused by the 'parentheses' message not being escaped in formatRow. It's probably worth a try. (Note: I didn't check what's the effect of changing text() to escaped() on the params)

P.S. Memo: those caused-by lines are terrible. I should check why and fix.

Change 579003 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/GlobalBlocking@master] Escape a message before outputting it

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

Change 578634 abandoned by SBassett:
Add @suppress SecurityCheck-XSS to GlobalBlockingHooks function

Reason:
See above comment

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

From a very quick look, it may be caused by the 'parentheses' message not being escaped in formatRow. It's probably worth a try. (Note: I didn't check what's the effect of changing text() to escaped() on the params)

Ah, yes, perhaps. Looks like your patch tests well, so I'm happy to go with that one instead.

P.S. Memo: those caused-by lines are terrible. I should check why and fix.

Agreed, not the most helpful in that format or when generated that way :/

sbassett updated the task description. (Show Details)Mar 11 2020, 7:26 PM

Change 579003 merged by jenkins-bot:
[mediawiki/extensions/GlobalBlocking@master] Escape a message before outputting it

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

sbassett closed this task as Resolved.Mar 12 2020, 8:35 PM
sbassett reassigned this task from sbassett to Daimona.

Calling this done for now per alternative, merged patch: https://gerrit.wikimedia.org/r/579003

sbassett moved this task from In Progress to Done on the user-sbassett board.