Skip to content

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Jan 24, 2020

This fixes FNs on the CWE-190/semmle/tainted/ArithmeticTainted.expected test with all open PRs applied.

It turns out that any function implementing SideEffectFunction is assumed not to read through its parameters unless hasSpecificReadSideEffect is implemented.

@jbj jbj added the C++ label Jan 24, 2020
@jbj jbj requested a review from rdmarsh2 January 24, 2020 14:58
@jbj jbj requested a review from a team as a code owner January 24, 2020 14:58
@jbj
Copy link
Contributor Author

jbj commented Jan 24, 2020

This is the diff for what's fixed:

--- expected
+++ actual
@@ -1,6 +1,9 @@
 | test3.c:15:10:15:10 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:15:14:15:14 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:15:18:15:18 | z | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test3.c:11:15:11:18 | argv | User-provided value |
+| test5.cpp:17:6:17:18 | call to getTaintedInt | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
+| test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
+| test5.cpp:19:6:19:6 | y | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test5.cpp:9:7:9:9 | buf | User-provided value |
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:14:15:14:28 | maxConnections | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:11:29:11:32 | argv | User-provided value |
 | test.c:44:7:44:10 | len2 | $@ flows to here and is used in arithmetic, potentially causing an underflow. | test.c:41:17:41:20 | argv | User-provided value |

The problem the was lack of a read side effect node on the call to strtoul.

@rdmarsh2
Copy link
Contributor

LGTM but needs test changes accepted.

@jbj
Copy link
Contributor Author

jbj commented Jan 27, 2020

Here's an additional improvement we're getting from this PR:

--- a/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected
@@ -1,3 +1,4 @@
 | test3.c:12:31:12:34 | * ... | $@ flows to here and is used in an expression which might overflow negatively. | test3.c:11:15:11:18 | argv | User-provided value |
 | test3.c:13:16:13:19 | * ... | $@ flows to here and is used in an expression which might overflow negatively. | test3.c:11:15:11:18 | argv | User-provided value |
 | test4.cpp:13:17:13:20 | access to array | $@ flows to here and is used in an expression which might overflow negatively. | test4.cpp:9:13:9:16 | argv | User-provided value |
+| test5.cpp:10:9:10:15 | call to strtoul | $@ flows to here and is used in an expression which might overflow. | test5.cpp:9:7:9:9 | buf | User-provided value |

There are also improvements in five expected files in the internal repo.

@rdmarsh2 rdmarsh2 merged commit 4d743d2 into github:master Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants