Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was 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
owen-mc wants to merge 5 commits into github:main
base: main
Choose a base branch
Loading
from owen-mc:log-injection-sanitizer-newreplacer-replace

Conversation

@owen-mc
Copy link
Contributor

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

strings.Replacer.Replace and strings.Replacer.WriteString

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.

antoineco reacted with eyes emoji

Copy link
Contributor Author

owen-mc commented May 4, 2022
edited
Loading

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

@smowton smowton May 4, 2022

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,intn| m = 2 * n and n =m/2 and result = getArgument(m))
exists(int m| m % 2 =0 and result = getArgument(m))

owen-mc reacted with thumbs up emoji
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

@smowton smowton May 4, 2022

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 reacted with thumbs up emoji
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.

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.

Reviewers

1 more reviewer

@smowton smowton smowton approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /