Page MenuHomePhabricator

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

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

Related Gerrit Patches:

Event Timeline

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

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

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.

Anomie added a subscriber: Anomie.Aug 31 2018, 4:58 PM

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

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 triaged this task as Medium priority.Oct 15 2019, 7:35 PM
sbassett raised the priority of this task from Medium to Needs Triage.
sbassett triaged this task as Medium priority.