C++: Add EmscriptenRunScriptTaint query #13493
Conversation
jketema
left a comment
jketema
left a comment
There was a problem hiding this comment.
Hi @spaceraccoon,
Thanks for your contribution. Some initial comments below.
In addition:
- This is missing tests, which should go into
cpp/ql/test/experimental/query-tests/Security/ - Currently dies goes into a directory called
CVE-094that should probably have beenCWE-094
| EmscriptenRunScriptTaintConfig() { this = "EmscriptenRunScriptTaintConfig" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { | ||
| exists(Parameter p | source.asParameter() = p) |
There was a problem hiding this comment.
With this source, it looks to me like the following will be picked up by the query:
void foo(char *s) {
emscripten_run_script(s);
}
int main() {
foo("alert('FIXED CONSTANT')");
return 0;
};This is clearly not what we want, as the input to emscripten_run_script is clearly not user provided here.
It's better to do something like:
override predicate isSource(DataFlow::Node source) { source instanceof FlowSource }where you'll need:
import semmle.code.cpp.security.FlowSources| override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) { | ||
| exists(FunctionCall fc | | ||
| fc.getTarget().hasGlobalName("TextFormat") and | ||
| fc.getArgument(1) = fromNode.asExpr() and |
There was a problem hiding this comment.
Why is this restricted to the first argument? I would assume that dangerous input can flow into this function through any of it's arguments.
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p>Finding for C++ WebAssembly function declarations that pass their parameters to Emscripten run script functions. If exported WebAssembly functions use user-supplied data in Emscripten run script commands without proper sanitization, this can make the code vulnerable to JavaScript code injection.</p> |
There was a problem hiding this comment.
The query doesn't seem to check for for any sanitisation. This that intended?
| fc.getTarget().hasGlobalName("emscripten_run_script_int") or | ||
| fc.getTarget().hasGlobalName("emscripten_run_script_string") or | ||
| fc.getTarget().hasGlobalName("emscripten_async_run_script") or | ||
| fc.getTarget().hasGlobalName("emscripten_async_load_script") |
There was a problem hiding this comment.
Why is emscripten_async_load_script included here? Looking at the documentation, it seems that this only loads a script, but doesn't do anything with it. Wouldn't this only cause problems if the script is actually executed? If that's the case, it seems more appropriate to have calls to emscripten_async_load_script as an additional taint step.
|
Closing this due to inactivity. |
2 participants
Adds an experimental EmscriptenRunScriptTaint query to detect unsanitized user-input flows to Emscripten run script functions for exported WebAssembly functions.
Submitted for All for one, one for all bounty.