Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

Add missing string replacement sanitizers to log-injection and string-break #731

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented May 4, 2022

strings.Replacer.Replace and strings.Replacer.WriteString

@owen-mc
Copy link
Contributor Author

owen-mc commented May 4, 2022

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.

@owen-mc
Copy link
Contributor Author

owen-mc commented May 4, 2022

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.

@owen-mc owen-mc changed the title Add missing log-injection sanitizers Add missing string replacement sanitizers to log-injection and string-break May 4, 2022
Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible, couple of small things to fix

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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))

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use exists( DataFlow::MethodCallNode mcn | ... this = mcn.getResult()) rather than assume that a method-call node is its own result

@owen-mc
Copy link
Contributor Author

owen-mc commented May 5, 2022

@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.

@smowton
Copy link
Contributor

smowton commented May 7, 2022

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants