Skip to content

Commit 1a91a79

Browse files
codeql-cicodeql-ci
authored
Merge pull request #5841 from erik-krogh/libCode
Approved by esbena, ethanpalm
2 parents d05dbb2 + 2f7f9d9 commit 1a91a79

17 files changed

+313
-72
lines changed

‎javascript/ql/lib/semmle/javascript/PackageExports.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,10 @@ private DataFlow::Node getAnExportFromModule(Module mod) {
216216
or
217217
result = mod.getABulkExportedNode()
218218
or
219+
// exports saved to the global object
220+
result = DataFlow::globalObjectRef().getAPropertyWrite().getRhs() and
221+
result.getTopLevel() = mod
222+
or
219223
result.analyze().getAValue() = TAbstractModuleObject(mod)
220224
}
221225

‎javascript/ql/lib/semmle/javascript/security/dataflow/CodeInjectionCustomizations.qll

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,25 @@ module CodeInjection {
252252
}
253253
}
254254

255+
/**
256+
* A system command execution of "node", where the executed code is seen as a code injection sink.
257+
*/
258+
class NodeCallSink extends Sink {
259+
NodeCallSink() {
260+
exists(SystemCommandExecution s |
261+
s.getACommandArgument().mayHaveStringValue("node")
262+
or
263+
s.getACommandArgument() =
264+
DataFlow::globalVarRef("process").getAPropertyRead("argv").getAPropertyRead("0")
265+
|
266+
exists(DataFlow::SourceNode arr | arr = s.getArgumentList().getALocalSource() |
267+
arr.getAPropertyWrite().getRhs().mayHaveStringValue("-e") and
268+
this = arr.getAPropertyWrite().getRhs()
269+
)
270+
)
271+
}
272+
}
273+
255274
/** A sink for code injection via template injection. */
256275
abstract private class TemplateSink extends Sink {
257276
override string getMessageSuffix() {
@@ -356,4 +375,9 @@ module CodeInjection {
356375
this = LodashUnderscore::member("template").getACall().getArgument(0)
357376
}
358377
}
378+
379+
/**
380+
* A call to JSON.stringify() seen as a sanitizer.
381+
*/
382+
class JSONStringifySanitizer extends Sanitizer, JsonStringifyCall { }
359383
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstruction.qll

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
/**
2+
* Provides a taint-tracking configuration for reasoning about code
3+
* constructed from libary input vulnerabilities.
4+
*
5+
* Note, for performance reasons: only import this file if
6+
* `UnsafeCodeConstruction::Configuration` is needed, otherwise
7+
* `UnsafeCodeConstructionCustomizations` should be imported instead.
8+
*/
9+
10+
import javascript
11+
12+
/**
13+
* Classes and predicates for the code constructed from library input query.
14+
*/
15+
module UnsafeCodeConstruction {
16+
private import semmle.javascript.security.dataflow.CodeInjectionCustomizations::CodeInjection as CodeInjection
17+
import UnsafeCodeConstructionCustomizations::UnsafeCodeConstruction
18+
19+
/**
20+
* A taint-tracking configuration for reasoning about unsafe code constructed from library input.
21+
*/
22+
class Configuration extends TaintTracking::Configuration {
23+
Configuration() { this = "UnsafeCodeConstruction" }
24+
25+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
26+
27+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
28+
29+
override predicate isSanitizer(DataFlow::Node node) {
30+
super.isSanitizer(node) or
31+
node instanceof CodeInjection::Sanitizer
32+
}
33+
34+
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
35+
// HTML sanitizers are insufficient protection against code injection
36+
src = trg.(HtmlSanitizerCall).getInput()
37+
or
38+
DataFlow::localFieldStep(src, trg)
39+
}
40+
41+
// override to require that there is a path without unmatched return steps
42+
override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
43+
super.hasFlowPath(source, sink) and
44+
DataFlow::hasPathWithoutUnmatchedReturn(source, sink)
45+
}
46+
}
47+
}

