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

tainting: Add new rule options #6327

Merged
merged 4 commits into from Oct 18, 2022
Merged

tainting: Add new rule options #6327

merged 4 commits into from Oct 18, 2022

Conversation

IagoAbal
Copy link
Collaborator

@IagoAbal IagoAbal commented Oct 17, 2022

  1. taint_assume_safe_functions to assume that function calls do not propagate taint from their arguments to their result.
  2. taint_assume_safe_indexes to assume that a tainted index does not cause an access expression to be tainted.

Both should help reducing FPs. Also, taint_assume_safe_functions is meant to replace the not_conflicting option in pattern-sanitizers, but is not enough by itself. This e.g. sink(not_tainted(tainted('a'))) will still be flagged due to how sinks work right now (anything within the range of the sink is considered a sink, also tainted('a')). We may also need to generalize pattern-propagators so you can enumerate the functions that propagate taint.

Closes PA-1541

test plan:
make test # added two tests

PR checklist:

  • Purpose of the code is evident to future readers
  • Tests included or PR comment includes a reproducible test plan
  • Documentation is up-to-date
  • A changelog entry was added to changelog.d for any user-facing change
  • Change has no security implications (otherwise, ping security team)

If you're unsure on any of this, please see:

@IagoAbal IagoAbal requested a review from a team October 17, 2022 12:00
@linear
Copy link

linear bot commented Oct 17, 2022

PA-1541 Add options to assume taint-safe arrays and functions

Current solution for sanitizing "all third functions" is based on the use of "not-conflicting sanitizers" . But these not-conflicting sanitizers may have been a bad choice.

Right now there is an obvious single point in the code where we assume that an unknown third-function propagates taint, and that a tainted index makes an array access tainted. We should just add rule options: to control this behavior. Perhaps we should even have these options set to true by default to reduce FPs?

E.g.

id: test
mode: taint
options:
  taint_safe_arrays: true
  taint_safe_functions: true
...

1. `taint_assume_safe_functions` to assume that function calls do not
   propagate taint from their arguments to their result.
2. `taint_assume_safe_indexes` to assume that a tainted index does not
   cause an access expression to be tainted.

Both should help reducing FPs. Also, `taint_assume_safe_functions` is
meant to replace the `not_conflicting` option in `pattern-sanitizers`,
but is not enough by itself. This e.g. `sink(not_tainted(tainted('a')))`
will still be flagged due to how sinks work right now (anything within
the range of the sink is considered a sink, also `tainted('a')`). We
may also need to generalize `pattern-propagators` so you can enumerate
the functions that propagate taint.

Closes PA-1541

test plan:
make test # added two tests
@ievans
Copy link
Member

ievans commented Oct 17, 2022

@IagoAbal this looks great, are these intended to be stable / relied on by external users or should we market them as experimental / internal?

@IagoAbal
Copy link
Collaborator Author

IagoAbal commented Oct 18, 2022

@IagoAbal this looks great, are these intended to be stable / relied on by external users or should we market them as experimental / internal?

taint_assume_safe_indexes could be considered stable and can be relied on, the only question is whether to change the default to true instead of false. The current default value (false) is more appropriate for audit rules.

taint_assume_safe_functions is meant to be stable, but we could not enable it by default right now due to the problems described in the PR comment, so for now I consider this one experimental.

I'll clarify this in the changelog.

@IagoAbal IagoAbal merged commit 3d4b1af into develop Oct 18, 2022
@IagoAbal IagoAbal deleted the iago/pa-1541 branch October 18, 2022 12:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants