Page MenuHomePhabricator

Raw HTML message in ProofreadPage
Closed, ResolvedPublic

Description

			$indexlink = Linker::link( $nt, $out->msg( 'proofreadpage_source' )->text(),
						[ 'title' => $out->msg( 'proofreadpage_source_message' )->text() ] );

The deprecated Linker does not escape the text - if you switch to LinkRenderer then it will do it for you (or use ->escaped() ).

phan-taint-check-plugin is complaining about two potential SQLi's but my quick look made it seem like they were false positives. (A more thorough look would be appreciated)

<checkstyle version="6.5">
  <file name="./ProofreadPage.body.php">
    <error line="523" severity="warning" message="Calling method \Linker::link() in \ProofreadPage::prepareArticle that outputs using tainted argument $[arg #2]. (Caused by: ../../includes/Linker.php +113)" source="SecurityCheck-XSS"/>
  </file>
  <file name="./includes/Page/ProofreadPageDbConnector.php">
    <error line="37" severity="error" message="Calling method \Wikimedia\Rdbms\Database::select() in \ProofreadPage\Page\ProofreadPageDbConnector::queryCount that outputs using tainted argument $[arg #1]. (Caused by: ../../includes/libs/rdbms/database/Database.php +1638) (Caused by: ./includes/Page/ProofreadPageDbConnector.php +35; ./includes/Page/ProofreadPageDbConnector.php +37)" source="SecurityCheck-SQLInjection"/>
    <error line="37" severity="error" message="Calling method \Wikimedia\Rdbms\Database::select() in \ProofreadPage\Page\ProofreadPageDbConnector::queryCount that outputs using tainted argument $[arg #2]. (Caused by: ../../includes/libs/rdbms/database/Database.php +1638) (Caused by: ./includes/Page/ProofreadPageDbConnector.php +35; ./includes/Page/ProofreadPageDbConnector.php +37)" source="SecurityCheck-SQLInjection"/>
  </file>
</checkstyle>

Event Timeline

I also checked about the potential SQLi and I don't see how it could happen. All "conds" arguments are key-values where the key is a string so should be properly escaped. More values seems to always be an integer or an array of DBKey obtained by using the Title::getDBKey method. The error are maybe raised because the query is constructed in a different class than the one where the execution is called.

I have just made a change that uses LinkRender instead of Linker and, so, should fix the possible XSS injection https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ProofreadPage/+/450398/

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 20 2018, 1:31 AM
sbassett triaged this task as Medium priority.Oct 15 2019, 7:42 PM