Incomprehensible error message aside

Yeah, it's not great unfortunately. There's T351366 about improving this. The main problem is that some issues, like here, have very long traces, and it's hard to condense all the information on a single line. I would also like to improve how those lines are collected and sorted, but last time I tried it turned out to be pretty hard and I didn't have good ideas for implementing it. In turn, this (clearer error messages) is virtually blocking T274780, which is the single most impactful change I could make to taint-check. I've been wanting to do that for years, but hard-to-parse messages (and, in part, performance) have prevented it thus far.

(what is it even trying to list?)

For unsafe function calls, the first group of lines explains why the callee is unsafe; in this case, it's only due to the @param-taint annotations in update. The second group of lines traces why the argument to that method call is unsafe. It tries to list everything relevant like assignments and function calls, in kind-of reverse order. When properties are involved though, the ordering becomes quite unpredictable.

$this->conds is somehow tainted while in execute(), but there are no other error messages. The only assignments to $this->conds are in where(), and if the values being assigned are tainted, that should generate a SecurityCheck error on the where() call that passes them.

Taint-check does not distinguish between properties of different instances (phan itself only does it for properties of $this via special-casing), so any time the conds property is modified anywhere in the codebase, that'll merge in the taintedness from the local code. This is based on the assumption that, similar to union types, properties should generally always be used with the same degree of taintedness/escaping. The assumption is fairly safe (except for some very polymorphic properties like StatusValue::$value), and definitely true here.

The problem, once again, lies in error reporting: here, taint-check isn't able to extract the relevant lines for reporting this issue, so it just includes basically everything that ever happened to put a tainted value in the argument to where(). Note how I said "put a tainted value in the argument", as in: the argument as a whole might have been perfectly safe, for example if the tainted value was used in a string-keyed element. The whole sql_numkey thing is basically a special-case, so it's probably a specific bug with that that I hope might not be too hard to fix.

I tried to do some debugging on this, but I didn't get far. I learned two things though:

• Commenting-out this line in where() makes the error disappear: https://gerrit.wikimedia.org/g/mediawiki/core/+/277d4b025d8327fbfedabf09fa3e2ea8b096ffea/includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php#210 $this->conds[$key] = $cond;
• @phan-debug-var-taintedness in execute() gives: SecurityCheckDebugTaintedness Variable $conds has taintedness: {Own: NONE; Keys: YES; Elements: {UNKNOWN => {Own: YES; Elements: {cat_title => {Own: YES}; ...(20+ other keys here)... job_token => {Own: NONE}; UNKNOWN => {Own: YES}}}}} – clearly it is the Keys: YES that is the problem, and that part goes away when commenting out that aforementioned line. I haven't found a way to figure out where this taint comes from.

This makes sense, and the two findings are consistent in that the call is likely deemed unsafe due to the keys. You also identified that the hard part is figuring out where the taintedness comes from... I don't have a magic way of doing that. I just started removing where() calls from the source code if they were listed in the warning and ran phan, until I got the error message down to something more manageable that doesn't get truncated (notice the ellipsis at the end). Eventually I got it down to:

includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php:332 SecurityCheck-SQLInjection Calling method \Wikimedia\Rdbms\IDatabase::update() in \Wikimedia\Rdbms\UpdateQueryBuilder::execute that outputs using tainted argument #3 (`$this->conds`). (Caused by: annotations in \Wikimedia\Rdbms\IDatabase::update) (Caused by: includes/user/User.php +454; includes/user/User.php +448; annotations in \Wikimedia\Rdbms\SelectQueryBuilder::fetchRow; includes/user/User.php +1264; includes/user/User.php +1283; includes/user/User.php +1307; includes/user/User.php +454; includes/user/User.php +448; annotations in \Wikimedia\Rdbms\SelectQueryBuilder::fetchRow; includes/user/User.php +1264; includes/user/User.php +2398; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +201; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +203; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +210; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +214)

