Page MenuHomePhabricator

Implement NUMKEY propagation the proper way
Open, Needs TriagePublic

Description

This should happen together with other taint types. We should simply pay attention if the incoming taint is numkey, to see which keys it can be propagated to. Might also want to ensure that the logic for setting NUMKEY on keys in Taintedness is up-to-date, as we're going to use that for unknown keys. Future (?) TODO might be to have a separated property for int vs non-int unknown keys. Aside from the improved accuracy, there's also a bunch of legacy code that we can remove once this is done:

  • Temporary parameter to markAllDependentMethodsExecForNode, conditional in backpropagateArgTaint; possibly merge the two
  • getObjsForNodeForNumkeyBackprop
  • SECCHECK_NUMKEY_SPERIMENTAL env variable
  • separate category of "numkey" tests (can be just merged back into the normal integration category)

Event Timeline

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

[mediawiki/tools/phan/SecurityCheckPlugin@master] [WIP] Backpropagate numkey taint more precisely

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

The patch above is an important step towards resolving this task. NUMKEY is still special-cased, but now the two code paths are almost identical. I don't think it's possible to do better given the current state of things. What would really help is

have a separated property for int vs non-int unknown keys

at which point we shouldn't even need to actually set NUMKEY_TAINT.