python: Rewrite path injection query to use flow state #7687
Conversation
This removes the FP cause by chaining This PR also removes `ChainedConfigs12.qll`, as we hope to solve future problems via flow states.
This allows a bit more precision. Specifically, we could require the sanitizer to only affect `ConvertedToDict`. In practice, most sanitizers woudl probably fail on raw input also, though.
since it was clearly already used. Add deprecation message instead.
also update test expectation
RasmusWL
left a comment
RasmusWL
left a comment
There was a problem hiding this comment.
Very cool that you have porting these queries to use flow state 💪 (also great to see that FP removed)
Is there an official doc for how to use flow state? (so I know if you made up the way to handle things, or if there is an established way already)
For python/ql/lib/semmle/python/security/dataflow/PathInjection.qll, I'm wondering how much deprecation we need to support. At least if someone has code that uses the old configurations (or old predicate), their code is going to stop working if this PR is merged as it is right now.
I also have a minor comment for the chained-config qll file, where I think we can mark even more things as deprecated.
python/ql/lib/semmle/python/security/dataflow/ChainedConfigs12.qll
Outdated
Show resolved
Hide resolved
python/ql/src/experimental/semmle/python/security/injection/NoSQLInjection.qll
Show resolved
Hide resolved
I am not aware of any official doc, so this is all made up, but it seemed pretty intuitive :-) |
|
Good point about the old configuration being public. We may have to leave it deprecated instead of deleting it. |
I am slightly concerned that the test now generates many more intermediate results. I suppose that maes the analysis heavy. Should the new library get a new name instead, so the old code does not get evaluated?
RasmusWL
left a comment
RasmusWL
left a comment
There was a problem hiding this comment.
👍 with the deprecations.
I'm still a bit surprised that the flow-state specific sanitizer is isBarrier/2, but 🤷
Out of interest, did you come up with the scheme of blocking flow for the old state when there is a state transition that occurs? (as with the barrier for NoSQL injection) I guess it helps with performance on the data-flow library, so I can see the benefit for sure 👍
Yes. I guess we should get the conversation going about how to actually use flow states effectively, but in many cases it has to be blocked or it will reach the sink. |
I think it is because it is implemented for the data flow configurations, and no taint-specific veneer has been added to taint configurations... |
2 participants
This removes the FP caused by chaining.
This PR also removes
ChainedConfigs12.qll, as we hope to solve future problems via flow states.I have marked it as not needing a change note, but let me know if you think it does :-)