Skip to content

Commit 57a6c0c

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #1918 from esben-semmle/js/improve-getAResponseDataNode
Approved by asger-semmle
2 parents 479fca9 + ac6554b commit 57a6c0c

File tree

14 files changed

+191
-40
lines changed

14 files changed

+191
-40
lines changed

‎change-notes/1.23/analysis-javascript.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@
2424
| Client-side cross-site scripting (`js/xss`) | More results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized. |
2525
| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. |
2626
| Hard-coded credentials (`js/hardcoded-credentials`) | Fewer false-positive results | This rule now flags fewer password examples. |
27+
| Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases.
28+
| Network data written to file (`js/http-to-file-access`) | Fewer false-positive results | This query has been renamed to better match its intended purpose, and now only considers network data untrusted. |
2729
| Password in configuration file (`js/password-in-configuration-file`) | Fewer false-positive results | This rule now flags fewer password examples. |
2830
| Prototype pollution (`js/prototype-pollution`) | More results | The query now highlights vulnerable uses of jQuery and Angular, and the results are shown on LGTM by default. |
29-
| Incorrect suffix check (`js/incorrect-suffix-check`) | Fewer false-positive results | The query recognizes valid checks in more cases. |
31+
| Uncontrolled command line (`js/command-line-injection`) | More results | This query now treats responses from servers as untrusted. |
3032

3133
## Changes to QL libraries
3234

‎javascript/ql/src/Security/CWE-078/CommandInjection.ql‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ import javascript
1616
import semmle.javascript.security.dataflow.CommandInjection::CommandInjection
1717
import DataFlow::PathGraph
1818

19-
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight
19+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node highlight, Source sourceNode
2020
where
2121
cfg.hasFlowPath(source, sink) and
2222
if cfg.isSinkWithHighlight(sink.getNode(), _)
2323
then cfg.isSinkWithHighlight(sink.getNode(), highlight)
24-
else highlight = sink.getNode()
25-
select highlight, source, sink, "This command depends on $@.", source.getNode(),
26-
"a user-provided value"
24+
else highlight = sink.getNode() and
25+
sourceNode = source.getNode()
26+
select highlight, source, sink, "This command depends on $@.", sourceNode, sourceNode.getSourceType()

‎javascript/ql/src/Security/CWE-912/HttpToFileAccess.ql‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
2-
* @name User-controlled data written to file
3-
* @description Writing user-controlled data directly to the file system allows arbitrary file upload and might indicate a backdoor.
2+
* @name Network data written to file
3+
* @description Writing network data directly to the file system allows arbitrary file upload and might indicate a backdoor.
44
* @kind path-problem
55
* @problem.severity warning
66
* @precision medium

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -452,6 +452,8 @@ module ClientRequest {
452452
or
453453
prop = "responseText" and responseType = "text"
454454
or
455+
prop = "responseUrl" and responseType = "text"
456+
or
455457
prop = "statusText" and responseType = "text"
456458
or
457459
prop = "responseXML" and responseType = "document"

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

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -735,34 +735,17 @@ module NodeJSLib {
735735
result = this.(DataFlow::SourceNode).getAMethodCall(name).getArgument(0)
736736
)
737737
}
738-
}
739738