‎javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstructionCustomizations.qll

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about code
3+
* constructed from libary input vulnerabilities, as well as extension points for
4+
* adding your own.
5+
*/
6+
7+
import javascript
8+
9+
/**
10+
* Module containing sources, sinks, and sanitizers for code constructed from library input.
11+
*/
12+
module UnsafeCodeConstruction {
13+
private import semmle.javascript.security.dataflow.CodeInjectionCustomizations::CodeInjection as CodeInjection
14+
private import semmle.javascript.PackageExports as Exports
15+
16+
/**
17+
* A source for code constructed from library input vulnerabilities.
18+
*/
19+
abstract class Source extends DataFlow::Node { }
20+
21+
/**
22+
* A parameter of an exported function, seen as a source.
23+
*/
24+
class ExternalInputSource extends Source, DataFlow::ParameterNode {
25+
ExternalInputSource() {
26+
this = Exports::getALibraryInputParameter() and
27+
// permit parameters that clearly are intended to contain executable code.
28+
not this.getName() = "code"
29+
}
30+
}
31+
32+
/**
33+
* A sink for unsafe code constructed from library input vulnerabilities.
34+
*/
35+
abstract class Sink extends DataFlow::Node {
36+
/**
37+
* Gets the node where the unsafe code is executed.
38+
*/
39+
abstract DataFlow::Node getCodeSink();
40+
}
41+
42+
/**
43+
* Gets a node that is later executed as code in `codeSink`.
44+
*/
45+
private DataFlow::Node isExecutedAsCode(DataFlow::TypeBackTracker t, CodeInjection::Sink codeSink) {
46+
t.start() and result = codeSink
47+
or
48+
exists(DataFlow::TypeBackTracker t2 | t2 = t.smallstep(result, isExecutedAsCode(t, codeSink)))
49+
}
50+
51+
/**
52+
* A string concatenation leaf that is later executed as code.
53+
*/
54+
class StringConcatExecutedAsCode extends Sink, StringOps::ConcatenationLeaf {
55+
CodeInjection::Sink codeSink;
56+
57+
StringConcatExecutedAsCode() {
58+
this.getRoot() = isExecutedAsCode(DataFlow::TypeBackTracker::end(), codeSink)
59+
}
60+
61+
override DataFlow::Node getCodeSink() { result = codeSink }
62+
}
63+
}

‎javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.qhelp

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
When a library function dynamically constructs code in a potentially unsafe way, then
9+
it's important to document to clients of the library that the function should only be
10+
used with trusted inputs.
11+
12+
If the function is not documented as being potentially unsafe, then a client may
13+
incorrectly use inputs containing unsafe code fragments, and thereby leave the
14+
client vulnerable to code-injection attacks.
15+
</p>
16+
17+
</overview>
18+
19+
<recommendation>
20+
<p>
21+
Properly document library functions that construct code from unsanitized
22+
inputs, or avoid constructing code in the first place.
23+
</p>
24+
</recommendation>
25+
26+
<example>
27+
<p>
28+
The following example shows two methods implemented using `eval`: a simple
29+
deserialization routine and a getter method.
30+
If untrusted inputs are used with these methods,
31+
then an attacker might be able to execute arbitrary code on the system.
32+
</p>
33+
34+
<sample src="examples/UnsafeCodeConstruction.js" />
35+
36+
<p>
37+
To avoid this problem, either properly document that the function is potentially
38+
unsafe, or use an alternative solution such as `JSON.parse` or another library, like in the examples below,
39+
that does not allow arbitrary code to be executed.
40+
</p>
41+
42+
<sample src="examples/UnsafeCodeConstructionSafe.js" />
43+
44+
</example>
45+
46+
<references>
47+
<li>
48+
OWASP:
49+
<a href="https://www.owasp.org/index.php/Code_Injection">Code Injection</a>.
50+
</li>
51+
<li>
52+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Code_injection">Code Injection</a>.
53+
</li>
54+
</references>
55+
</qhelp>

‎javascript/ql/src/Security/CWE-094/UnsafeCodeConstruction.ql

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Unsafe code constructed from libary input
3+
* @description Using externally controlled strings to construct code may allow a malicious
4+
* user to execute arbitrary code.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @precision medium
8+
* @id js/unsafe-code-construction
9+
* @tags security
10+
* external/cwe/cwe-094
11+
* external/cwe/cwe-079
12+
* external/cwe/cwe-116
13+
*/
14+
15+
import javascript
16+
import DataFlow::PathGraph
17+
import semmle.javascript.security.dataflow.UnsafeCodeConstruction::UnsafeCodeConstruction
18+
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
20+
where cfg.hasFlowPath(source, sink)
21+
select sink.getNode(), source, sink, "$@ flows to here and is later $@.", source.getNode(),
22+
"Library input", sink.getNode().(Sink).getCodeSink(), "interpreted as code"

‎javascript/ql/src/Security/CWE-094/examples/UnsafeCodeConstruction.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
export function unsafeDeserialize(value) {
2+
return eval(`(${value})`);
3+
}
4+
5+
export function unsafeGetter(obj, path) {
6+
return eval(`obj.${path}`);
7+
}

‎javascript/ql/src/Security/CWE-094/examples/UnsafeCodeConstructionSafe.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
export function safeDeserialize(value) {
2+
return JSON.parse(value);
3+
}
4+
5+
const _ = require("lodash");
6+
export function safeGetter(object, path) {
7+
return _.get(object, path);
8+
}

‎javascript/ql/src/change-notes/2022-02-04-unsafe-code-construction.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: newQuery
3+
---
4+
* A new query, `js/unsafe-code-construction`, has been added to the query suite, highlighting libraries that may leave clients vulnerable to arbitary code execution.
5+
The query is not run by default.

