RB: don't flag code-injection for dynamic loading where an attacker only controls a substring #10883
Conversation
… a substring in code-injection
…to refactor the string-concatenation-sanitizer
|
I haven't given this a very thorough review, sorry. I agree that the DCA results all look like FPs. I have a bikeshed suggestion for the predicate name. I can't really comment on the flow state stuff because I've also not used it. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| sourceNode = source.getNode() | ||
| sourceNode = source.getNode() and | ||
| // removing duplications of the same path, but different flow-labels. | ||
| sink = | ||
| min(DataFlow::PathNode otherSink | | ||
| config.hasFlowPath(any(DataFlow::PathNode s | s.getNode() = source.getNode()), otherSink) | ||
| | | ||
| otherSink order by otherSink.getState() | ||
| ) |
There was a problem hiding this comment.
Seems like this should be fixed in the shared libraries, the same way we did in JS. I've opened an internal issue for it.
| /** Gets a flow state for which this is a sink. */ | ||
| override DataFlow::FlowState getAFlowState() { | ||
| if c.runsArbitraryCode() | ||
| then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, then it's always vulnerable. |
There was a problem hiding this comment.
| then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, then it's always vulnerable. | |
| then result = [FlowState::substring(), FlowState::full()] // If it runs arbitrary code then it's always vulnerable. |
| */ | ||
| class StringConcatenationSanitizer extends Sanitizer { | ||
| StringConcatenationSanitizer() { | ||
| // string concatenations sanitize the `full` state, as an attacker no longer controls the entire string |
There was a problem hiding this comment.
This seems overly restrictive. User input flowing into eval is still bad even if it is concatenated with some constant string.
params[:foo] = "f; end; system('bad stuff'); def g"
eval("def " + params[:foo] + "; end")
# => def f; end; system('bad stuff'); def g; endThere was a problem hiding this comment.
It only restricts flow for the full state, which means that flow are only blocked for the sinks where runsArbitraryCode() does not hold.
That specifically means that your example is still flagged, because flow is not blocked for flow that is heading towards the eval() sink.
But I found an unrelated issue while adding a test that confirmed that it works: #10968
There was a problem hiding this comment.
Interesting. I tested this myself to check that it didn't work correctly, and I guess was also affected by that issue.
I wasn't sure what to name the new predicate on
CodeExecution. So I wrote the QLDoc and let Copilot pick a name.It's my first use of
DataFlow::FlowState, but I think I figured it out.Evaluation looks good.
Neutral performance, and removal of a few results that I think look like FPs.