-
Notifications
You must be signed in to change notification settings - Fork 568
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
tainting: Add new rule options (#6327)
* tainting: Add new rule options 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 * Fix -dfg_tainting to use rule options and clean up * Isaac's review * Fix typo in test example
- Loading branch information
Showing
17 changed files
with
136 additions
and
28 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
taint-mode: Added two new rule `options` that help minimizing false positives. | ||
|
||
First one is `taint_assume_safe_indexes`, which makes Semgrep assume that an | ||
array-access expression is safe even if the index expression is tainted. Otherwise | ||
Semgrep assumes that e.g. `a[i]` is tainted if `i` is tainted, even if `a` is not. | ||
Enabling this option is recommended for high-signal rules, whereas disabling it | ||
may be preferred for audit rules. Currently, it is disabled by default for pure | ||
backwards compatibility reasons, but this may change in the near future after some | ||
evaluation. | ||
|
||
The other one is `taint_assume_safe_functions`, which makes Semgrep assume that | ||
function calls do *NOT* propagate taint from their arguments to their output. | ||
Otherwise, Semgrep always assumes that functions may propagate taint. This is | ||
intended to replace _not conflicting_ sanitizers (added in v0.69.0) in the future. | ||
This option is still experimental and needs to be complemented by other changes | ||
to be made in future releases. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<?php | ||
|
||
function not_tainted($data) { | ||
// do stuff | ||
return '2'; | ||
} | ||
|
||
//Problem here is that `tainted('a')` is in itself considered a sink because | ||
//currently taint-mode considers anything that falls within a sink-range to be | ||
//a sink... and the sink range here is whatever it matches `sink(...)`! | ||
//todook:tainted | ||
sink(not_tainted(tainted('a'))); | ||
|
||
$ok = not_tainted(tainted('a')); | ||
//OK: | ||
sink($ok); | ||
|
||
//ruleid:tainted | ||
sink(tainted('b')); | ||
|
||
$bad = tainted('b'); | ||
//ruleid:tainted | ||
sink($bad); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
rules: | ||
- id: tainted | ||
languages: | ||
- php | ||
message: Match | ||
mode: taint | ||
options: | ||
taint_assume_safe_functions: true | ||
pattern-sinks: | ||
- pattern: sink(...) | ||
pattern-sources: | ||
- pattern: tainted(...) | ||
severity: WARNING | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
a = safe() | ||
i = tainted() | ||
x = a[i] | ||
#ok:tainted | ||
sink(x) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
rules: | ||
- id: tainted | ||
languages: | ||
- python | ||
message: Match | ||
mode: taint | ||
options: | ||
taint_assume_safe_indexes: true | ||
pattern-sinks: | ||
- pattern: sink(...) | ||
pattern-sources: | ||
- pattern: tainted(...) | ||
severity: WARNING | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.