740-
/**
741-
* A data flow node that is the parameter of a result callback for an HTTP or HTTPS request made by a Node.js process, for example `res` in `https.request(url, (res) => {})`.
742-
*/
743-
private class ClientRequestCallbackParam extends DataFlow::ParameterNode, RemoteFlowSource {
744-
ClientRequestCallbackParam() {
745-
exists(NodeJSClientRequest req |
746-
this = req.(DataFlow::MethodCallNode).getCallback(1).getParameter(0)
739+
override DataFlow::Node getAResponseDataNode(string responseType, boolean promise) {
740+
promise = false and
741+
exists(DataFlow::ParameterNode res, DataFlow::CallNode onData |
742+
res = getCallback(1).getParameter(0) and
743+
onData = res.getAMethodCall("on") and
744+
onData.getArgument(0).mayHaveStringValue("data") and
745+
result = onData.getCallback(1).getParameter(0) and
746+
responseType = "arraybuffer"
747747
)
748748
}
749-
750-
override string getSourceType() { result = "NodeJSClientRequest callback parameter" }
751-
}
752-
753-
/**
754-
* A data flow node that is the parameter of a data callback for an HTTP or HTTPS request made by a Node.js process, for example `body` in `http.request(url, (res) => {res.on('data', (body) => {})})`.
755-
*/
756-
private class ClientRequestCallbackData extends RemoteFlowSource {
757-
ClientRequestCallbackData() {
758-
exists(ClientRequestCallbackParam rcp, DataFlow::MethodCallNode mcn |
759-
rcp.getAMethodCall("on") = mcn and
760-
mcn.getArgument(0).mayHaveStringValue("data") and
761-
this = mcn.getCallback(1).getParameter(0)
762-
)
763-
}
764-
765-
override string getSourceType() { result = "http.request data parameter" }
766749
}
767750

768751
/**

‎javascript/ql/src/semmle/javascript/heuristics/AdditionalSources.qll‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,25 @@ private class JSONStringifyAsCommandInjectionSource extends HeuristicSource,
3131
JSONStringifyAsCommandInjectionSource() {
3232
this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
3333
}
34+
35+
override string getSourceType() { result = "a string from JSON.stringify" }
36+
}
37+
38+
/**
39+
* A response from a remote server.
40+
*/
41+
class RemoteServerResponse extends HeuristicSource, RemoteFlowSource {
42+
RemoteServerResponse() {
43+
exists(ClientRequest r |
44+
this = r.getAResponseDataNode() and
45+
not exists(string url, string protocolPattern |
46+
// exclude URLs to the current host
47+
r.getUrl().mayHaveStringValue(url) and
48+
protocolPattern = "(?[a-z+]{3,10}:)" and
49+
not url.regexpMatch(protocolPattern + "?//.*")
50+
)
51+
)
52+
}
53+
54+
override string getSourceType() { result = "a response from a remote server" }
3455
}

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ module CommandInjection {
1111
/**
1212
* A data flow source for command-injection vulnerabilities.
1313
*/
14-
abstract class Source extends DataFlow::Node { }
14+
abstract class Source extends DataFlow::Node {
15+
/** Gets a string that describes the type of this remote flow source. */
16+
abstract string getSourceType();
17+
}
1518

1619
/**
1720
* A data flow sink for command-injection vulnerabilities.
@@ -26,6 +29,17 @@ module CommandInjection {
2629
/** A source of remote user input, considered as a flow source for command injection. */
2730
class RemoteFlowSourceAsSource extends Source {
2831
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
32+
33+
override string getSourceType() { result = "a user-provided value" }
34+
}
35+
36+
/**
37+
* A response from a server, considered as a flow source for command injection.
38+
*/
39+
class ServerResponse extends Source {
40+
ServerResponse() { this = any(ClientRequest r).getAResponseDataNode() }
41+
42+
override string getSourceType() { result = "a server-provided value" }
2943
}
3044

3145
/**

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,22 @@ module HttpToFileAccess {
2424
abstract class Sanitizer extends DataFlow::Node { }
2525

2626
/** A source of remote user input, considered as a flow source for writing user-controlled data to files. */
27-
class RemoteFlowSourceAsSource extends Source {
27+
deprecated class RemoteFlowSourceAsSource extends DataFlow::Node {
2828
RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
2929
}
3030

31+
/**
32+
* An access to a user-controlled HTTP request input, considered as a flow source for writing user-controlled data to files
33+
*/
34+
private class RequestInputAccessAsSource extends Source {
35+
RequestInputAccessAsSource() { this instanceof HTTP::RequestInputAccess }
36+
}
37+
38+
/** A response from a server, considered as a flow source for writing user-controlled data to files. */
39+
private class ServerResponseAsSource extends Source {
40+
ServerResponseAsSource() { this = any(ClientRequest r).getAResponseDataNode() }
41+
}
42+
3143
/** A sink that represents file access method (write, append) argument */
3244
class FileAccessAsSink extends Sink {
3345
FileAccessAsSink() { exists(FileSystemWriteAccess src | this = src.getADataNode()) }

‎javascript/ql/test/library-tests/frameworks/NodeJSLib/tests.expected‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,8 +137,6 @@ test_RemoteFlowSources
137137
| src/http.js:6:26:6:32 | req.url |
138138
| src/http.js:8:3:8:20 | req.headers.cookie |
139139
| src/http.js:9:3:9:17 | req.headers.foo |
140-
| src/http.js:21:33:21:40 | response |
141-
| src/http.js:23:28:23:32 | chunk |
142140
| src/http.js:29:26:29:33 | response |
143141
| src/http.js:30:28:30:32 | chunk |
144142
| src/http.js:40:23:40:30 | authInfo |

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ nodes
6767
| other.js:17:27:17:29 | cmd |
6868
| other.js:18:22:18:24 | cmd |
6969
| other.js:19:36:19:38 | cmd |
70+
| third-party-command-injection.js:5:20:5:26 | command |
71+
| third-party-command-injection.js:6:21:6:27 | command |
7072
edges
7173
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
7274
| child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |
@@ -134,6 +136,7 @@ edges
134136
| other.js:5:15:5:44 | url.par ... ).query | other.js:5:15:5:49 | url.par ... ry.path |
135137
| other.js:5:15:5:49 | url.par ... ry.path | other.js:5:9:5:49 | cmd |
136138
| other.js:5:25:5:31 | req.url | other.js:5:15:5:38 | url.par ... , true) |
139+
| third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command |
137140
#select
138141
| child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
139142
| child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
@@ -160,3 +163,4 @@ edges
160163
| other.js:17:27:17:29 | cmd | other.js:5:25:5:31 | req.url | other.js:17:27:17:29 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
161164
| other.js:18:22:18:24 | cmd | other.js:5:25:5:31 | req.url | other.js:18:22:18:24 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
162165
| other.js:19:36:19:38 | cmd | other.js:5:25:5:31 | req.url | other.js:19:36:19:38 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
166+
| third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command depends on $@. | third-party-command-injection.js:5:20:5:26 | command | a server-provided value |

0 commit comments

Comments
 (0)