Skip to content

Commit 8083f23

Browse files
committed
Rewrite XSJSZipSlip using modern APIs
1 parent 5a63676 commit 8083f23

File tree

4 files changed

+121
-101
lines changed

4 files changed

+121
-101
lines changed

javascript/frameworks/xsjs/ext/xsjs.model.yml

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -35,19 +35,14 @@ extensions:
3535
extensible: sourceModel
3636
data:
3737
# ========== 1. Retrieving Web Request Body ==========
38-
- [WebRequestBody, "Member[asArrayBuffer].ReturnValue", remote]
39-
- [WebRequestBody, "Member[asString].ReturnValue", remote]
40-
- [WebRequestBody, "Member[asWebRequest].ReturnValue", remote]
38+
- [WebRequestBody, "", remote]
4139

4240
# ========== 2. Retrieving Web Request Parameter Value ==========
4341
- [WebRequestParameters, "Member[get].ReturnValue", remote]
4442
- [WebRequestParameters, AnyMember, remote]
4543

4644
# ========== 3. Receiving Response through HTTPClient ==========
4745
- [HTTPClient, "InboundResponse.Member[body]", remote]
48-
- [HTTPClient, "InboundResponse.Member[body].Member[asArrayBuffer].ReturnValue", remote]
49-
- [HTTPClient, "InboundResponse.Member[body].Member[asString].ReturnValue", remote]
50-
- [HTTPClient, "InboundResponse.Member[body].Member[asWebRequest].ReturnValue", remote]
5146
- [HTTPClient, "InboundResponse.Member[cacheControl]", remote]
5247
- [HTTPClient, "InboundResponse.Member[contentType]", remote]
5348
- [HTTPClient, "InboundResponse.Member[cookies]", remote]
@@ -60,12 +55,15 @@ extensions:
6055
extensible: sinkModel
6156
data:
6257
- [WebResponse, "Member[setBody].Argument[0]", html-injection]
63-
# - [Mail, "Member[send].Argument[this]", "???"]
64-
# - [SMTPConnection, "Member[send].Argument[0]", "???"]
65-
# - ["HTTPClient", "Member[request].Argument[0]", "???"]
6658

6759
- addsTo:
6860
pack: codeql/javascript-all
6961
extensible: summaryModel
7062
data:
71-
- [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint]
63+
- [global, "Member[JSON].Member[parse]", "Argument[0]", "ReturnValue", taint]
64+
- [WebRequestBody, "Member[asArrayBuffer]", "Argument[this]", "ReturnValue", taint]
65+
- [WebRequestBody, "Member[asString]", "Argument[this]", "ReturnValue", taint]
66+
- [WebRequestBody, "Member[asWebRequest]", "Argument[this]", "ReturnValue", taint]
67+
- [InboundResponse, "Member[body].Member[asArrayBuffer]", "Argument[this]", "ReturnValue", taint]
68+
- [InboundResponse, "Member[body].Member[asString]", "Argument[this]", "ReturnValue", taint]
69+
- [InboundResponse, "Member[body].Member[asWebRequest]", "Argument[this]", "ReturnValue", taint]

javascript/frameworks/xsjs/lib/advanced_security/javascript/frameworks/xsjs/XSJSZipSlipQuery.qll

Lines changed: 49 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -4,45 +4,15 @@ import semmle.javascript.security.dataflow.ZipSlipQuery as ZipSlip
44
import semmle.javascript.security.dataflow.TaintedPathCustomizations::TaintedPath as TaintedPath
55

