Skip to content

C++: Fix false negatives for postfix crement expressions#2799

Merged
jbj merged 8 commits into
github:masterfrom
MathiasVP:missing-flow-in-crement
Feb 12, 2020
Merged

C++: Fix false negatives for postfix crement expressions#2799
jbj merged 8 commits into
github:masterfrom
MathiasVP:missing-flow-in-crement

Conversation

@MathiasVP

@MathiasVP MathiasVP commented Feb 9, 2020

Copy link
Copy Markdown
Contributor

This commit fixes an issue where a PostfixIncrExpr in a void context would not be assigned a DataFlow::Node when switching to DefaultTaintTracking. i.e., the query

import cpp
import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil

from PostfixIncrExpr e
select e, exprNode(e)

never returned any results. This meant that adjustedSink in DefaultTaintTracking never returned a PostfixIncrExpr.

The fix is to always generate a CopyValueInstruction even though the e++ (or e--) expression happens in a void context. The fix is to add a case for PostfixCrementOperation in adjustedSink. This recovers two false negatives in Security/CWE/CWE-190/semmle/tainted/IntegerOverflowTainted.expected:

--- expected
+++ actual
@@ -2,3 +2,5 @@
 | 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 |
+| test.c:44:7:44:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:41:17:41:20 | argv | User-provided value |
+| test.c:54:7:54:12 | ... -- | $@ flows to here and is used in an expression which might overflow negatively. | test.c:51:17:51:20 | argv | User-provided value |

@MathiasVP MathiasVP added the C++ label Feb 9, 2020
@MathiasVP MathiasVP requested a review from a team as a code owner February 9, 2020 20:44
@jbj

jbj commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

I made a similar change in the original version of #1999, but based on feedback from @dbartol in #1999 (comment) I changed it so there's no instruction mapped to the result of an assignment/crement instruction in void context. I think the main motivation was to keep the size of the IR minimal.

Can you work around this problem in adjustedSink instead, making it taint the crement (and assignment?) expression whenever the cremented expression is tainted?

@MathiasVP

Copy link
Copy Markdown
Contributor Author

I made a similar change in the original version of #1999, but based on feedback from @dbartol in #1999 (comment) I changed it so there's no instruction mapped to the result of an assignment/crement instruction in void context. I think the main motivation was to keep the size of the IR minimal.

Thanks for the heads up. It's an interesting read.

Can you work around this problem in adjustedSink instead, making it taint the crement (and assignment?) expression whenever the cremented expression is tainted?

I've pushed a commit that does this now. Hopefully this is along the lines of what you suggested.

@jbj

jbj commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

Looks good. Is there a need to do something similar for += etc. as well? Or even for =?

@MathiasVP

Copy link
Copy Markdown
Contributor Author

Looks good. Is there a need to do something similar for += etc. as well? Or even for =?

For += and friends we need a similar fix, yes.

I'm not sure if any query benefits from propagating taint from an operand to an = expression in a void context, though.

@MathiasVP

Copy link
Copy Markdown
Contributor Author

I've added the noncontroversial part of @jbj's comment (i.e., the += expressions and friends) to adjustedSink as it was an obvious follow up that improved the same query IntegerOverflowTainted.ql as the original PR (albeit lacking tests to back this up).

I've left out adding = since it's not immediately clear what the benefits of adding a case for that one in adjustedSink is.

Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll Outdated
@rdmarsh2

rdmarsh2 commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

I don't think flow to the result of = in a void context would benefit our existing queries, but the distinctions between the RHS, LHS, and result of an assignment are a common source of confusion for new QL writers.

Comment thread cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll Outdated

@jbj jbj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@jbj jbj merged commit 2abe416 into github:master Feb 12, 2020
@MathiasVP MathiasVP deleted the missing-flow-in-crement branch February 12, 2020 14:08
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.

4 participants