Skip to content

Commit 9d4cd0a

Browse files
codeql-cicodeql-ci
authored
Merge pull request #4862 from erik-krogh/shellSanitizer
Approved by esbena
2 parents f69ceb3 + 530a4ae commit 9d4cd0a

File tree

11 files changed

+80
-66
lines changed

11 files changed

+80
-66
lines changed

‎javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import javascript
1818
import DataFlow
1919
import PathGraph
2020
import semmle.javascript.DynamicPropertyAccess
21+
private import semmle.javascript.dataflow.InferredTypes
2122

2223
/**
2324
* A call of form `x.split(".")` where `x` is a parameter.
@@ -394,34 +395,31 @@ class InstanceOfGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::Value
394395
*/
395396
class TypeofGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::ValueNode {
396397
override EqualityTest astNode;
397-
TypeofExpr typeof;
398-
string typeofStr;
398+
Expr operand;
399+
TypeofTag tag;
399400

400-
TypeofGuard() {
401-
typeof = astNode.getAnOperand() and
402-
typeofStr = astNode.getAnOperand().getStringValue()
403-
}
401+
TypeofGuard() { TaintTracking::isTypeofGuard(astNode, operand, tag) }
404402

405403
override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
406-
e = typeof.getOperand() and
404+
e = operand and
407405
outcome = astNode.getPolarity() and
408406
(
409-
typeofStr = "object" and
407+
tag = "object" and
410408
label = "constructor"
411409
or
412-
typeofStr = "function" and
410+
tag = "function" and
413411
label = "__proto__"
414412
)
415413
or
416-
e = typeof.getOperand() and
414+
e = operand and
417415
outcome = astNode.getPolarity().booleanNot() and
418416
(
419417
// If something is not an object, sanitize object, as both must end
420418
// in non-function prototype object.
421-
typeofStr = "object" and
419+
tag = "object" and
422420
label instanceof UnsafePropLabel
423421
or
424-
typeofStr = "function" and
422+
tag = "function" and
425423
label = "constructor"
426424
)
427425
}