So your real culprit is User. Two things to note there:

• User::$mId is considered unsafe because it comes from the DB. Yes, it is actually an integer, but phan obviously doesn't see the database schema, and the code has no int cast; it would probably be a good idea to add it regardless
• Something is clearly wrong in how it handles makeUpdateConditions. I don't understand why, though.

I'll look into the redundant caused-by lines for the numkey special-case (trivially reproducible), and then see if I can get a minimal test case to reproduce the issue in User.

Change #1178129 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/tools/phan/SecurityCheckPlugin@master] Improve error reporting for numkey taint

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

With r1178129 applied (plus any other unreleased changes in taint-check master, but I don't think there's anything relevant), the updated message for the modified code is

includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php:332 SecurityCheck-SQLInjection Calling method \Wikimedia\Rdbms\IDatabase::update() in \Wikimedia\Rdbms\UpdateQueryBuilder::execute that outputs using tainted argument #3 (`$this->conds`). (Caused by: annotations in \Wikimedia\Rdbms\IDatabase::update) (Caused by: includes/user/User.php +454; includes/user/User.php +448; annotations in \Wikimedia\Rdbms\SelectQueryBuilder::fetchRow; includes/user/User.php +1264; includes/user/User.php +2398; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +201; includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php +210)

Reverting all my changes to MW core, thus going back to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1176470, the issue message becomes

includes/libs/rdbms/querybuilder/UpdateQueryBuilder.php:332 SecurityCheck-SQLInjection Calling method \Wikimedia\Rdbms\IDatabase::update() in \Wikimedia\Rdbms\UpdateQueryBuilder::execute that outputs using tainted argument #3 (`$this->conds`). (Caused by: annotations in \Wikimedia\Rdbms\IDatabase::update)

It's shorter because we're hitting the internal limit for caused-by limits (as before), so there's no space for the actual culprit lines. The difference is that when we filter those lines for emitting the issue, nothing relevant remains in the truncated list. I have tested increasing the max size from 100 to 800 lines, and the updated issue message became identical to the first one in this comment (for the modified core code).

Of course it would be possible to permanently increase the limit, but I'm a bit wary because we may get exponential growth causing performance issues. This happened in the past and it's why a limit exists. I'd be more comfortable doing that after the caused-by overhaul that I've already mentioned a million times in my last comment.

As for why the issue is reported in the first place: I'm not sure, it looks weird. Probably a stupid bug. One thing I quickly confirmed is that User.php#1264 (assignment involving dynamic properties) is unnecessary to reproduce the issue. So most likely a shape that isn't handled correctly somewhere. I might dig deeper tomorrow.

In T401748#11080915, @Daimona wrote:

As for why the issue is reported in the first place: I'm not sure, it looks weird. Probably a stupid bug. One thing I quickly confirmed is that User.php#1264 (assignment involving dynamic properties) is unnecessary to reproduce the issue. So most likely a shape that isn't handled correctly somewhere. I might dig deeper tomorrow.

I looked into this, and realized way too late what I should've checked first: it's hitting the maximum analysis depth. Basically, once we hit the limit, we stop analyzing functions further down the call stack, and the function for which analysis was aborted (in this example, User::makeUpdateConditions) gets marked as UNKNOWN_TAINT, which is later treated the same as if it were fully tainted. This thing has been around for a while, and it's caused a bunch of false positives over the years, some of which I've already spent a bunch of time investigating.

The fix for this is to increase the recursive analysis depth limit; we're still going to have to analyze all functions one way or another, so I believe it would mostly change the order in which this happens, as well as the distribution (i.e., more things getting analyzed at the start, so progress increasing slower, then getting faster and faster). There's already a patch for it: r1081465. I'm not sure why I marked it as WIP/blocked; maybe I wanted to do the caused-by improvements (once again!) first, or maybe r681443. But now I think we could just go ahead and increase the max depth, as long as it does not cause severe performance issues.

