| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Currently missing: tracing back from the receiver of a call to strings.Replacer.Replace to find the original call to strings.NewReplacer to determine what is actually being replaced, and ensure it is \r and \n. The downside is that sometimes we wouldn't be able to find the call to strings.NewReplacer, and the question is whether we'd want to consider it a sanitizer or not in that case. |
Sorry, something went wrong.
|
I've also add the extra sanitizers to the other place where string replacement is a sanitizer. (Note: this does include the "tracing back" to find out what kind of quote is being replaced.) It might be worth combining the two very similar implementations. |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks sensible, couple of small things to fix
Sorry, something went wrong.
| StringsNewReplacerCall() { this.getTarget().hasQualifiedName("strings", "NewReplacer") } | ||
|
|
||
| DataFlow::Node getAReplacedArgument() { | ||
| exists(int m, int n | m = 2 * n and n = m / 2 and result = getArgument(m)) |
There was a problem hiding this comment.
| exists(int m, int n | m = 2 * n and n = m / 2 and result = getArgument(m)) | |
| exists(int m | m % 2 = 0 and result = getArgument(m)) |
Sorry, something went wrong.
| exists(StringsNewReplacerConfiguration config, DataFlow::Node source, DataFlow::Node sink | | ||
| config.hasFlow(source, sink) and | ||
| this.getTarget().hasQualifiedName("strings", "Replacer", "Replace") and | ||
| sink = this.getReceiver() and |
There was a problem hiding this comment.
Use exists( DataFlow::MethodCallNode mcn | ... this = mcn.getResult()) rather than assume that a method-call node is its own result
Sorry, something went wrong.
|
@smowton Currently, for log injection this PR doesn't check that the receiver of a call to strings.Replacer.Replace was constructed with \r and/or \n as strings that will be replaced. It would be easy enough to do, as evidenced by the fact that it is done for string break, but I wasn't sure about the pros and cons. If we add it in then we will correctly alert (TP) in cases where something else happens to be replaced, but incorrectly alert (FP) in cases where the right things are being replaced but in a complicated enough way that we couldn't track it. My guess was that the first situation would be very rare, so it was better to not check what is being replaced. Do you agree? If so, feel free to merge. |
Sorry, something went wrong.
|
Given we're doing it for StringBreak already and the other sanitisers in LogInjectionCustomizations do check what's being replaced, yes I think we should do likewise and try to check the replace args. If it proves too expensive to measure cross-method we can do it locally. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
strings.Replacer.Replace and strings.Replacer.WriteString