‎javascript/ql/src/semmle/javascript/DefensiveProgramming.qll‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -328,12 +328,7 @@ module DefensiveExpressionTest {
328328
Expr operand;
329329
TypeofTag tag;
330330

331-
TypeofTest() {
332-
exists(Expr op1, Expr op2 | hasOperands(op1, op2) |
333-
operand = op1.(TypeofExpr).getOperand() and
334-
op2.mayHaveStringValue(tag)
335-
)
336-
}
331+
TypeofTest() { TaintTracking::isTypeofGuard(this, operand, tag) }
337332

338333
boolean getTheTestResult() {
339334
exists(boolean testResult |

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -899,12 +899,7 @@ module TaintTracking {
899899
Expr x;
900900
override EqualityTest astNode;
901901

902-
TypeOfUndefinedSanitizer() {
903-
exists(StringLiteral str, TypeofExpr typeof | astNode.hasOperands(str, typeof) |
904-
str.getValue() = "undefined" and
905-
typeof.getOperand() = x
906-
)
907-
}
902+
TypeOfUndefinedSanitizer() { isTypeofGuard(astNode, x, "undefined") }
908903

909904
override predicate sanitizes(boolean outcome, Expr e) {
910905
outcome = astNode.getPolarity() and
@@ -914,6 +909,18 @@ module TaintTracking {
914909
override predicate appliesTo(Configuration cfg) { any() }
915910
}
916911

912+
/**
913+
* Holds if `test` is a guard that checks if `operand` is typeof `tag`.
914+
*
915+
* See `TypeOfUndefinedSanitizer` for example usage.
916+
*/
917+
predicate isTypeofGuard(EqualityTest test, Expr operand, TypeofTag tag) {
918+
exists(Expr str, TypeofExpr typeof | test.hasOperands(str, typeof) |
919+
str.mayHaveStringValue(tag) and
920+
typeof.getOperand() = operand
921+
)
922+
}
923+
917924
/** DEPRECATED. This class has been renamed to `MembershipTestSanitizer`. */
918925
deprecated class StringInclusionSanitizer = MembershipTestSanitizer;
919926

‎javascript/ql/src/semmle/javascript/security/TaintedObject.qll‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
*/
1515

1616
import javascript
17+
private import semmle.javascript.dataflow.InferredTypes
1718

1819
module TaintedObject {
1920
private import DataFlow
@@ -98,25 +99,24 @@ module TaintedObject {
9899
*/
99100
private class TypeTestGuard extends SanitizerGuard, ValueNode {
100101
override EqualityTest astNode;
101-
TypeofExpr typeof;
102+
Expr operand;
102103
boolean polarity;
103104

104105
TypeTestGuard() {
105-
astNode.getAnOperand() = typeof and
106-
(
106+
exists(TypeofTag tag | TaintTracking::isTypeofGuard(astNode, operand, tag) |
107107
// typeof x === "object" sanitizes `x` when it evaluates to false
108-
astNode.getAnOperand().getStringValue() = "object" and
108+
tag = "object" and
109109
polarity = astNode.getPolarity().booleanNot()
110110
or
111111
// typeof x === "string" sanitizes `x` when it evaluates to true
112-
astNode.getAnOperand().getStringValue() != "object" and
112+
tag != "object" and
113113
polarity = astNode.getPolarity()
114114
)
115115
}
116116

117117
override predicate sanitizes(boolean outcome, Expr e, FlowLabel label) {
118118
polarity = outcome and
119-
e = typeof.getOperand() and
119+
e = operand and
120120
label = label()
121121
}
122122
}

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

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
private import javascript
1111
private import semmle.javascript.DynamicPropertyAccess
12+
private import semmle.javascript.dataflow.InferredTypes
1213

1314
/**
1415
* Provides a taint tracking configuration for reasoning about
@@ -164,22 +165,18 @@ module PrototypePollutingAssignment {
164165
private class TypeofCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
165166
override EqualityTest astNode;
166167
Expr operand;
167-
string value;
168+
boolean polarity;
168169

169170
TypeofCheck() {
170-
exists(TypeofExpr typeof, Expr str |
171-
astNode.hasOperands(typeof, str) and
172-
typeof.getOperand() = operand and
173-
str.getStringValue() = value
171+
exists(TypeofTag value | TaintTracking::isTypeofGuard(astNode, operand, value) |
172+
value = "object" and polarity = astNode.getPolarity().booleanNot()
173+
or
174+
value != "object" and polarity = astNode.getPolarity()
174175
)
175176
}
176177

177178
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
178-
(
179-
value = "object" and outcome = astNode.getPolarity().booleanNot()
180-
or
181-
value != "object" and outcome = astNode.getPolarity()
182-
) and
179+
polarity = outcome and
183180
e = operand and
184181
label instanceof ObjectPrototype
185182
}

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,7 @@ module UnsafeJQueryPlugin {
134134
SyntacticConstants::isUndefined(undef)
135135
)
136136
or
137-
exists(Expr op1, Expr op2 | test.hasOperands(op1, op2) |
138-
read.asExpr() = op1.(TypeofExpr).getOperand() and
139-
op2.mayHaveStringValue(any(InferredType t | t = TTUndefined()).getTypeofTag())
140-
)
137+
TaintTracking::isTypeofGuard(test, read.asExpr(), "undefined")
141138
)
142139
or
143140
polarity = true and

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import javascript
88
private import semmle.javascript.security.dataflow.RemoteFlowSources
99
private import semmle.javascript.PackageExports as Exports
10+
private import semmle.javascript.dataflow.InferredTypes
1011

1112
/**
1213
* Module containing sources, sinks, and sanitizers for shell command constructed from library input.
@@ -191,4 +192,20 @@ module UnsafeShellCommandConstruction {
191192
)
192193
}
193194
}
195+
196+
/**
197+
* A guard of the form `typeof x === "<T>"`, where <T> is "number", or "boolean",
198+
* which sanitizes `x` in its "then" branch.
199+
*/
200+
class TypeOfSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
201+
Expr x;
202+
override EqualityTest astNode;
203+
204+
TypeOfSanitizer() { TaintTracking::isTypeofGuard(astNode, x, ["number", "boolean"]) }
205+
206+
override predicate sanitizes(boolean outcome, Expr e) {
207+
outcome = astNode.getPolarity() and
208+
e = x
209+
}
210+
}
194211
}

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,16 +98,13 @@ module UnvalidatedDynamicMethodCall {
9898
*/
9999
class FunctionCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
100100
override EqualityTest astNode;
101-
TypeofExpr t;
101+
Expr operand;
102102

103-
FunctionCheck() {
104-
astNode.getAnOperand().getStringValue() = "function" and
105-
astNode.getAnOperand().getUnderlyingValue() = t
106-
}
103+
FunctionCheck() { TaintTracking::isTypeofGuard(astNode, operand, "function") }
107104

108105
override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
109106
outcome = astNode.getPolarity() and
110-
e = t.getOperand().getUnderlyingValue() and
107+
e = operand and
111108
label instanceof MaybeNonFunction
112109
}
113110
}

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55

66
import javascript
7+
private import semmle.javascript.dataflow.InferredTypes
78

89
/**
910
* Classes and predicates for the XSS through DOM query.
@@ -103,25 +104,24 @@ module XssThroughDom {
103104
*/
104105
class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
105106
override EqualityTest astNode;
106-
TypeofExpr typeof;
107+
Expr operand;
107108
boolean polarity;
108109

109110
TypeTestGuard() {
110-
astNode.getAnOperand() = typeof and
111-
(
111+
exists(TypeofTag tag | TaintTracking::isTypeofGuard(astNode, operand, tag) |
112112
// typeof x === "string" sanitizes `x` when it evaluates to false
113-
astNode.getAnOperand().getStringValue() = "string" and
113+
tag = "string" and
114114
polarity = astNode.getPolarity().booleanNot()
115115
or
116116
// typeof x === "object" sanitizes `x` when it evaluates to true
117-
astNode.getAnOperand().getStringValue() != "string" and
117+
tag != "string" and
118118
polarity = astNode.getPolarity()
119119
)
120120
}
121121

122122
override predicate sanitizes(boolean outcome, Expr e) {
123123
polarity = outcome and
124-
e = typeof.getOperand()
124+
e = operand
125125
}
126126
}
127127
}

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,10 @@ nodes
191191
| lib/lib.js:340:22:340:26 | id(n) |
192192
| lib/lib.js:340:22:340:26 | id(n) |
193193
| lib/lib.js:340:25:340:25 | n |
194-
| lib/lib.js:343:29:343:34 | unsafe |
195-
| lib/lib.js:343:29:343:34 | unsafe |
196-
| lib/lib.js:345:22:345:27 | unsafe |
197-
| lib/lib.js:345:22:345:27 | unsafe |
194+
| lib/lib.js:349:29:349:34 | unsafe |
195+
| lib/lib.js:349:29:349:34 | unsafe |
196+
| lib/lib.js:351:22:351:27 | unsafe |
197+
| lib/lib.js:351:22:351:27 | unsafe |
198198
edges
199199
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
200200
| lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name |
@@ -421,10 +421,10 @@ edges
421421
| lib/lib.js:339:39:339:39 | n | lib/lib.js:340:25:340:25 | n |
422422
| lib/lib.js:340:25:340:25 | n | lib/lib.js:340:22:340:26 | id(n) |
423423
| lib/lib.js:340:25:340:25 | n | lib/lib.js:340:22:340:26 | id(n) |
424-
| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
425-
| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
426-
| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
427-
| lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe |
424+
| lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe |
425+
| lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe |
426+
| lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe |
427+
| lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe |
428428
#select
429429
| lib/lib2.js:4:10:4:25 | "rm -rf " + name | lib/lib2.js:3:28:3:31 | name | lib/lib2.js:4:22:4:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:4:10:4:25 | "rm -rf " + name | String concatenation | lib/lib2.js:4:2:4:26 | cp.exec ... + name) | shell command |
430430
| lib/lib2.js:8:10:8:25 | "rm -rf " + name | lib/lib2.js:7:32:7:35 | name | lib/lib2.js:8:22:8:25 | name | $@ based on library input is later used in $@. | lib/lib2.js:8:10:8:25 | "rm -rf " + name | String concatenation | lib/lib2.js:8:2:8:26 | cp.exec ... + name) | shell command |
@@ -480,4 +480,4 @@ edges
480480
| lib/lib.js:320:11:320:26 | "rm -rf " + name | lib/lib.js:314:40:314:43 | name | lib/lib.js:320:23:320:26 | name | $@ based on library input is later used in $@. | lib/lib.js:320:11:320:26 | "rm -rf " + name | String concatenation | lib/lib.js:320:3:320:27 | cp.exec ... + name) | shell command |
481481
| lib/lib.js:325:12:325:51 | "MyWind ... " + arg | lib/lib.js:324:40:324:42 | arg | lib/lib.js:325:49:325:51 | arg | $@ based on library input is later used in $@. | lib/lib.js:325:12:325:51 | "MyWind ... " + arg | String concatenation | lib/lib.js:326:2:326:13 | cp.exec(cmd) | shell command |
482482
| lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | lib/lib.js:339:39:339:39 | n | lib/lib.js:340:22:340:26 | id(n) | $@ based on library input is later used in $@. | lib/lib.js:340:10:340:26 | "rm -rf " + id(n) | String concatenation | lib/lib.js:340:2:340:27 | cp.exec ... id(n)) | shell command |
483-
| lib/lib.js:345:10:345:27 | "rm -rf " + unsafe | lib/lib.js:343:29:343:34 | unsafe | lib/lib.js:345:22:345:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:345:10:345:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:345:2:345:28 | cp.exec ... unsafe) | shell command |
483+
| lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | lib/lib.js:349:29:349:34 | unsafe | lib/lib.js:351:22:351:27 | unsafe | $@ based on library input is later used in $@. | lib/lib.js:351:10:351:27 | "rm -rf " + unsafe | String concatenation | lib/lib.js:351:2:351:28 | cp.exec ... unsafe) | shell command |

0 commit comments

Comments
 (0)