Skip to content

Commit 530a4ae

Browse files
erik-krogherik-krogh
committed
Merge branch 'main' into shellSanitizer
2 parents f7f8868 + 2bb9636 commit 530a4ae

File tree

17 files changed

+161
-45
lines changed

17 files changed

+161
-45
lines changed

‎cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ class QueryString extends EnvironmentRead {
2929
}
3030

3131
class Configuration extends TaintTrackingConfiguration {
32+
override predicate isSource(Expr source) { source instanceof QueryString }
33+
3234
override predicate isSink(Element tainted) {
3335
exists(PrintStdoutCall call | call.getAnArgument() = tainted)
3436
}

‎cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ predicate sqlite_encryption_used() {
3434
}
3535

3636
class Configuration extends TaintTrackingConfiguration {
37+
override predicate isSource(Expr source) {
38+
super.isSource(source) and source instanceof SensitiveExpr
39+
}
40+
3741
override predicate isSink(Element taintedArg) {
3842
exists(SqliteFunctionCall sqliteCall |
3943
taintedArg = sqliteCall.getASource() and

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll‎

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,21 +46,23 @@ predicate predictableOnlyFlow(string name) {
4646

4747
private DataFlow::Node getNodeForSource(Expr source) {
4848
isUserInput(source, _) and
49-
(
50-
result = DataFlow::exprNode(source)
51-
or
52-
// Some of the sources in `isUserInput` are intended to match the value of
53-
// an expression, while others (those modeled below) are intended to match
54-
// the taint that propagates out of an argument, like the `char *` argument
55-
// to `gets`. It's impossible here to tell which is which, but the "access
56-
// to argv" source is definitely not intended to match an output argument,
57-
// and it causes false positives if we let it.
58-
//
59-
// This case goes together with the similar (but not identical) rule in
60-
// `nodeIsBarrierIn`.
61-
result = DataFlow::definitionByReferenceNodeFromArgument(source) and
62-
not argv(source.(VariableAccess).getTarget())
63-
)
49+
result = getNodeForExpr(source)
50+
}
51+
52+
private DataFlow::Node getNodeForExpr(Expr node) {
53+
result = DataFlow::exprNode(node)
54+
or
55+
// Some of the sources in `isUserInput` are intended to match the value of
56+
// an expression, while others (those modeled below) are intended to match
57+
// the taint that propagates out of an argument, like the `char *` argument
58+
// to `gets`. It's impossible here to tell which is which, but the "access
59+
// to argv" source is definitely not intended to match an output argument,
60+
// and it causes false positives if we let it.
61+
//
62+
// This case goes together with the similar (but not identical) rule in
63+
// `nodeIsBarrierIn`.
64+
result = DataFlow::definitionByReferenceNodeFromArgument(node) and
65+
not argv(node.(VariableAccess).getTarget())
6466
}
6567

6668
private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
@@ -537,6 +539,9 @@ module TaintedWithPath {
537539
* a characteristic predicate.
538540
*/
539541
class TaintTrackingConfiguration extends TSingleton {
542+
/** Override this to specify which elements are sources in this configuration. */
543+
predicate isSource(Expr source) { exists(getNodeForSource(source)) }
544+
540545
/** Override this to specify which elements are sinks in this configuration. */
541546
abstract predicate isSink(Element e);
542547

@@ -553,7 +558,11 @@ module TaintedWithPath {
553558
private class AdjustedConfiguration extends DataFlow3::Configuration {
554559
AdjustedConfiguration() { this = "AdjustedConfiguration" }
555560

556-
override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
561+
override predicate isSource(DataFlow::Node source) {
562+
exists(TaintTrackingConfiguration cfg, Expr e |
563+
cfg.isSource(e) and source = getNodeForExpr(e)
564+
)
565+
}
557566

558567
override predicate isSink(DataFlow::Node sink) {
559568
exists(TaintTrackingConfiguration cfg | cfg.isSink(adjustedSink(sink)))
@@ -596,7 +605,8 @@ module TaintedWithPath {
596605
exists(AdjustedConfiguration cfg, DataFlow3::Node sourceNode, DataFlow3::Node sinkNode |
597606
cfg.hasFlow(sourceNode, sinkNode)
598607
|
599-
sourceNode = getNodeForSource(e)
608+
sourceNode = getNodeForExpr(e) and
609+
exists(TaintTrackingConfiguration ttCfg | ttCfg.isSource(e))
600610
or
601611
e = adjustedSink(sinkNode) and
602612
exists(TaintTrackingConfiguration ttCfg | ttCfg.isSink(e))
@@ -650,7 +660,7 @@ module TaintedWithPath {
650660

651661
/** A PathNode whose `Element` is a source. It may also be a sink. */
652662
private class InitialPathNode extends EndpointPathNode {
653-
InitialPathNode() { exists(getNodeForSource(this.inner())) }
663+
InitialPathNode() { exists(TaintTrackingConfiguration cfg | cfg.isSource(this.inner())) }
654664
}
655665

656666
/** A PathNode whose `Element` is a sink. It may also be a source. */
@@ -672,14 +682,14 @@ module TaintedWithPath {
672682
// Same for the first node
673683
exists(WrapPathNode sourceNode |
674684
DataFlow3::PathGraph::edges(sourceNode.inner(), b.(WrapPathNode).inner()) and
675-
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner())
685+
sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner())
676686
)
677687
or
678688
// Finally, handle the case where the path goes directly from a source to a
679689
// sink, meaning that they both need to be translated.
680690
exists(WrapPathNode sinkNode, WrapPathNode sourceNode |
681691
DataFlow3::PathGraph::edges(sourceNode.inner(), sinkNode.inner()) and
682-
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner()) and
692+
sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner()) and
683693
b.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
684694
)
685695
}
@@ -702,7 +712,7 @@ module TaintedWithPath {
702712
predicate taintedWithPath(Expr source, Element tainted, PathNode sourceNode, PathNode sinkNode) {
703713
exists(AdjustedConfiguration cfg, DataFlow3::Node flowSource, DataFlow3::Node flowSink |
704714
source = sourceNode.(InitialPathNode).inner() and
705-
flowSource = getNodeForSource(source) and
715+
flowSource = getNodeForExpr(source) and
706716
cfg.hasFlow(flowSource, flowSink) and
707717
tainted = adjustedSink(flowSink) and
708718
tainted = sinkNode.(FinalPathNode).inner()
@@ -724,8 +734,8 @@ module TaintedWithPath {
724734
* through a global variable.
725735
*/
726736
predicate taintedWithoutGlobals(Element tainted) {
727-
exists(PathNode sourceNode, FinalPathNode sinkNode |
728-
sourceNode.(WrapPathNode).inner().getNode() = getNodeForSource(_) and
737+
exists(AdjustedConfiguration cfg, PathNode sourceNode, FinalPathNode sinkNode |
738+
cfg.isSource(sourceNode.(WrapPathNode).inner().getNode()) and
729739
edgesWithoutGlobals+(sourceNode, sinkNode) and
730740
tainted = sinkNode.inner()
731741
)

‎docs/codeql/codeql-language-guides/analyzing-data-flow-in-csharp.rst‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ Exercise 4
480480
Configuration() { this="Environment to System.Uri" }
481481
482482
override predicate isSource(DataFlow::Node src) {
483-
src.asExpr() instanceof EnvironmentVariableFlowSource
483+
src instanceof EnvironmentVariableFlowSource
484484
}
485485
486486
override predicate isSink(DataFlow::Node sink) {

‎javascript/change-notes/202020-12-22-execa.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* The command injection security queries now recognize additional sinks.
3+
Affected packages are
4+
[execa](https://npmjs.com/package/execa)

‎javascript/ql/src/AngularJS/DoubleCompilation.ql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
* @id js/angular/double-compilation
88
* @tags reliability
99
* frameworks/angularjs
10+
* security
1011
* @precision very-high
1112
*/
1213

‎javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -628,7 +628,11 @@ module DataFlow {
628628
override string getPropertyName() { result = astNode.getArgument(1).getStringValue() }
629629

630630
override Node getRhs() {
631-
result = astNode.getArgument(2).(ObjectExpr).getPropertyByName("value").getInit().flow()
631+
exists(ObjectExpr obj | obj = astNode.getArgument(2) |
632+
result = obj.getPropertyByName("value").getInit().flow()
633+
or
634+
result = obj.getPropertyByName("get").getInit().flow().(DataFlow::FunctionNode).getAReturn()
635+
)
632636
}
633637

634638
override ControlFlowNode getWriteNode() { result = astNode }

‎javascript/ql/src/semmle/javascript/frameworks/SystemCommandExecutors.qll‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,15 +18,18 @@ private predicate execApi(string mod, string fn, int cmdArg, int optionsArg, boo
1818
shell = false and
1919
(
2020
fn = "node" or
21-
fn = "shell" or
22-
fn = "shellSync" or
2321
fn = "stdout" or
2422
fn = "stderr" or
2523
fn = "sync"
2624
)
2725
or
2826
shell = true and
29-
(fn = "command" or fn = "commandSync")
27+
(
28+
fn = "command" or
29+
fn = "commandSync" or
30+
fn = "shell" or
31+
fn = "shellSync"
32+
)
3033
) and
3134
cmdArg = 0
3235
}

‎javascript/ql/src/semmle/javascript/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,12 @@ module UnsafeShellCommandConstruction {
5252
*/
5353
class ExternalInputSource extends Source, DataFlow::ParameterNode {
5454
ExternalInputSource() {
55-
this =
56-
Exports::getAValueExportedBy(Exports::getTopmostPackageJSON())
57-
.(DataFlow::FunctionNode)
58-
.getAParameter() and
55+
exists(int bound, DataFlow::FunctionNode func |
56+
func =
57+
Exports::getAValueExportedBy(Exports::getTopmostPackageJSON())
58+
.getABoundFunctionValue(bound) and
59+
this = func.getParameter(any(int arg | arg >= bound))
60+
) and
5961
not this.getName() = ["cmd", "command"] // looks to be on purpose.
6062
}
6163
}

‎javascript/ql/test/query-tests/Security/CWE-078/ShellCommandInjectionFromEnvironment.expected‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,33 @@
11
nodes
2-
| tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
3-
| tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
4-
| tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") |
5-
| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname |
6-
| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname |
2+
| tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") |
3+
| tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") |
4+
| tst_shell-command-injection-from-environment.js:6:26:6:53 | path.jo ... "temp") |
5+
| tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname |
6+
| tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname |
7+
| tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") |
8+
| tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") |
9+
| tst_shell-command-injection-from-environment.js:8:26:8:53 | path.jo ... "temp") |
10+
| tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname |
11+
| tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname |
12+
| tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") |
13+
| tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") |
14+
| tst_shell-command-injection-from-environment.js:9:30:9:57 | path.jo ... "temp") |
15+
| tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname |
16+
| tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname |
717
edges
8-
| tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
9-
| tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") |
10-
| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") |
11-
| tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | tst_shell-command-injection-from-environment.js:5:26:5:53 | path.jo ... "temp") |
18+
| tst_shell-command-injection-from-environment.js:6:26:6:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") |
19+
| tst_shell-command-injection-from-environment.js:6:26:6:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") |
20+
| tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname | tst_shell-command-injection-from-environment.js:6:26:6:53 | path.jo ... "temp") |
21+
| tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname | tst_shell-command-injection-from-environment.js:6:26:6:53 | path.jo ... "temp") |
22+
| tst_shell-command-injection-from-environment.js:8:26:8:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") |
23+
| tst_shell-command-injection-from-environment.js:8:26:8:53 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") |
24+
| tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname | tst_shell-command-injection-from-environment.js:8:26:8:53 | path.jo ... "temp") |
25+
| tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname | tst_shell-command-injection-from-environment.js:8:26:8:53 | path.jo ... "temp") |
26+
| tst_shell-command-injection-from-environment.js:9:30:9:57 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") |
27+
| tst_shell-command-injection-from-environment.js:9:30:9:57 | path.jo ... "temp") | tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") |
28+
| tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname | tst_shell-command-injection-from-environment.js:9:30:9:57 | path.jo ... "temp") |
29+
| tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname | tst_shell-command-injection-from-environment.js:9:30:9:57 | path.jo ... "temp") |
1230
#select
13-
| tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") | tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | tst_shell-command-injection-from-environment.js:5:14:5:53 | 'rm -rf ... "temp") | This shell command depends on an uncontrolled $@. | tst_shell-command-injection-from-environment.js:5:36:5:44 | __dirname | absolute path |
31+
| tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") | tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname | tst_shell-command-injection-from-environment.js:6:14:6:53 | 'rm -rf ... "temp") | This shell command depends on an uncontrolled $@. | tst_shell-command-injection-from-environment.js:6:36:6:44 | __dirname | absolute path |
32+
| tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") | tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname | tst_shell-command-injection-from-environment.js:8:14:8:53 | 'rm -rf ... "temp") | This shell command depends on an uncontrolled $@. | tst_shell-command-injection-from-environment.js:8:36:8:44 | __dirname | absolute path |
33+
| tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") | tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname | tst_shell-command-injection-from-environment.js:9:18:9:57 | 'rm -rf ... "temp") | This shell command depends on an uncontrolled $@. | tst_shell-command-injection-from-environment.js:9:40:9:48 | __dirname | absolute path |

0 commit comments

Comments
 (0)