Maniphest T202380

Add phan-taint-check-plugin to Scribunto extension
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Umherirrender
	Aug 21 2018, 11:35 AM

Description

Would be nice to add phan-taint-check-plugin to Scribunto extensions

<?xml version="1.0" encoding="ISO-8859-15"?>
<checkstyle version="6.5">
  <file name="./includes/common/ScribuntoContent.php">
    <error line="91" severity="warning" message="Calling method \Parser::parse() in \ScribuntoContent::fillParserOutput that outputs using tainted argument $docWikitext. (Caused by: Builtin-\Parser::parse) (Caused by: ./includes/common/ScribuntoContent.php +75)" source="SecurityCheck-DoubleEscaped"/>
  </file>
  <file name="./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php">
    <error line="62" severity="warning" message="Echoing expression that was not html escaped (Caused by: ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +10)" source="SecurityCheck-XSS"/>
    <error line="63" severity="warning" message="Argument to require, include or eval is user controlled (Caused by: ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +10)" source="SecurityCheck-OTHER"/>
    <error line="65" severity="warning" message="Echoing expression that was not html escaped (Caused by: ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +32; ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +44; ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +41; ./includes/engi...)" source="SecurityCheck-XSS"/>
    <error line="66" severity="warning" message="Argument to require, include or eval is user controlled (Caused by: ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +32; ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +44; ./includes/engines/LuaCommon/lualib/ustring/make-normalization-table.php +41; ./includes/engi...)" source="SecurityCheck-OTHER"/>
  </file>
</checkstyle>

make-normalization-table.php it seems issues cannot be suppressed on global state

Details

Subject	Repo	Branch	Lines +/-
seccheck for Scribunto	integration/config	master	+1 -1
Suppress phan-taint-check false positives in make-normalization-table.php	mediawiki/extensions/Scribunto	master	+18 -4
Use annotations for taint in Parser & ParserOutput.	mediawiki/core	master	+7 -0
Move builtin taints for Parser&ParserOutput into inline annotations	mediawiki/tools/phan/SecurityCheckPlugin	master	+0 -21

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Legoktm	T201219 Enable phan-taint-check-plugin on all Wikimedia-deployed repositories after getting it to pass
Resolved	Bawolff	T202380 Add phan-taint-check-plugin to Scribunto extension
Declined	None	T203217 convert scribunto's ustring maintenance scripts (e.g. make-normalization-table.php) to use MW's maintenance class

Event Timeline

Umherirrender created this task.Aug 21 2018, 11:35 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 21 2018, 11:35 AM

Umherirrender moved this task from Backlog to Wikimedia deployed on the phan-taint-check-plugin board.Aug 21 2018, 11:36 AM

Umherirrender added a parent task: T201219: Enable phan-taint-check-plugin on all Wikimedia-deployed repositories after getting it to pass.Aug 21 2018, 12:40 PM

Bawolff added a subtask: T203217: convert scribunto's ustring maintenance scripts (e.g. make-normalization-table.php) to use MW's maintenance class.Aug 31 2018, 1:33 PM

Bawolff closed subtask T203217: convert scribunto's ustring maintenance scripts (e.g. make-normalization-table.php) to use MW's maintenance class as Declined.Aug 31 2018, 3:23 PM

Change 456653 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/Scribunto@master] Suppress phan-taint-check false positives in make-normalization-table.php

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

gerritbot added a project: Patch-For-Review.Aug 31 2018, 3:46 PM

So the last warning is about throwing Html::rawElement() into Parser::parse. (Since Html::rawElement is outputting escaped html, and parse also escapes stuff, it considers it double escaping). So it probably makes sense not to have Parser::parse warn for escaped content as you often want to provide it escaped content. But if we do that, that means it stops warnings about $wgParser->parse( wfMessage('foo')->parse()) which we do want to know about as that would be double-escaping.

In T202380#4549116, @Bawolff wrote:

Since Html::rawElement is outputting escaped html,

It escapes the stuff that goes into the tag's attributes, but anything in the $contents isn't touched.

Also of note is that many HTML tags are also valid wikitext and their attributes don't get double-escaped by the Parser.

It escapes the stuff that goes into the tag's attributes, but anything in the $contents isn't touched.

What I meant was, that Html::rawElement creates html, so it marks the return value as escaped html (Checking that $contents is properly escaped happens separately), so that if someone runs htmlspecialchars( Html::rawElement( ... ) ) we get a double escaping warning.

Currently Parser::parse is set to warn if someone feeds html to it (So something like Parser::parse( wfMessage('foo' )->parse(), ... ) gives a double escaping warning). this is great for checking wfMessage is right, but there are also lots of legit cases where you want to put html-like content into Parser::parse(), Html::rawElement being a prime example. So probably we should stop making Parser::parse warn about inserting html.

Change 456669 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/core@master] Use annotations for taint in Parser & ParserOutput.

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

Change 456670 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/tools/phan/SecurityCheckPlugin@master] Move builtin taints for Parser&ParserOutput into inline annotations

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

Change 456670 merged by jenkins-bot:
[mediawiki/tools/phan/SecurityCheckPlugin@master] Move builtin taints for Parser&ParserOutput into inline annotations

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

Change 456669 merged by jenkins-bot:
[mediawiki/core@master] Use annotations for taint in Parser & ParserOutput.

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

Change 456653 merged by jenkins-bot:
[mediawiki/extensions/Scribunto@master] Suppress phan-taint-check false positives in make-normalization-table.php

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-09-04 (1.32.0-wmf.20)).Aug 31 2018, 7:00 PM

Change 456782 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[integration/config@master] seccheck for Scribunto

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

Change 456782 merged by jenkins-bot:
[integration/config@master] seccheck for Scribunto

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

Legoktm closed this task as Resolved.Sep 1 2018, 5:11 AM

Legoktm assigned this task to Bawolff.

sbassett moved this task from Wikimedia deployed to Done on the phan-taint-check-plugin board.Oct 15 2019, 6:53 PM

Maintenance_bot removed a project: Patch-For-Review.Oct 15 2019, 7:14 PM

sbassett triaged this task as Medium priority.Oct 15 2019, 7:35 PM

sbassett raised the priority of this task from Medium to Needs Triage.