Open owen-mc opened 2 years ago
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.
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.
@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.
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.
strings.Replacer.Replace
andstrings.Replacer.WriteString