Skip to content

Commit 785b6d2

Browse files
Merge pull request #230 from advanced-security/jeongsoolee09/address-pathexpr-zipslip-deprecation
Address deprecation of `PathExpr` and port `ZipSlipQuery`
2 parents 0718315 + fb50f23 commit 785b6d2

File tree

7 files changed

+136
-165
lines changed

7 files changed

+136
-165
lines changed

javascript/frameworks/ui5/lib/advanced_security/javascript/frameworks/ui5/UI5.qll

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import advanced_security.javascript.frameworks.ui5.JsonParser
44
import semmle.javascript.security.dataflow.DomBasedXssCustomizations
55
import advanced_security.javascript.frameworks.ui5.UI5View
66
import advanced_security.javascript.frameworks.ui5.UI5HTML
7+
import codeql.util.FileSystem
78

89
private module WebAppResourceRootJsonReader implements JsonParser::MakeJsonReaderSig<WebApp> {
910
class JsonReader extends WebApp {
@@ -33,12 +34,13 @@ private predicate isAnUnResolvedResourceRoot(WebApp webApp, string name, string
3334
)
3435
}
3536

36-
class ResourceRootPathString extends PathString {
37-
WebApp webApp;
38-
39-
ResourceRootPathString() { isAnUnResolvedResourceRoot(webApp, _, this) }
40-
41-
override Folder getARootFolder() { result = webApp.getWebAppFolder() }
37+
private module UI5WebAppResolverConfig implements Folder::ResolveSig {
38+
predicate shouldResolve(Container f, string relativePath) {
39+
exists(WebApp webApp |
40+
f = webApp.getWebAppFolder() and
41+
isAnUnResolvedResourceRoot(webApp, _, relativePath)
42+
)
43+
}
4244
}
4345

4446
class ResourceRoot extends Container {
@@ -48,7 +50,7 @@ class ResourceRoot extends Container {
4850

4951
ResourceRoot() {
5052
isAnUnResolvedResourceRoot(webApp, name, path) and
51-
path.(PathString).resolve(webApp.getWebAppFolder()).getContainer() = this
53+
Folder::Resolve<UI5WebAppResolverConfig>::resolve(webApp.getWebAppFolder(), path) = this
5254
}
5355

5456
string getName() { result = name }
@@ -168,7 +170,9 @@ class SapDefineModule extends AmdModuleDefinition::Range, MethodCallExpr, UserMo
168170
)
169171
}
170172

171-
string getDependency(int i) { result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue() }
173+
string getDependency(int i) {
174+
result = this.(AmdModuleDefinition).getDependencyExpr(i).getStringValue()
175+
}
172176

173177
override string getADependency() { result = this.getDependency(_) }
174178

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: 48 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,68 @@ 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+
postState = "$.util.Zip initialized"
9971
or
100-
end instanceof ZipSlip::Sink
101-
}
72+
/*
73+
* 2. Jump from a domain of a for-in statement to an access of the iteration variable.
74+
* e.g.
75+
* ``` javascript
76+
* for (var x in y) {
77+
* var z = x;
78+
* }
79+
* ```
80+
* This step jumps from `y` to `x` in the body of the for-in loop.
81+
*/
10282

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

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

Lines changed: 0 additions & 44 deletions
This file was deleted.

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"

javascript/frameworks/xsjs/test/models/source/source.expected

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,7 @@
1-
| source.xsjs:42:24:42:54 | webRequ ... uffer() | Remote flow source of type: Remote flow |
2-
| source.xsjs:42:24:42:54 | webRequ ... uffer() | Remote flow source of type: Source node (remote) [from data-extension] |
3-
| source.xsjs:43:24:43:49 | webRequ ... tring() | Remote flow source of type: Remote flow |
4-
| source.xsjs:43:24:43:49 | webRequ ... tring() | Remote flow source of type: Source node (remote) [from data-extension] |
5-
| source.xsjs:44:24:44:53 | webRequ ... quest() | Remote flow source of type: Remote flow |
6-
| source.xsjs:44:24:44:53 | webRequ ... quest() | Remote flow source of type: Source node (remote) [from data-extension] |
7-
| source.xsjs:46:24:46:54 | webRequ ... uffer() | Remote flow source of type: Remote flow |
8-
| source.xsjs:46:24:46:54 | webRequ ... uffer() | Remote flow source of type: Source node (remote) [from data-extension] |
9-
| source.xsjs:47:24:47:49 | webRequ ... tring() | Remote flow source of type: Remote flow |
10-
| source.xsjs:47:24:47:49 | webRequ ... tring() | Remote flow source of type: Source node (remote) [from data-extension] |
11-
| source.xsjs:48:24:48:53 | webRequ ... quest() | Remote flow source of type: Remote flow |
12-
| source.xsjs:48:24:48:53 | webRequ ... quest() | Remote flow source of type: Source node (remote) [from data-extension] |
1+
| source.xsjs:8:23:8:38 | webRequest1.body | Remote flow source of type: Remote flow |
2+
| source.xsjs:8:23:8:38 | webRequest1.body | Remote flow source of type: Source node (remote) [from data-extension] |
3+
| source.xsjs:9:23:9:38 | webRequest2.body | Remote flow source of type: Remote flow |
4+
| source.xsjs:9:23:9:38 | webRequest2.body | Remote flow source of type: Source node (remote) [from data-extension] |
135
| source.xsjs:52:25:52:44 | webRequestParam1.get | Remote flow source of type: Remote flow |
146
| source.xsjs:52:25:52:44 | webRequestParam1.get | Remote flow source of type: Source node (remote) [from data-extension] |
157
| source.xsjs:52:25:52:51 | webRequ ... ("key") | Remote flow source of type: Remote flow |

0 commit comments

Comments
 (0)