Skip to content

Commit 7c4e10d

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#4014 from erik-krogh/stringify
Approved by esbena
2 parents 5874ecc + aab2e6f commit 7c4e10d

File tree

11 files changed

+105
-25
lines changed

11 files changed

+105
-25
lines changed

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# Improvements to JavaScript analysis
2+
3+
## General improvements
4+
5+
* Support for the following frameworks and libraries has been improved:
6+
- [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
7+
- [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
8+
- [javascript-stringify](https://www.npmjs.com/package/javascript-stringify)
9+
- [js-stringify](https://www.npmjs.com/package/js-stringify)
10+
- [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)
11+
- [json-stringify-safe](https://www.npmjs.com/package/json-stringify-safe)
12+
- [json3](https://www.npmjs.com/package/json3)
13+
- [object-inspect](https://www.npmjs.com/package/object-inspect)
14+
- [pretty-format](https://www.npmjs.com/package/pretty-format)
15+
- [stringify-object](https://www.npmjs.com/package/stringify-object)
16+
17+
## New queries
18+
19+
| **Query** | **Tags** | **Purpose** |
20+
|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
21+
22+
23+
## Changes to existing queries
24+
25+
| **Query** | **Expected impact** | **Change** |
26+
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
27+
28+
29+
## Changes to libraries
30+

‎javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ predicate isBackslashEscape(StringReplaceCall mce, DataFlow::RegExpLiteralNode r
6767
*/
6868
predicate allBackslashesEscaped(DataFlow::Node nd) {
6969
// `JSON.stringify` escapes backslashes
70-
nd = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
70+
nd instanceof JsonStringifyCall
7171
or
7272
// check whether `nd` itself escapes backslashes
7373
exists(DataFlow::RegExpLiteralNode rel | isBackslashEscape(nd, rel) |

‎javascript/ql/src/javascript.qll

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import semmle.javascript.InclusionTests
3434
import semmle.javascript.JSDoc
3535
import semmle.javascript.JSON
3636
import semmle.javascript.JsonParsers
37+
import semmle.javascript.JsonStringifiers
3738
import semmle.javascript.JSX
3839
import semmle.javascript.Lines
3940
import semmle.javascript.Locations

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/**
2+
* Provides classes for working with JSON serializers.
3+
*/
4+
5+
import javascript
6+
7+
/**
8+
* A call to a JSON stringifier such as `JSON.stringify` or `require("util").inspect`.
9+
*/
10+
class JsonStringifyCall extends DataFlow::CallNode {
11+
JsonStringifyCall() {
12+
exists(DataFlow::SourceNode callee | this = callee.getACall() |
13+
callee = DataFlow::globalVarRef("JSON").getAPropertyRead("stringify") or
14+
callee = DataFlow::moduleMember("json3", "stringify") or
15+
callee =
16+
DataFlow::moduleImport(["json-stringify-safe", "json-stable-stringify", "stringify-object",
17+
"fast-json-stable-stringify", "fast-safe-stringify", "javascript-stringify",
18+
"js-stringify"]) or
19+
// require("util").inspect() and similar
20+
callee = DataFlow::moduleMember("util", "inspect") or
21+
callee = DataFlow::moduleImport(["pretty-format", "object-inspect"])
22+
)
23+
}
24+
25+
/**
26+
* Gets the data flow node holding the input object to be stringified.
27+
*/
28+
DataFlow::Node getInput() { result = getArgument(0) }
29+
30+
/**
31+
* Gets the data flow node holding the resulting string.
32+
*/
33+
DataFlow::SourceNode getOutput() { result = this }
34+
}

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

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,8 @@ module TaintTracking {
543543
/**
544544
* A taint propagating data flow edge arising from JSON unparsing.
545545
*/
546-
private class JsonStringifyTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode {
547-
JsonStringifyTaintStep() { this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") }
546+
private class JsonStringifyTaintStep extends AdditionalTaintStep, DataFlow::CallNode {
547+
JsonStringifyTaintStep() { this instanceof JsonStringifyCall }
548548

549549
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
550550
pred = getArgument(0) and succ = this
@@ -693,18 +693,6 @@ module TaintTracking {
693693
}
694694
}
695695

696-
/**
697-
* A taint step through the Node.JS function `util.inspect(..)`.
698-
*/
699-
class UtilInspectTaintStep extends AdditionalTaintStep, DataFlow::InvokeNode {
700-
UtilInspectTaintStep() { this = DataFlow::moduleImport("util").getAMemberCall("inspect") }
701-
702-
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
703-
succ = this and
704-
this.getAnArgument() = pred
705-
}
706-
}
707-
708696
private module RegExpCaptureSteps {
709697
/** Gets a reference to a string derived from the most recent RegExp match, such as `RegExp.$1`. */
710698
private DataFlow::PropRead getAStaticCaptureRef() {

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,7 @@ private class RemoteFlowPassword extends HeuristicSource, RemoteFlowSource {
2828
*/
2929
private class JSONStringifyAsCommandInjectionSource extends HeuristicSource,
3030
CommandInjection::Source {
31-
JSONStringifyAsCommandInjectionSource() {
32-
this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
33-
}
31+
JSONStringifyAsCommandInjectionSource() { this instanceof JsonStringifyCall }
3432

3533
override string getSourceType() { result = "a string from JSON.stringify" }
3634
}

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,9 @@ module CleartextLogging {
202202
exists(DataFlow::PropWrite write, DataFlow::PropRead read |
203203
read = write.getRhs()
204204
or
205-
exists(DataFlow::MethodCallNode stringify |
206-
stringify = write.getRhs() and
207-
stringify = DataFlow::globalVarRef("JSON").getAMethodCall("stringify") and
208-
stringify.getArgument(0) = read
205+
exists(JsonStringifyCall stringify |
206+
stringify.getOutput() = write.getRhs() and
207+
stringify.getInput() = read
209208
)
210209
|
211210
not exists(write.getPropertyName()) and

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ module ImproperCodeSanitization {
3636
* A call to `JSON.stringify()` seen as a source for improper code sanitization
3737
*/
3838
class JSONStringifyAsSource extends Source {
39-
JSONStringifyAsSource() { this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") }
39+
JSONStringifyAsSource() { this instanceof JsonStringifyCall }
4040
}
4141

4242
/**

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,7 @@ module PostMessageStar {
4949
exists(DataFlow::InvokeNode toString | toString = trg |
5050
toString.(DataFlow::MethodCallNode).calls(src, "toString")
5151
or
52-
toString = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") and
53-
src = toString.getArgument(0)
52+
src = toString.(JsonStringifyCall).getInput()
5453
) and
5554
inlbl instanceof PartiallyTaintedObject and
5655
outlbl.isTaint()

‎javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,18 @@ typeInferenceMismatch
6868
| exceptions.js:158:13:158:20 | source() | exceptions.js:161:10:161:10 | e |
6969
| importedReactComponent.jsx:4:40:4:47 | source() | exportedReactComponent.jsx:2:10:2:19 | props.text |
7070
| indexOf.js:4:11:4:18 | source() | indexOf.js:9:10:9:10 | x |
71+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:5:8:5:29 | JSON.st ... source) |
72+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:9:8:9:47 | require ... source) |
73+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:10:8:10:42 | require ... source) |
74+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:11:8:11:41 | require ... source) |
75+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:12:8:12:52 | require ... source) |
76+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:13:8:13:45 | require ... source) |
77+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:14:8:14:46 | require ... source) |
78+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:15:8:15:38 | require ... source) |
79+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:16:8:16:38 | require ... source) |
80+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:17:8:17:39 | require ... source) |
81+
| json-stringify.js:2:16:2:23 | source() | json-stringify.js:18:8:18:40 | require ... source) |
82+
| json-stringify.js:3:15:3:22 | source() | json-stringify.js:8:8:8:31 | jsonStr ... (taint) |
7183
| nested-props.js:4:13:4:20 | source() | nested-props.js:5:10:5:14 | obj.x |
7284
| nested-props.js:9:18:9:25 | source() | nested-props.js:10:10:10:16 | obj.x.y |
7385
| nested-props.js:35:13:35:20 | source() | nested-props.js:36:10:36:20 | doLoad(obj) |

0 commit comments

Comments
 (0)