Add missing string replacement sanitizers to log-injection and string-break#11910
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owen-mc
force-pushed
the
go/log-injection-sanitizer-newreplacer-replace
branch
from
6282385 to
9d2d4f4
Compare
|
Performance evaluation showed no changes |
| class StringsNewReplacerCall extends DataFlow::CallNode { | ||
| StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") } | ||
|
|
||
| /** | ||
| * Gets an argument to this call corresponding to a string that will be | ||
| * replaced. | ||
| */ | ||
| DataFlow::Node getAReplacedArgument() { | ||
| exists(int n | n % 2 = 0 and result = this.getArgument(n)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A configuration for tracking flow from a call to `strings.NewReplacer` to | ||
| * the receiver of a call to `strings.Replacer.Replace` or | ||
| * `strings.Replacer.WriteString`. | ||
| */ | ||
| class StringsNewReplacerConfiguration extends DataFlow2::Configuration { | ||
| StringsNewReplacerConfiguration() { this = "StringsNewReplacerConfiguration" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof StringsNewReplacerCall } | ||
|
|
||
| override predicate isSink(DataFlow::Node sink) { | ||
| exists(DataFlow::MethodCallNode call | | ||
| sink = call.getReceiver() and | ||
| call.getTarget().hasQualifiedName("strings", "Replacer", ["Replace", "WriteString"]) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This part seems to be identical between LogInjectionCustomizations and StringBreakCustomizations. Would it make sense / be possible to de-duplicate this in a shared location?
There was a problem hiding this comment.
Good idea. It feels like something that should be usable in many different places. I've moved it to StringOps.qll, which should hopefully make that simple. I've started a performance evaluation as this is a fairly major change.
There was a problem hiding this comment.
The performance analysis was neutral.
owen-mc
force-pushed
the
go/log-injection-sanitizer-newreplacer-replace
branch
from
9d2d4f4 to
3fda9f6
Compare
|
I've updated this to use its own copy of the dataflow library. The failing QLDoc test is because of an undocumented predicate in the dataflow library, which obviously I'm not going to fix in this PR, so I believe it should be ignored. |
smowton
left a comment
smowton
left a comment
There was a problem hiding this comment.
Made an import of an internal library private; otherwise LGTM
3 participants
Add
strings.Replacer.Replaceandstrings.Replacer.WriteStringas sanitizers for log-injection and string-break.This is a resurrection of github/codeql-go#731.