Skip to content
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.

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

Fix interaction between defaultAdditionalTaintStep and defaultImplicitTaintRead #18776

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

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Feb 13, 2025

defaultImplicitTaintRead is described as follows:

  /**
   * Holds if taint flow configurations should allow implicit reads of `c` at sinks
   * and inputs to additional taint steps.
   */

However where we implement the functionality of defaultImplicitTaintRead (in allowImplicitRead, see the first commit), it is only applied at sinks and additional taint steps defined in the configuration. Additional taint steps defined via defaultAdditionalTaintStep are overlooked.

In Rust, one such defaultAdditionalTaintStep is for concatenation with +, and one such defaultImplicitTaintRead is for reading from reference content. As a result we've been missing flow in common cases like this:

let _ = string1 + &string2;

This PR is a draft (for now) because I'm nervous of unintended consequences, or whether this interaction was left out on purpose (for performance reasons perhaps). The change is in shared code and may affect all languages.

TODO:

  • discuss
  • review / accept test changes
    • Rust changes are positive
    • Swift changes are positive
    • JS changes to library-tests/TaintedUrlSuffix are positive, the rest I don't understand (neutral???)
    • Go TODO
    • Ruby TODO
  • DCA runs
  • change note

@geoffw0 geoffw0 added Rust Pull requests that update Rust code DataFlow Library Ruby Go javascript Pull requests that update Javascript code Swift labels Feb 13, 2025
@@ -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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataFlow Library javascript Pull requests that update Javascript code JS Rust Pull requests that update Rust code Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants