Page MenuHomePhabricator

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

Description

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

<?xml version="1.0" encoding="ISO-8859-15"?>
<checkstyle version="6.5">
  <file name="./includes/Banner.php">
    <error line="1461" severity="error" message="Calling method \Wikimedia\Rdbms\IDatabase::insert() in \Banner::logBannerChange that outputs using tainted argument $log. (Caused by: ./includes/Banner.php +1443; ./includes/Banner.php +1458; ./includes/Banner.php +1453)" source="SecurityCheck-SQLInjection"/>
  </file>
  <file name="./maintenance/CleanCNTranslateMetadata.php">
    <error line="63" severity="error" message="Calling method \Wikimedia\Rdbms\IDatabase::delete() in \CleanCNTranslateMetadata::cleanDuplicates that outputs using tainted argument $[arg #2]." source="SecurityCheck-SQLInjection"/>
  </file>
  <file name="./special/SpecialBannerLoader.php">
    <error line="49" severity="warning" message="Echoing expression that was not html escaped (Caused by: ./special/SpecialBannerLoader.php +32)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./special/SpecialCentralNoticeBanners.php">
    <error line="682" severity="warning" message="HTMLForm info field (non-raw) escapes default key already (Caused by: Builtin-\Html::rawElement; Builtin-\Html::rawElement; Builtin-\Html::rawElement)" source="SecurityCheck-DoubleEscaped"/>
    <error line="697" severity="warning" message="HTMLForm info field (non-raw) escapes default key already" source="SecurityCheck-DoubleEscaped"/>
    <error line="699" severity="warning" message="Calling method \wfEscapeWikiText() in \SpecialCentralNoticeBanners::generateBannerEditForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/GlobalFunctions.php +1639)" source="SecurityCheck-DoubleEscaped"/>
    <error line="701" severity="warning" message="Calling method \wfEscapeWikiText() in \SpecialCentralNoticeBanners::generateBannerEditForm that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/GlobalFunctions.php +1639)" source="SecurityCheck-DoubleEscaped"/>
    <error line="712" severity="warning" message="HTMLForm info field (non-raw) escapes default key already (Caused by: Builtin-\Html::rawElement; Builtin-\Html::rawElement; Builtin-\Html::rawElement)" source="SecurityCheck-DoubleEscaped"/>
  </file>
</checkstyle>

The issues in SpecialCentralNoticeBanners.php are T201902

The insert looks safe on first look
The delete may needs an (int) for $row->maxrev

SpecialBannerLoader.php is using OutputPage::disable. I have no idea if the echo after that is safe or not

Event Timeline

Change 460195 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/extensions/CentralNotice@master] Make phan-taint-check pass on this extension

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

Change 460195 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Build: Make phan-taint-check 1.5.0 pass on this extension

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

Legoktm assigned this task to Bawolff.
sbassett triaged this task as Medium priority.Oct 15 2019, 7:34 PM