‎javascript/ql/test/library-tests/frameworks/Templating/CodeInjection.expected

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ nodes
1515
| app.js:53:30:53:58 | req.que ... tedCode |
1616
| app.js:54:33:54:64 | req.que ... CodeRaw |
1717
| app.js:54:33:54:64 | req.que ... CodeRaw |
18-
| app.js:55:37:55:72 | req.que ... JsonRaw |
19-
| app.js:55:37:55:72 | req.que ... JsonRaw |
2018
| app.js:56:25:56:48 | req.que ... shSink1 |
2119
| app.js:56:25:56:48 | req.que ... shSink1 |
2220
| app.js:58:35:58:68 | req.que ... rString |
@@ -64,11 +62,6 @@ nodes
6462
| views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
6563
| views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
6664
| views/njk_sinks.njk:14:45:14:73 | dataInG ... \| safe |
67-
| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
68-
| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
69-
| views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
70-
| views/njk_sinks.njk:15:49:15:81 | dataInG ... \| json |
71-
| views/njk_sinks.njk:15:49:15:88 | dataInG ... \| safe |
7265
| views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
7366
| views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
7467
| views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
@@ -96,8 +89,6 @@ edges
9689
| app.js:53:30:53:58 | req.que ... tedCode | views/njk_sinks.njk:13:42:13:60 | dataInGeneratedCode |
9790
| app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
9891
| app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw |
99-
| app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
100-
| app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw |
10192
| app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
10293
| app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:22:17:35 | backslashSink1 |
10394
| app.js:58:35:58:68 | req.que ... rString | views/njk_sinks.njk:22:42:22:65 | dataInE ... rString |
@@ -137,10 +128,6 @@ edges
137128
| views/njk_sinks.njk:14:45:14:66 | dataInG ... CodeRaw | views/njk_sinks.njk:14:45:14:73 | dataInG ... \| safe |
138129
| views/njk_sinks.njk:14:45:14:73 | dataInG ... \| safe | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
139130
| views/njk_sinks.njk:14:45:14:73 | dataInG ... \| safe | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} |
140-
| views/njk_sinks.njk:15:49:15:74 | dataInG ... JsonRaw | views/njk_sinks.njk:15:49:15:81 | dataInG ... \| json |
141-
| views/njk_sinks.njk:15:49:15:81 | dataInG ... \| json | views/njk_sinks.njk:15:49:15:88 | dataInG ... \| safe |
142-
| views/njk_sinks.njk:15:49:15:88 | dataInG ... \| safe | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
143-
| views/njk_sinks.njk:15:49:15:88 | dataInG ... \| safe | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} |
144131
| views/njk_sinks.njk:17:22:17:35 | backslashSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
145132
| views/njk_sinks.njk:17:22:17:35 | backslashSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} |
146133
| views/njk_sinks.njk:22:42:22:65 | dataInE ... rString | views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} |
@@ -161,7 +148,6 @@ edges
161148
| views/hbs_sinks.hbs:33:39:33:68 | {{ dataInEventHandlerString }} | app.js:38:35:38:68 | req.que ... rString | views/hbs_sinks.hbs:33:39:33:68 | {{ dataInEventHandlerString }} | $@ flows to here and is interpreted as code. | app.js:38:35:38:68 | req.que ... rString | User-provided value |
162149
| views/njk_sinks.njk:13:39:13:63 | {{ dataInGeneratedCode }} | app.js:53:30:53:58 | req.que ... tedCode | views/njk_sinks.njk:13:39:13:63 | {{ dataInGeneratedCode }} | $@ flows to here and is interpreted as code. | app.js:53:30:53:58 | req.que ... tedCode | User-provided value |
163150
| views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} | app.js:54:33:54:64 | req.que ... CodeRaw | views/njk_sinks.njk:14:42:14:76 | {{ dataInGeneratedCodeRaw \| safe }} | $@ flows to here and is interpreted as code. | app.js:54:33:54:64 | req.que ... CodeRaw | User-provided value |
164-
| views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} | app.js:55:37:55:72 | req.que ... JsonRaw | views/njk_sinks.njk:15:46:15:91 | {{ dataInGeneratedCodeJsonRaw \| json \| safe }} | $@ flows to here and is interpreted as code. | app.js:55:37:55:72 | req.que ... JsonRaw | User-provided value |
165151
| views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} | app.js:56:25:56:48 | req.que ... shSink1 | views/njk_sinks.njk:17:19:17:38 | {{ backslashSink1 }} | $@ flows to here and is interpreted as code. | app.js:56:25:56:48 | req.que ... shSink1 | User-provided value |
166152
| views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} | app.js:58:35:58:68 | req.que ... rString | views/njk_sinks.njk:22:39:22:68 | {{ dataInEventHandlerString }} | $@ flows to here and is interpreted as code. | app.js:58:35:58:68 | req.que ... rString | User-provided value |
167153
| views/njk_sinks.njk:23:39:23:78 | {{ dataInEventHandlerStringRaw \| safe }} | app.js:59:38:59:74 | req.que ... ringRaw | views/njk_sinks.njk:23:39:23:78 | {{ dataInEventHandlerStringRaw \| safe }} | $@ flows to here and is interpreted as code. | app.js:59:38:59:74 | req.que ... ringRaw | User-provided value |

0 commit comments

Comments
 (0)