For completeness, the other possible approach is to avoid considering UNKNOWN as unsafe in functions. I think this was done in the spirit of favouring false positives, since it's security issues we're talking about. I think we can keep doing this, it shouldn't be too bad on its own. It only becomes problematic when paired with a low max depth.

In T401748#11083321, @Daimona wrote:

The fix for this is to increase the recursive analysis depth limit; [...] There's already a patch for it: r1081465.

I tested this on MW core. Memory usage increased from 2802MB to 2881MB; total time from 1m40s to 1m48s. It's noticeable , but not too bad. However, it also brings 5 new issues, some with really long caused-by lines; I haven't analyzed these but they might be false positives. Also worth noting that there are no unused suppressions.

Change #1188423 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] User: Simplify makeUpdateConditions()

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

Change #1188423 merged by jenkins-bot:

[mediawiki/core@master] User: Simplify makeUpdateConditions()

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

Change #1188715 had a related patch set uploaded (by Gergő Tisza; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.45.0-wmf.18] User: Simplify makeUpdateConditions()

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

Change #1188715 merged by jenkins-bot:

[mediawiki/core@wmf/1.45.0-wmf.18] User: Simplify makeUpdateConditions()

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

Mentioned in SAL (#wikimedia-operations) [2025-09-16T13:29:43Z] <tgr@deploy1003> Started scap sync-world: Backport for [[gerrit:1188715|User: Simplify makeUpdateConditions() (T401748)]], [[gerrit:1188716|session: Add a mechanism for forcing a refresh (T399200)]], [[gerrit:1188717|Use short expiry for JWT cookies (T399200)]], [[gerrit:1188718|tests: Update for SessionCookieJwtExpiration added in core (T399200 T404667)]], [[gerrit:1188765|xLab: Fix instrument to produce valid events

Mentioned in SAL (#wikimedia-operations) [2025-09-16T13:35:39Z] <tgr@deploy1003> hueitan, tgr: Backport for [[gerrit:1188715|User: Simplify makeUpdateConditions() (T401748)]], [[gerrit:1188716|session: Add a mechanism for forcing a refresh (T399200)]], [[gerrit:1188717|Use short expiry for JWT cookies (T399200)]], [[gerrit:1188718|tests: Update for SessionCookieJwtExpiration added in core (T399200 T404667)]], [[gerrit:1188765|xLab: Fix instrument to produce valid events (T404420)]]

Mentioned in SAL (#wikimedia-operations) [2025-09-16T13:48:33Z] <tgr@deploy1003> Finished scap sync-world: Backport for [[gerrit:1188715|User: Simplify makeUpdateConditions() (T401748)]], [[gerrit:1188716|session: Add a mechanism for forcing a refresh (T399200)]], [[gerrit:1188717|Use short expiry for JWT cookies (T399200)]], [[gerrit:1188718|tests: Update for SessionCookieJwtExpiration added in core (T399200 T404667)]], [[gerrit:1188765|xLab: Fix instrument to produce valid events

@Daimona Thank you for the explanations.

I wrote that workaround patch, because something in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1188321/comments/15ff156e_e6ed0623 caused this failure in UpdateQueryBuilder::execute to disappear, and I didn't want us to be randomly adding and removing the suppression every couple of patches.

The workaround is to just simplify makeUpdateConditions() (thanks for pointing out that function), which is apparently enough for it to be more completely analyzed, and avoid hitting the maximum analysis depth, or something like that? I am not sure if I understand the mechanism here exactly. I hope this works for a while, until someone writes more complex code somewhere else.

I think this can be closed in favor of T351366: phan-taint-check-plugin needs clearer error messages. I commented there with some ideas on how to make the output better.

Change #1178129 merged by jenkins-bot:

[mediawiki/tools/phan/SecurityCheckPlugin@master] Improve error reporting for numkey taint

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