Skip to content

Commit bd51ef3

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #2731 from erik-krogh/CVE527
Approved by esbena
2 parents 931c0e9 + 5ff958a commit bd51ef3

File tree

8 files changed

+107
-17
lines changed

8 files changed

+107
-17
lines changed

‎javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql‎

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -47,25 +47,13 @@ abstract class EnumeratedPropName extends DataFlow::Node {
4747
*/
4848
abstract DataFlow::Node getSourceObject();
4949

50-
/**
51-
* Gets a local reference of the source object.
52-
*/
53-
SourceNode getASourceObjectRef() {
54-
exists(SourceNode root, string path |
55-
getSourceObject() = AccessPath::getAReferenceTo(root, path) and
56-
result = AccessPath::getAReferenceTo(root, path)
57-
)
58-
or
59-
result = getSourceObject().getALocalSource()
60-
}
61-
6250
/**
6351
* Gets a property read that accesses the corresponding property value in the source object.
6452
*
6553
* For example, gets `src[key]` in `for (var key in src) { src[key]; }`.
6654
*/
6755
PropRead getASourceProp() {
68-
result = getASourceObjectRef().getAPropertyRead() and
56+
result = AccessPath::getAnAliasedSourceNode(getSourceObject()).getAPropertyRead() and
6957
result.getPropertyNameExpr().flow().getImmediatePredecessor*() = this
7058
}
7159
}
@@ -125,7 +113,7 @@ class EntriesEnumeratedPropName extends EnumeratedPropName {
125113
* Holds if the properties of `node` are enumerated locally.
126114
*/
127115
predicate arePropertiesEnumerated(DataFlow::SourceNode node) {
128-
node = any(EnumeratedPropName name).getASourceObjectRef()
116+
node = AccessPath::getAnAliasedSourceNode(any(EnumeratedPropName name).getSourceObject())
129117
}
130118

131119
/**

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,4 +412,17 @@ module AccessPath {
412412
isAssignedInUniqueFile(name)
413413
)
414414
}
415+
416+
/**
417+
* Gets a `SourceNode` that refers to the same value or access path as the given node.
418+
*/
419+
pragma[inline]
420+
DataFlow::SourceNode getAnAliasedSourceNode(DataFlow::Node node) {
421+
exists(DataFlow::SourceNode root, string accessPath |
422+
node = AccessPath::getAReferenceTo(root, accessPath) and
423+
result = AccessPath::getAReferenceTo(root, accessPath)
424+
)
425+
or
426+
result = node.getALocalSource()
427+
}
415428
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,10 +165,10 @@ module StringOps {
165165

166166
StartsWith_Substring() {
167167
astNode.hasOperands(call.asExpr(), substring.asExpr()) and
168-
(call.getMethodName() = "substring" or call.getMethodName() = "substr") and
168+
(call.getMethodName() = "substring" or call.getMethodName() = "substr" or call.getMethodName() = "slice") and
169169
call.getNumArgument() = 2 and
170170
(
171-
substring.getALocalSource().getAPropertyRead("length").flowsTo(call.getArgument(1))
171+
AccessPath::getAnAliasedSourceNode(substring).getAPropertyRead("length").flowsTo(call.getArgument(1))
172172
or
173173
substring.getStringValue().length() = call.getArgument(1).asExpr().getIntValue()
174174
)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ module ClientSideUrlRedirect {
5555
// exclude `location.href.split('?')[0]`, which can never refer to the query string
5656
not exists(PropAccess pacc | mce = pacc.getBase() | pacc.getPropertyName() = "0")
5757
or
58-
(methodName = "substring" or methodName = "substr") and
58+
(methodName = "substring" or methodName = "substr" or methodName = "slice") and
5959
// exclude `location.href.substring(0, ...)` and similar, which can
6060
// never refer to the query string
6161
not mce.getArgument(0).(NumberLiteral).getIntValue() = 0

‎javascript/ql/test/library-tests/StringOps/StartsWith/StartsWith.expected‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,5 @@
1414
| tst.js:19:9:19:36 | A.subst ... "web/" | tst.js:19:9:19:9 | A | tst.js:19:31:19:36 | "web/" | true |
1515
| tst.js:32:9:32:32 | strings ... h(A, B) | tst.js:32:28:32:28 | A | tst.js:32:31:32:31 | B | true |
1616
| tst.js:33:9:33:47 | strings ... h(A, B) | tst.js:33:43:33:43 | A | tst.js:33:46:33:46 | B | true |
17+
| tst.js:34:9:34:34 | A.slice ... ) !== B | tst.js:34:9:34:9 | A | tst.js:34:34:34:34 | B | false |
18+
| tst.js:35:9:35:42 | A.slice ... = B.foo | tst.js:35:9:35:9 | A | tst.js:35:38:35:42 | B.foo | false |

‎javascript/ql/test/library-tests/StringOps/StartsWith/tst.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,6 @@ function f(A, B) {
3131

3232
if (strings.startsWith(A, B)) {}
3333
if (strings.caseInsensitiveStartsWith(A, B)) {}
34+
if (A.slice(0, B.length) !== B) {}
35+
if (A.slice(0, B.foo.length) !== B.foo) {}
3436
}

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,6 +1145,34 @@ nodes
11451145
| normalizedPaths.js:228:21:228:24 | path |
11461146
| normalizedPaths.js:228:21:228:24 | path |
11471147
| normalizedPaths.js:228:21:228:24 | path |
1148+
| normalizedPaths.js:236:7:236:47 | path |
1149+
| normalizedPaths.js:236:7:236:47 | path |
1150+
| normalizedPaths.js:236:7:236:47 | path |
1151+
| normalizedPaths.js:236:7:236:47 | path |
1152+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
1153+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
1154+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
1155+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
1156+
| normalizedPaths.js:236:33:236:46 | req.query.path |
1157+
| normalizedPaths.js:236:33:236:46 | req.query.path |
1158+
| normalizedPaths.js:236:33:236:46 | req.query.path |
1159+
| normalizedPaths.js:236:33:236:46 | req.query.path |
1160+
| normalizedPaths.js:236:33:236:46 | req.query.path |
1161+
| normalizedPaths.js:238:19:238:22 | path |
1162+
| normalizedPaths.js:238:19:238:22 | path |
1163+
| normalizedPaths.js:238:19:238:22 | path |
1164+
| normalizedPaths.js:238:19:238:22 | path |
1165+
| normalizedPaths.js:238:19:238:22 | path |
1166+
| normalizedPaths.js:245:21:245:24 | path |
1167+
| normalizedPaths.js:245:21:245:24 | path |
1168+
| normalizedPaths.js:245:21:245:24 | path |
1169+
| normalizedPaths.js:245:21:245:24 | path |
1170+
| normalizedPaths.js:245:21:245:24 | path |
1171+
| normalizedPaths.js:250:21:250:24 | path |
1172+
| normalizedPaths.js:250:21:250:24 | path |
1173+
| normalizedPaths.js:250:21:250:24 | path |
1174+
| normalizedPaths.js:250:21:250:24 | path |
1175+
| normalizedPaths.js:250:21:250:24 | path |
11481176
| tainted-require.js:7:19:7:37 | req.param("module") |
11491177
| tainted-require.js:7:19:7:37 | req.param("module") |
11501178
| tainted-require.js:7:19:7:37 | req.param("module") |
@@ -2903,6 +2931,42 @@ edges
29032931
| normalizedPaths.js:226:35:226:48 | req.query.path | normalizedPaths.js:226:14:226:49 | pathMod ... y.path) |
29042932
| normalizedPaths.js:226:35:226:48 | req.query.path | normalizedPaths.js:226:14:226:49 | pathMod ... y.path) |
29052933
| normalizedPaths.js:226:35:226:48 | req.query.path | normalizedPaths.js:226:14:226:49 | pathMod ... y.path) |
2934+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2935+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2936+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2937+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2938+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2939+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2940+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2941+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:238:19:238:22 | path |
2942+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2943+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2944+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2945+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2946+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2947+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2948+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2949+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:245:21:245:24 | path |
2950+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2951+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2952+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2953+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2954+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2955+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2956+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2957+
| normalizedPaths.js:236:7:236:47 | path | normalizedPaths.js:250:21:250:24 | path |
2958+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) | normalizedPaths.js:236:7:236:47 | path |
2959+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) | normalizedPaths.js:236:7:236:47 | path |
2960+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) | normalizedPaths.js:236:7:236:47 | path |
2961+
| normalizedPaths.js:236:14:236:47 | pathMod ... y.path) | normalizedPaths.js:236:7:236:47 | path |
2962+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2963+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2964+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2965+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2966+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2967+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2968+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
2969+
| normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:236:14:236:47 | pathMod ... y.path) |
29062970
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
29072971
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
29082972
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -3016,6 +3080,9 @@ edges
30163080
| normalizedPaths.js:210:21:210:34 | normalizedPath | normalizedPaths.js:174:14:174:27 | req.query.path | normalizedPaths.js:210:21:210:34 | normalizedPath | This path depends on $@. | normalizedPaths.js:174:14:174:27 | req.query.path | a user-provided value |
30173081
| normalizedPaths.js:222:21:222:24 | path | normalizedPaths.js:214:35:214:48 | req.query.path | normalizedPaths.js:222:21:222:24 | path | This path depends on $@. | normalizedPaths.js:214:35:214:48 | req.query.path | a user-provided value |
30183082
| normalizedPaths.js:228:21:228:24 | path | normalizedPaths.js:226:35:226:48 | req.query.path | normalizedPaths.js:228:21:228:24 | path | This path depends on $@. | normalizedPaths.js:226:35:226:48 | req.query.path | a user-provided value |
3083+
| normalizedPaths.js:238:19:238:22 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:238:19:238:22 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
3084+
| normalizedPaths.js:245:21:245:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:245:21:245:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
3085+
| normalizedPaths.js:250:21:250:24 | path | normalizedPaths.js:236:33:236:46 | req.query.path | normalizedPaths.js:250:21:250:24 | path | This path depends on $@. | normalizedPaths.js:236:33:236:46 | req.query.path | a user-provided value |
30193086
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | This path depends on $@. | tainted-require.js:7:19:7:37 | req.param("module") | a user-provided value |
30203087
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
30213088
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,3 +231,21 @@ app.get('/replace', (req, res) => {
231231
fs.readFileSync(path); // OK
232232
}
233233
});
234+
235+
app.get('/resolve-path', (req, res) => {
236+
let path = pathModule.resolve(req.query.path);
237+
238+
fs.readFileSync(path); // NOT OK
239+
240+
var self = something();
241+
242+
if (path.substring(0, self.dir.length) === self.dir)
243+
fs.readFileSync(path); // OK
244+
else
245+
fs.readFileSync(path); // NOT OK - wrong polarity
246+
247+
if (path.slice(0, self.dir.length) === self.dir)
248+
fs.readFileSync(path); // OK
249+
else
250+
fs.readFileSync(path); // NOT OK - wrong polarity
251+
});

0 commit comments

Comments
 (0)