-
Notifications
You must be signed in to change notification settings - Fork 1.8k
CPP: Model taint through std::string and std::stringstream #1637
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
Changes from all commits
fde6227
0cabc3a
2cd6aa7
afcedcf
23167c4
f90647a
eabfd9f
157c707
4843c73
348d64d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import semmle.code.cpp.models.interfaces.DataFlow | ||
import semmle.code.cpp.models.interfaces.Taint | ||
|
||
/** | ||
* The `std::basic_string` constructor(s). | ||
*/ | ||
class StringConstructor extends DataFlowFunction { | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
StringConstructor() { | ||
this.hasQualifiedName("std", "basic_string", "basic_string") | ||
} | ||
|
||
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { | ||
// flow from any constructor argument to return value | ||
input.isInParameter(_) and | ||
output.isOutReturnValue() | ||
} | ||
} | ||
|
||
/** | ||
* The standard function `std::string.c_str`. | ||
*/ | ||
class StringCStr extends DataFlowFunction { | ||
StringCStr() { | ||
this.hasQualifiedName("std", "basic_string", "c_str") | ||
} | ||
|
||
override predicate hasDataFlow(FunctionInput input, FunctionOutput output) { | ||
// flow from string itself (qualifier) to return value | ||
input.isInQualifier() and | ||
output.isOutReturnValue() | ||
} | ||
} | ||
|
||
/** | ||
* The standard function `operator<<`, for example on `std::stringstream`. | ||
*/ | ||
class InsertionOperator extends TaintFunction { | ||
InsertionOperator() { | ||
this.hasQualifiedName("std", "operator<<") | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { | ||
( | ||
// flow from second argument to return value | ||
input.isInParameterPointer(1) and | ||
output.isOutReturnValue() | ||
) or ( | ||
// flow from return value to first argument | ||
// (this is a bit odd; another way to think of it is the sink effectively | ||
// flowing from the first argument to the return value) | ||
input.isInReturnValue() and | ||
output.isOutParameterPointer(0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it makes sense for taint to come in through the return value. Can we model There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of:
i.e.
taint needs to flow from the bracketed expression to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I understand the problem now. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm understanding correctly this won't actually be a change to the model - you're still asking for flow from the return value to the first parameter. You just want the taint library to wire that up to the post-update node of that parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think I understand the problem now. It's the third time I think I understand it, so bear with me. I think there are two things to be modelled about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more precise, I think these are the steps we want in the model of
We should really write some documentation in When a model specifies "parameter value" to "return value" data flow, I think the data flow library should add both forward flow between the pre-update nodes and backwards flow between the post-update nodes. I had at one point a plan to generalise https://github.com/Semmle/ql/pull/1766/files#diff-255652465f915d69e8f3adeed3e96f11R238 to add such backwards post-update flow everywhere that the right post-update nodes existed, but we decided that it was more sensible to add these cases one at a time. As an example of why we want such flow, consider an identity function on reference types: When a model specifies flow to a With this mapping in place between the models and the data flow library, I think it's going to work. Then we get the following data-flow edges for your example with
In that graph, there is a path from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've discussed this with @aschackmull. The Java library has a
The post-update node I mentioned above may not exist. I'd like to try and investigate whether a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the two-rule modelling of
That gives us the following edges for
|
||
) | ||
} | ||
} | ||
|
||
/** | ||
* The standard function `std::stringstream.str`. | ||
*/ | ||
class StringStreamStr extends TaintFunction { | ||
StringStreamStr() { | ||
this.hasQualifiedName("std", "basic_stringstream", "str") | ||
} | ||
|
||
override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) { | ||
// flow from object itself (qualifier) to return value | ||
input.isInQualifier() and | ||
output.isOutReturnValue() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
| stl.cpp:66:16:66:21 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 0 | OutReturnValue | | ||
| stl.cpp:66:16:66:21 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 1 | OutReturnValue | | ||
| stl.cpp:66:16:66:21 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 2 | OutReturnValue | | ||
| stl.cpp:67:16:67:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 0 | OutReturnValue | | ||
| stl.cpp:67:16:67:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 1 | OutReturnValue | | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| stl.cpp:67:16:67:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 2 | OutReturnValue | | ||
| stl.cpp:72:9:72:13 | call to c_str | stl.cpp:21:16:21:20 | c_str | dataflow | InQualifier | OutReturnValue | | ||
| stl.cpp:73:9:73:13 | call to c_str | stl.cpp:21:16:21:20 | c_str | dataflow | InQualifier | OutReturnValue | | ||
| stl.cpp:79:16:79:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 0 | OutReturnValue | | ||
| stl.cpp:79:16:79:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 1 | OutReturnValue | | ||
| stl.cpp:79:16:79:24 | call to basic_string | stl.cpp:19:3:19:14 | basic_string | dataflow | InParameter 2 | OutReturnValue | | ||
| stl.cpp:81:6:81:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InParameterPointer 1 | OutReturnValue | | ||
| stl.cpp:81:6:81:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InReturnValue | OutParameterPointer 0 | | ||
| stl.cpp:82:6:82:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InParameterPointer 1 | OutReturnValue | | ||
| stl.cpp:82:6:82:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InReturnValue | OutParameterPointer 0 | | ||
| stl.cpp:83:6:83:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InParameterPointer 1 | OutReturnValue | | ||
| stl.cpp:83:6:83:6 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InReturnValue | OutParameterPointer 0 | | ||
| stl.cpp:83:15:83:15 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InParameterPointer 1 | OutReturnValue | | ||
| stl.cpp:83:15:83:15 | call to operator<< | stl.cpp:39:67:39:76 | operator<< | taint | InReturnValue | OutParameterPointer 0 | | ||
| stl.cpp:84:6:84:6 | call to operator<< | stl.cpp:40:85:40:94 | operator<< | taint | InParameterPointer 1 | OutReturnValue | | ||
| stl.cpp:84:6:84:6 | call to operator<< | stl.cpp:40:85:40:94 | operator<< | taint | InReturnValue | OutParameterPointer 0 | | ||
| stl.cpp:90:11:90:13 | call to str | stl.cpp:52:42:52:44 | str | taint | InQualifier | OutReturnValue | | ||
| stl.cpp:91:11:91:13 | call to str | stl.cpp:52:42:52:44 | str | taint | InQualifier | OutReturnValue | | ||
| stl.cpp:92:11:92:13 | call to str | stl.cpp:52:42:52:44 | str | taint | InQualifier | OutReturnValue | | ||
| stl.cpp:93:11:93:13 | call to str | stl.cpp:52:42:52:44 | str | taint | InQualifier | OutReturnValue | | ||
| taint.cpp:170:3:170:8 | call to strcpy | taint.cpp:156:7:156:12 | strcpy | dataflow | InParameter 0 | OutReturnValue | | ||
| taint.cpp:170:3:170:8 | call to strcpy | taint.cpp:156:7:156:12 | strcpy | dataflow | InParameterPointer 1 | OutParameterPointer 0 | | ||
| taint.cpp:170:3:170:8 | call to strcpy | taint.cpp:156:7:156:12 | strcpy | dataflow | InParameterPointer 1 | OutReturnPointer | | ||
| taint.cpp:172:3:172:8 | call to strcat | taint.cpp:157:7:157:12 | strcat | dataflow | InParameter 0 | OutReturnValue | | ||
| taint.cpp:172:3:172:8 | call to strcat | taint.cpp:157:7:157:12 | strcat | taint | InParameter 1 | OutParameterPointer 0 | | ||
| taint.cpp:172:3:172:8 | call to strcat | taint.cpp:157:7:157:12 | strcat | taint | InParameterPointer 0 | OutParameterPointer 0 | | ||
| taint.cpp:194:2:194:7 | call to memcpy | taint.cpp:190:7:190:12 | memcpy | dataflow | InParameter 0 | OutReturnValue | | ||
| taint.cpp:194:2:194:7 | call to memcpy | taint.cpp:190:7:190:12 | memcpy | dataflow | InParameterPointer 1 | OutParameterPointer 0 | | ||
| taint.cpp:194:2:194:7 | call to memcpy | taint.cpp:190:7:190:12 | memcpy | dataflow | InParameterPointer 1 | OutReturnPointer | | ||
| taint.cpp:194:2:194:7 | call to memcpy | taint.cpp:190:7:190:12 | memcpy | taint | InParameter 2 | OutParameterPointer 0 | | ||
| taint.cpp:194:2:194:7 | call to memcpy | taint.cpp:190:7:190:12 | memcpy | taint | InParameter 2 | OutReturnPointer | | ||
| taint.cpp:213:2:213:10 | call to swap | taint.cpp:201:35:201:38 | swap | dataflow | InParameterPointer 0 | OutParameterPointer 1 | | ||
| taint.cpp:213:2:213:10 | call to swap | taint.cpp:201:35:201:38 | swap | dataflow | InParameterPointer 1 | OutParameterPointer 0 | |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import cpp | ||
import semmle.code.cpp.dataflow.TaintTracking | ||
import semmle.code.cpp.models.interfaces.DataFlow | ||
import semmle.code.cpp.models.interfaces.Taint | ||
|
||
from FunctionCall fc, Function f, string str, FunctionInput inModel, FunctionOutput outModel | ||
where | ||
fc.getTarget() = f and | ||
( | ||
( | ||
f.(TaintFunction).hasTaintFlow(inModel, outModel) and | ||
str = "taint" | ||
) or ( | ||
f.(DataFlowFunction).hasDataFlow(inModel, outModel) and | ||
str = "dataflow" | ||
) | ||
) | ||
select | ||
fc, f, str, inModel, outModel |
Uh oh!
There was an error while loading. Please reload this page.