66
/**
7-
* An instance of `$.util.Zip`, but the argument to the constructor call is reachable from a remote flow source.
8-
*/
9-
class XSJSZipInstanceDependingOnRemoteFlowSource extends XSJSZipInstance {
10-
RemoteFlowSource remoteArgument;
11-
12-
XSJSZipInstanceDependingOnRemoteFlowSource() {
13-
this.getAnArgument().getALocalSource() = remoteArgument
14-
}
15-
16-
RemoteFlowSource getRemoteArgument() { result = remoteArgument }
17-
}
18-
19-
class XSJSRemoteFlowSourceToZipInstanceStep extends DataFlow::SharedFlowStep {
20-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
21-
exists(XSJSZipInstanceDependingOnRemoteFlowSource dollarUtilZip |
22-
pred = dollarUtilZip.getRemoteArgument() and
23-
succ = dollarUtilZip
24-
)
25-
}
26-
}
27-
28-
class ForInLoopDomainToVariableStep extends DataFlow::SharedFlowStep {
29-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
30-
exists(ForInStmt forLoop |
31-
pred = forLoop.getIterationDomain().flow() and
32-
succ = forLoop.getAnIterationVariable().getAnAccess().flow()
33-
)
34-
}
35-
}
36-
37-
/**
38-
* A node that checks if the path that is being extracted is indeed the prefix of the entry, e.g.
7+
* A node that checks if the path that is being extracted is indeed the prefix of the entry.
8+
* e.g.
399
* ``` javascript
4010
* if (entryPath.indexOf("SomePrefix") === 0) {
4111
* // extract the file with the path.
4212
* }
4313
* ```
4414
*/
45-
class ZipEntryPathIndexOfCallEqualsZeroGuard extends TaintTracking::SanitizerGuardNode {
15+
class ZipEntryPathIndexOfCallEqualsZeroGuard extends DataFlow::Node {
4616
EqualityTest equalityTest;
4717
MethodCallNode indexOfCall;
4818
ForInStmt forLoop;
@@ -55,53 +25,69 @@ class ZipEntryPathIndexOfCallEqualsZeroGuard extends TaintTracking::SanitizerGua
5525
equalityTest.getRightOperand().getIntValue() = 0
5626
}
5727

58-
override predicate sanitizes(boolean outcome, Expr receiver) {
28+
predicate blocksExpr(boolean outcome, Expr receiver) {
5929
exists(DataFlow::Node targetFilePath, DataFlow::Node forLoopVariable |
6030
receiver = targetFilePath.asExpr() and
6131
targetFilePath = indexOfCall.getReceiver() and
6232
forLoopVariable = forLoop.getAnIterationVariable().getAnAccess().flow() and
63-
TaintedPath::isAdditionalTaintedPathFlowStep(forLoopVariable,
64-
targetFilePath.getALocalSource(), _, _) and
33+
TaintedPath::isAdditionalFlowStep(forLoopVariable, _, targetFilePath.getALocalSource(), _) and
6534
outcome = equalityTest.getPolarity()
6635
)
6736
}
6837
}
6938

70-
/**
71-
* A class wraps `TaintedPath::BarrierGuardNode` by delegating its `sanitizes/0` to the `blocks/0` predicate.
72-
* The characteristic predicate of this class is deliberately left out.
73-
*/
74-
class TaintedPathSanitizerGuard extends TaintTracking::SanitizerGuardNode {
75-
TaintedPathSanitizerGuard() { this = this }
76-
77-
override predicate sanitizes(boolean outcome, Expr receiver) {
78-
exists(TaintedPath::BarrierGuard node | node.blocksExpr(outcome, receiver))
39+
module XSJSZipSlip implements DataFlow::StateConfigSig {
40+
class FlowState extends string {
41+
FlowState() { this in ["$.util.Zip uninitialized", "$.util.Zip initialized"] }
7942
}
80-
}
8143

82-
class Configuration extends TaintTracking::Configuration {
83-
Configuration() { this = "XSJS Zip Slip Query" }
44+
predicate isSource(DataFlow::Node node, FlowState state) {
45+
node instanceof RemoteFlowSource and
46+
state = "$.util.Zip uninitialized"
47+
}
8448

85-
override predicate isSource(DataFlow::Node start) {
86-
super.isSource(start)
87-
or
88-
exists(XSJSZipInstanceDependingOnRemoteFlowSource dollarUtilZip |
89-
start = dollarUtilZip.getRemoteArgument()
90-
)
49+
predicate isSink(DataFlow::Node node, FlowState state) {
50+
node instanceof ZipSlip::Sink and
51+
state = "$.util.Zip initialized"
9152
}
9253

93-
override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node dst) {
94-
TaintedPath::isAdditionalTaintedPathFlowStep(src, dst, _, _)
54+
predicate isBarrier(DataFlow::Node node, FlowState state) {
55+
(
56+
node = DataFlow::MakeBarrierGuard<ZipEntryPathIndexOfCallEqualsZeroGuard>::getABarrierNode()
57+
or
58+
node = DataFlow::MakeBarrierGuard<TaintedPath::BarrierGuard>::getABarrierNode()
59+
) and
60+
state = state
9561
}
9662

97-
override predicate isSink(DataFlow::Node end) {
98-
super.isSink(end)
63+
predicate isAdditionalFlowStep(
64+
DataFlow::Node start, FlowState preState, DataFlow::Node end, FlowState postState
65+
) {
66+
/* 1. `$.util.Zip` initialized */
67+
start = start and
68+
preState = "$.util.Zip uninitialized" and
69+
end instanceof XSJSZipInstance and
70+
// start.getBasicBlock().getASuccessor+() = end.getBasicBlock() and
71+
postState = "$.util.Zip initialized"
9972
or
100-
end instanceof ZipSlip::Sink
101-
}
73+
/*
74+
* 2. Jump from a domain of a for-in statement to an access of the iteration variable.
75+
* e.g.
76+
* ``` javascript
77+
* for (var x in y) {
78+
* var z = x;
79+
* }
80+
* ```
81+
* This step jumps from `y` to `x` in the body of the for-in loop.
82+
*/
10283

103-
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
104-
node instanceof ZipEntryPathIndexOfCallEqualsZeroGuard or
105-
node instanceof TaintedPathSanitizerGuard
84+
exists(ForInStmt forLoop |
85+
start = forLoop.getIterationDomain().flow() and
86+
end = forLoop.getAnIterationVariable().getAnAccess().flow() and
87+
preState = postState
88+
)
89+
or
90+
TaintedPath::isAdditionalFlowStep(start, _, end, _) and
91+
preState = postState
10692
}
10793
}

javascript/frameworks/xsjs/src/XSJSZipSlip/XSJSZipSlip.ql

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,13 @@
1212

1313
import javascript
1414
import advanced_security.javascript.frameworks.xsjs.XSJSZipSlipQuery
15-
import DataFlow::PathGraph
15+
import semmle.javascript.frameworks.data.ModelsAsData
1616

17-
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
18-
where config.hasFlowPath(source, sink)
19-
select sink, source, sink, "The path of $@ being saved depends on a $@.", sink, "this zip file", source, "user-provided value"
17+
module XSJSZipSlipFlow = DataFlow::GlobalWithState<XSJSZipSlip>;
18+
19+
import XSJSZipSlipFlow::PathGraph
20+
21+
from XSJSZipSlipFlow::PathNode source, XSJSZipSlipFlow::PathNode sink
22+
where XSJSZipSlipFlow::flowPath(source, sink)
23+
select sink, source, sink, "The path of $@ being saved depends on a $@.", sink, "this zip file",
24+
source, "user-provided value"
Lines changed: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,57 @@
1-
WARNING: module 'PathGraph' has been deprecated and may be removed in future (XSJSZipSlip.ql:15,8-27)
2-
WARNING: type 'PathNode' has been deprecated and may be removed in future (XSJSZipSlip.ql:17,28-46)
3-
WARNING: type 'PathNode' has been deprecated and may be removed in future (XSJSZipSlip.ql:17,55-73)
4-
nodes
5-
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive |
6-
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) |
7-
| XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() |
8-
| XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() |
9-
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive |
10-
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath |
11-
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) |
12-
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath |
13-
| XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath |
14-
| XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath |
151
edges
16-
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | XSJSZipSlip.xsjs:10:25:10:34 | zipArchive |
17-
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | XSJSZipSlip.xsjs:7:7:7:62 | zipArchive |
18-
| XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) |
19-
| XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) |
20-
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | XSJSZipSlip.xsjs:11:65:11:73 | entryPath |
21-
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath |
22-
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath |
23-
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath |
24-
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath | XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) |
2+
| XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
3+
| XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | XSJSZipSlip.xsjs:31:7:31:17 | requestBody | provenance | |
4+
| XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | XSJSZipSlip.xsjs:32:7:32:17 | requestBody | provenance | |
5+
| XSJSZipSlip.xsjs:6:16:6:26 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
6+
| XSJSZipSlip.xsjs:6:16:6:26 | requestBody | XSJSZipSlip.xsjs:7:35:7:45 | requestBody | provenance | |
7+
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | provenance | |
8+
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | provenance | |
9+
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | provenance | |
10+
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | provenance | |
11+
| XSJSZipSlip.xsjs:7:35:7:45 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
12+
| XSJSZipSlip.xsjs:7:35:7:45 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
13+
| XSJSZipSlip.xsjs:7:35:7:45 | requestBody | XSJSZipSlip.xsjs:7:35:7:59 | request ... yBuffer | provenance | Config |
14+
| XSJSZipSlip.xsjs:7:35:7:59 | request ... yBuffer | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
15+
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | XSJSZipSlip.xsjs:11:65:11:73 | entryPath | provenance | Config |
16+
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | XSJSZipSlip.xsjs:11:65:11:73 | entryPath | provenance | Config |
17+
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | provenance | |
18+
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | provenance | |
19+
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | provenance | |
20+
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | provenance | |
21+
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath | XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | provenance | Config |
22+
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath | XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | provenance | Config |
23+
| XSJSZipSlip.xsjs:19:16:19:26 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
24+
| XSJSZipSlip.xsjs:19:16:19:26 | requestBody | XSJSZipSlip.xsjs:20:35:20:45 | requestBody | provenance | |
25+
| XSJSZipSlip.xsjs:20:35:20:45 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
26+
| XSJSZipSlip.xsjs:20:35:20:45 | requestBody | XSJSZipSlip.xsjs:20:35:20:59 | request ... yBuffer | provenance | Config |
27+
| XSJSZipSlip.xsjs:20:35:20:59 | request ... yBuffer | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
28+
| XSJSZipSlip.xsjs:31:7:31:17 | requestBody | XSJSZipSlip.xsjs:6:16:6:26 | requestBody | provenance | |
29+
| XSJSZipSlip.xsjs:31:7:31:17 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
30+
| XSJSZipSlip.xsjs:32:7:32:17 | requestBody | XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | provenance | Config |
31+
| XSJSZipSlip.xsjs:32:7:32:17 | requestBody | XSJSZipSlip.xsjs:19:16:19:26 | requestBody | provenance | |
32+
nodes
33+
| XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | semmle.label | $.request.body |
34+
| XSJSZipSlip.xsjs:6:16:6:26 | requestBody | semmle.label | requestBody |
35+
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | semmle.label | zipArchive |
36+
| XSJSZipSlip.xsjs:7:7:7:62 | zipArchive | semmle.label | zipArchive |
37+
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | semmle.label | new $.u ... ffer()) |
38+
| XSJSZipSlip.xsjs:7:20:7:62 | new $.u ... ffer()) | semmle.label | new $.u ... ffer()) |
39+
| XSJSZipSlip.xsjs:7:35:7:45 | requestBody | semmle.label | requestBody |
40+
| XSJSZipSlip.xsjs:7:35:7:59 | request ... yBuffer | semmle.label | request ... yBuffer |
41+
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | semmle.label | zipArchive |
42+
| XSJSZipSlip.xsjs:10:25:10:34 | zipArchive | semmle.label | zipArchive |
43+
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | semmle.label | targetFilePath |
44+
| XSJSZipSlip.xsjs:11:9:11:74 | targetFilePath | semmle.label | targetFilePath |
45+
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | semmle.label | require ... ryPath) |
46+
| XSJSZipSlip.xsjs:11:26:11:74 | require ... ryPath) | semmle.label | require ... ryPath) |
47+
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath | semmle.label | entryPath |
48+
| XSJSZipSlip.xsjs:11:65:11:73 | entryPath | semmle.label | entryPath |
49+
| XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | semmle.label | targetFilePath |
50+
| XSJSZipSlip.xsjs:19:16:19:26 | requestBody | semmle.label | requestBody |
51+
| XSJSZipSlip.xsjs:20:35:20:45 | requestBody | semmle.label | requestBody |
52+
| XSJSZipSlip.xsjs:20:35:20:59 | request ... yBuffer | semmle.label | request ... yBuffer |
53+
| XSJSZipSlip.xsjs:31:7:31:17 | requestBody | semmle.label | requestBody |
54+
| XSJSZipSlip.xsjs:32:7:32:17 | requestBody | semmle.label | requestBody |
55+
subpaths
2556
#select
26-
| XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | The path of $@ being saved depends on a $@. | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | this zip file | XSJSZipSlip.xsjs:7:35:7:61 | request ... uffer() | user-provided value |
57+
| XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | The path of $@ being saved depends on a $@. | XSJSZipSlip.xsjs:12:37:12:50 | targetFilePath | this zip file | XSJSZipSlip.xsjs:1:19:1:32 | $.request.body | user-provided value |

0 commit comments

Comments
 (0)