Python: Fix attribute taint #7873
Conversation
In `x.arg = TAINTED_STRING` there is a store step to the attribute `arg` of `x`. In our taint modeling, we allow _any_ store step with the code below. This means that we also say there is a taint-step directly from `TAINTED_STRING` to `x` :| ```codeql // construction by literal // TODO: Not limiting the content argument here feels like a BIG hack, but we currently get nothing for free :| DataFlowPrivate::storeStep(nodeFrom, _, nodeTo) ```
|
Judging from the results from the internal experiment, I think this looks good 👍 |
There was a problem hiding this comment.
Basically looks fine. Is this removed alert an FP in this context?
| @@ -49,7 +49,7 @@ def __exit__(self, exc_type, exc, tb): | |||
| def test_with_arg(): | |||
| ctx = Context_arg(TAINTED_STRING) | |||
| with ctx as tainted: | |||
| ensure_tainted(tainted) # $ tainted | |||
| ensure_tainted(tainted) # $ MISSING: tainted | |||
There was a problem hiding this comment.
This is actually expected, I think, until we model __enter__.
yes, that's a FP. The current path is: 1: ControlFlowNode for cert_file
2: [post store] ControlFlowNode for self
3: ControlFlowNode for Attribute()
4: ControlFlowNode for Fstring
This PR means that we no longer treat the |
1 similar comment
yes, that's a FP. The current path is: 1: ControlFlowNode for cert_file
2: [post store] ControlFlowNode for self
3: ControlFlowNode for Attribute()
4: ControlFlowNode for Fstring
This PR means that we no longer treat the |
|
Cool, thanks for showing the path. |
Fixes #7786
Draft PR so we can gauge what the impact of this change is before merging it (and look for failing QL tests 😅)