Page MenuHomePhabricator
Create Task
Maniphest T211471

analyse and fix suppressed taint-check issue for Renameuser
Closed, ResolvedPublic
Actions

Description

https://integration.wikimedia.org/ci/job/mwext-php70-phan-seccheck-docker/29152/console on version 1.5.0

<?xml version="1.0" encoding="ISO-8859-15"?>
<checkstyle version="6.5">
  <file name="./includes/SpecialRenameuser.php">
    <error line="355" severity="warning" message="Calling method \Title::moveTo() in \SpecialRenameuser::execute that outputs using tainted argument $[arg #3]. (Caused by: ../../includes/Title.php +4069) (Caused by: Builtin-\Message::text)" source="SecurityCheck-XSS"/>
  </file>
</checkstyle>

Details

SubjectRepoBranchLines +/-
[Renameuser] Make flaky seccheck non-votingintegration/configmaster+1 -1
Use wfEscapeWikiText for user names in log messagemediawiki/extensions/Renameusermaster+6 -3
Customize query in gerrit

Related Objects

Event Timeline

Umherirrender created this task.Dec 8 2018, 1:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 8 2018, 1:44 PM
Umherirrender added a comment.Dec 8 2018, 1:45 PM

Possible flaky, because there are some passes on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Renameuser/+/461442/

Umherirrender merged a task: T212876: phan-taint-check-plugin failing for Renameuser extension.Feb 1 2019, 6:08 PM
Umherirrender added a subscriber: Paladox.
xSavitar awarded a token.Feb 1 2019, 6:14 PM
xSavitar subscribed.

Occurred here too: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Renameuser/+/487134.

gerritbot subscribed.Feb 8 2019, 8:13 PM

Change 489307 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/Renameuser@master] Rename variable to avoid clutter in taint-check-plugin

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

gerritbot added a project: Patch-For-Review.Feb 8 2019, 8:13 PM
gerritbot added a comment.Feb 23 2019, 5:44 PM

Change 492488 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/extensions/Renameuser@master] Migrate from Title::moveTo to MovePage::move

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

Umherirrender merged a task: T217067: Calling method \Title::moveTo() in \SpecialRenameuser::execute that outputs using tainted argument $reason. File: SpecialRenameuser.php L 359..Feb 25 2019, 8:08 PM
Umherirrender added a subscriber: Tgr.
Umherirrender added a comment.Feb 25 2019, 8:10 PM

I have uploaded some patch sets but I am not sure if there are combined can fix the problem, because it seems flaky.

I cannot run it locally due to (old) requirements.

Please have a look to get this passed.

  • Job non-voting?
  • Moving to function and add @suppress?
  • Find the error in extension or plugin
  • Go back to older version
  • Sitting around ...
Tgr added a comment.Feb 25 2019, 9:09 PM

It would be nice to document the steps to run the test locally. I have tried but the results were... strange.

gerritbot added a comment.Mar 1 2019, 5:01 PM

Change 493719 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[integration/config@master] [Renameuser] Make flaky seccheck non-voting

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

gerritbot added a comment.Mar 1 2019, 5:01 PM

Change 489307 abandoned by Umherirrender:
Use wfEscapeWikiText for user names in log message

Reason:
This was a proof of concept for the seccheck failure

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

gerritbot added a comment.Mar 1 2019, 5:36 PM

Change 493719 abandoned by Umherirrender:
[Renameuser] Make flaky seccheck non-voting

Reason:
Suppressed in I13fb505ea5ece15c007e6e857481996a11c1aad4

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

Umherirrender renamed this task from taint-checks for Renameuser failing to analyse and fix suppressed taint-check issue for Renameuser.Mar 1 2019, 8:44 PM
Umherirrender removed a project: Patch-For-Review.
Umherirrender removed a subscriber: gerritbot.
Daimona subscribed.May 6 2019, 12:15 PM

I just checked and this will be fixed (without the need to suppress the issue) with taint-check 2.0 (still in dev).

Reedy subscribed.Jun 6 2019, 2:16 PM
15:08:59   <file name="./includes/RenameUserJob.php">
15:08:59     <error line="143" severity="error" message="Calling method \Wikimedia\Rdbms\Database::selectFieldValues() in \RenameUserJob::run that outputs using tainted argument $conds. (Caused by: Builtin-\Wikimedia\Rdbms\Database::selectFieldValues) (Caused by: ./includes/RenameUserJob.php +123; ./includes/RenameUserJob.php +78; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +50; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +143)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="143" severity="error" message="Calling method \Wikimedia\Rdbms\Database::selectFieldValues() in \RenameUserJob::run that outputs using tainted argument $table. (Caused by: Builtin-\Wikimedia\Rdbms\Database::selectFieldValues) (Caused by: ./includes/RenameUserJob.php +49; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +143)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="149" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $[arg #2]. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +79; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149; ./includes/RenameUserJob.php +50; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="149" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +145; ./includes/RenameUserJob.php +78; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149; ./includes/RenameUserJob.php +50; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="149" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $table. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +49; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +143; ./includes/RenameUserJob.php +149)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="158" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $[arg #2]. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +79; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149; ./includes/RenameUserJob.php +158; ./includes/RenameUserJob.php +50; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +149; ./includes/RenameUserJob.php +158)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="158" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $conds. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +123; ./includes/RenameUserJob.php +78; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +50; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +143; ./includes/RenameUserJob.php +158)" source="SecurityCheck-SQLInjection"/>
15:08:59     <error line="158" severity="error" message="Calling method \Wikimedia\Rdbms\Database::update() in \RenameUserJob::run that outputs using tainted argument $table. (Caused by: Builtin-\Wikimedia\Rdbms\Database::update) (Caused by: ./includes/RenameUserJob.php +49; ../../includes/jobqueue/Job.php +145; ./includes/RenameUserJob.php +143; ./includes/RenameUserJob.php +149; ./includes/RenameUserJob.php +158)" source="SecurityCheck-SQLInjection"/>
15:08:59   </file>
Daimona closed this task as Resolved.Jul 12 2019, 11:10 AM
Daimona claimed this task.

Fixed with seccheck 2.0.1, see e.g. https://integration.wikimedia.org/ci/job/mwext-php72-phan-seccheck-docker/68/console.

sbassett moved this task from Backlog to Done on the phan-taint-check-plugin board.Oct 15 2019, 7:01 PM
sbassett triaged this task as Medium priority.Oct 15 2019, 7:20 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits