Skip to content

Fix interaction between defaultAdditionalTaintStep and defaultImplicitTaintRead #18776

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
geoffw0 wants to merge 5 commits into from
Closed

Fix interaction between defaultAdditionalTaintStep and defaultImplicitTaintRead #18776

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
974e75b
Fix interaction between defaultAdditionalTaintStep and defaultImplici…
geoffw0 Feb 13, 2025
c1a4cb2
Rust: Effect on tests.
geoffw0 Feb 13, 2025
dd2ca0d
Swift: Effect on tests.
geoffw0 Feb 17, 2025
1d1cf0e
JS: Effect on tests.
geoffw0 Feb 17, 2025
aaa9c8d
JS: More effects on tests.
geoffw0 Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion shared/dataflow/codeql/dataflow/TaintTracking.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ module TaintFlowMake<
Config::isSink(node) or
Config::isSink(node, _) or
Config::isAdditionalFlowStep(node, _, _) or
Config::isAdditionalFlowStep(node, _, _, _, _)
Config::isAdditionalFlowStep(node, _, _, _, _) or
defaultAdditionalTaintStep(node, _, _)
Copy link
Contributor Author

@geoffw0 geoffw0 Feb 17, 2025

Choose a reason for hiding this comment

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

^ for the benefit of reviewers, this is the actual change, everything else is consequences.

Copy link
Contributor Author

@geoffw0 geoffw0 Feb 18, 2025

Choose a reason for hiding this comment

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

@aschackmull does this looks like a reasonable change to you?

Copy link
Contributor

@aschackmull aschackmull Feb 20, 2025

Choose a reason for hiding this comment

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

It was left out on purpose to avoid lazy/imprecise models. Models from e.g. MaD are expected to handle content precisely without the need for implicit reads. I'm afraid that adding this could yield unintended FP flow in all sorts of places, but I don't know for sure. It's certainly risky, and I wouldn't expect it to be necessary.

Could you elaborate on the motivating example?

let _ = string1 + &string2;

Where's the missing read step - is it from string2 to &string2 or something like that? (please excuse my ignorance of Rust semantics). If so, it would seem that there would be plenty of room to add a proper read step.

Copy link
Contributor Author

@geoffw0 geoffw0 Feb 27, 2025

Choose a reason for hiding this comment

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

No, we have flow from string2 to &string2 and it adds ReferenceContent. The problem is that our model for + (in defaultAdditionalTaintStep) expects there to be no content. The compiler I believe adds an implicit dereference but there's nothing in our AST representing that.

Thus, I think we want to be able to read out of a ReferenceContent just about anywhere. or we need to figure out where the implicit dereferences occur – but I suspect we don't have enough information about types in the AST for that yet.

Copy link
Contributor

@aschackmull aschackmull Feb 28, 2025

Choose a reason for hiding this comment

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

we need to figure out where the implicit dereferences occur – but I suspect we don't have enough information about types in the AST for that yet.

If the key word in that sentence is "yet", then this sounds like the right approach.

Copy link
Contributor Author

@geoffw0 geoffw0 Feb 28, 2025

Choose a reason for hiding this comment

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

@hvitved do we have plans around implicit dereferences (I think that's what's going on here)? Can I write up an issue if we don't already have one?

(we can see from the tests that it's not just string arguments to + that get implicitly dereferenced, though that's a common situation)

Copy link
Contributor

@paldepind paldepind Feb 28, 2025

Choose a reason for hiding this comment

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

I think the problem is that what we currently do for + is not precise. The + operator is sugar for invoking add on the Add trait. So what happens for + depends on the specific implementation of that trait. For String the add implementation takes a &str as its parameter. So it's not an implicit dereference, but that the function actually taking a reference as it's argument.

Once we can handle calls to trait methods, we should represent + as a trait call, and then we should be able to write a model for String::add.

) and
defaultImplicitTaintRead(node, c)
}
Expand Down