8000 Merge pull request #5555 from asgerf/js/misc-steps · jsutil/codeql@e8d7925 · GitHub
[go: up one dir, main page]
Skip to content

Commit e8d7925

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5555 from asgerf/js/misc-steps
Approved by esbena
2 parents 25e26b9 + 2770a53 commit e8d7925

File tree

13 files changed

+102
-26
lines changed

13 files changed

+102
-26
lines changed

‎javascript/change-notes/2021-03-29-misc-steps.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
lgtm,codescanning
2+
* The `lodash-es` package is now recognized as a variant of `lodash`.
3+
* Taint is now propagated through the `babel.transform` function.
4+
* Improved data flow through React applications using `redux-form` or `react-router`.
5+
* Base64 decoding using the `react-native-base64` package is now recognized.
6+
* An expression of form `o[o.length] = y` is now recognized as appending to an array.

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,8 @@ private class NpmBase64Encode extends Base64::Encode::Range, DataFlow::CallNode
150150
enc = DataFlow::moduleMember("base64url", "toBase64") or
151151
enc = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("encode") or
152152
enc = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("encodeURI") or
153-
enc = DataFlow::moduleMember("urlsafe-base64", "encode")
153+
enc = DataFlow::moduleMember("urlsafe-base64", "encode") or
154+
enc = DataFlow::moduleMember("react-native-base64", ["encode", "encodeFromByteArray"])
154155
|
155156
this = enc.getACall()
156157
)
@@ -186,7 +187,8 @@ private class NpmBase64Decode extends Base64::Decode::Range, DataFlow::CallNode
186187
dec = DataFlow::moduleMember("base64url", "decode") or
187188
dec = DataFlow::moduleMember("base64url", "fromBase64") or
188189
dec = DataFlow::moduleMember("js-base64", "Base64").getAPropertyRead("decode") or
189-
dec = DataFlow::moduleMember("urlsafe-base64", "decode")
190+
dec = DataFlow::moduleMember("urlsafe-base64", "decode") or
191+
dec = DataFlow::moduleMember("react-native-base64", "decode")
190192
|
191193
this = dec.getACall()
192194
)

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

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -584,22 +584,26 @@ module TaintTracking {
584584

585585
/**
586586
* A taint propagating data flow edge for assignments of the form `o[k] = v`, where
587-
* `k` is not a constant and `o` refers to some object literal; in this case, we consider
588-
* taint to flow from `v` to that object literal.
587+
* one of the following holds:
589588
*
590-
* The rationale for this heuristic is that if properties of `o` are accessed by
591-
* computed (that is, non-constant) names, then `o` is most likely being treated as
592-
* a map, not as a real object. In this case, it makes sense to consider the entire
593-
* map to be tainted as soon as one of its entries is.
589+
* - `k` is not a constant and `o` refers to some object literal. The rationale
590+
* here is that `o` is most likely being used like a dictionary object.
591+
*
592+
* - `k` refers to `o.length`, that is, the assignment is of form `o[o.length] = v`.
593+
* In this case, the assignment behaves like `o.push(v)`.
594594
*/
595-
private class DictionaryTaintStep extends SharedTaintStep {
595+
private class ComputedPropWriteTaintStep extends SharedTaintStep {
596596
override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
597-
exists(AssignExpr assgn, IndexExpr idx, DataFlow::ObjectLiteralNode obj |
597+
exists(AssignExpr assgn, IndexExpr idx, DataFlow::SourceNode obj |
598598
assgn.getTarget() = idx and
599599
obj.flowsToExpr(idx.getBase()) and
600600
not exists(idx.getPropertyName()) and
601601
pred = DataFlow::valueNode(assgn.getRhs()) and
602602
succ = obj
603+
|
604+
obj instanceof DataFlow::ObjectLiteralNode
605+
or
606+
obj.getAPropertyRead("length").flowsToExpr(idx.getPropertyNameExpr())
603607
)
604608
}
605609
}

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,4 +188,20 @@ module Babel {
188188
/** Gets the name of the variable used to create JSX elements. */
189189
string getJsxFactoryVariableName() { result = getOption("pragma").(JSONString).getValue() }
190190
}
191+
192+
/**
193+
* A taint step through a call to the Babel `transform` function.
194+
*/
195+
private class TransformTaintStep extends TaintTracking::SharedTaintStep {
196+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
197+
exists(API::CallNode call |
198+
call =
199+
API::moduleImport(["@babel/standalone", "@babel/core"])
200+
.getMember(["transform", "transformSync", "transformAsync"])
201+
.getACall() and
202+
pred = call.getArgument(0) and
203+
succ = [call, call.getParameter(2).getParameter(0).getAnImmediateUse()]
204+
)
205+
}
206+
}
191207
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,9 @@ module LodashUnderscore {
2121
string name;
2222

2323
DefaultMember() {
24-
this = DataFlow::moduleMember("underscore", name)
24+
this = DataFlow::moduleMember(["underscore", "lodash", "lodash-es"], name)
2525
or
26-
this = DataFlow::moduleMember("lodash", name)
27-
or
28-
this = DataFlow::moduleImport("lodash/" + name)
26+
this = DataFlow::moduleImport(["lodash/", "lodash-es/"] + name)
2927
or
3028
this = DataFlow::moduleImport("lodash." + name.toLowerCase()) and isLodashMember(name)
3129
or

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

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,6 @@ private DataFlow::SourceNode getASimplePropertyProjectionCallee(
7575
) {
7676
singleton = false and
7777
(
78-
result = LodashUnderscore::member("pick") and
79-
objectIndex = 0 and
80-
selectorIndex = [1 .. max(result.getACall().getNumArgument())]
81-
or
8278
result = LodashUnderscore::member("pickBy") and
8379
objectIndex = 0 and
8480
selectorIndex = 1
@@ -131,6 +127,19 @@ private class SimplePropertyProjection extends PropertyProjection::Range {
131127
override predicate isSingletonProjection() { singleton = true }
132128
}
133129

130+
/**
131+
* A property projection with a variable number of selector indices.
132+
*/
133+
private class VarArgsPropertyProjection extends PropertyProjection::Range {
134+
VarArgsPropertyProjection() { this = LodashUnderscore::member("pick").getACall() }
135+
136+
override DataFlow::Node getObject() { result = getArgument(0) }
137+
138+
override DataFlow::Node getASelector() { result = getArgument(any(int i | i > 0)) }
139+
140+
override predicate isSingletonProjection() { none() }
141+
}
142+
134143
/**
135144
* A taint step for a property projection.
136145
*/

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -691,15 +691,22 @@ private DataFlow::SourceNode reactRouterDom() {
691691
result = DataFlow::moduleImport("react-router-dom")
692692
}
693693

694+
private DataFlow::SourceNode reactRouterMatchObject() {
695+
result = reactRouterDom().getAMemberCall(["useRouteMatch", "matchPath"])
696+
or
697+
exists(ReactComponent c |
698+
dependedOnByReactRouterClient(c.getTopLevel()) and
699+
result = c.getAPropRead("match")
700+
)
701+
}
702+
694703
private class ReactRouterSource extends ClientSideRemoteFlowSource {
695704
ClientSideRemoteFlowKind kind;
696705

697706
ReactRouterSource() {
698707
this = reactRouterDom().getAMemberCall("useParams") and kind.isPath()
699708
or
700-
exists(string prop |
701-
this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(prop)
702-
|
709+
exists(string prop | this = reactRouterMatchObject().getAPropertyRead(prop) |
703710
prop = "params" and kind.isPath()
704711
or
705712
prop = "url" and kind.isUrl()
@@ -713,16 +720,25 @@ private class ReactRouterSource extends ClientSideRemoteFlowSource {
713720

714721
/**
715722
* Holds if `mod` transitively depends on `react-router-dom`.
716-
*
717-
* We assume any React component in such a file may be used in a context where react-router
718-
* injects the `location` property in its `props` object.
719723
*/
720724
private predicate dependsOnReactRouter(Module mod) {
721725
mod.getAnImport().getImportedPath().getValue() = "react-router-dom"
722726
or
723727
dependsOnReactRouter(mod.getAnImportedModule() F438 )
724728
}
725729

730+
/**
731+
* Holds if `mod` is imported from a module that transitively depends on `react-router-dom`.
732+
*
733+
* We assume any React component in such a file may be used in a context where react-router
734+
* injects the `location` and `match` properties in its `props` object.
735+
*/
736+
private predicate dependedOnByReactRouterClient(Module mod) {
737+
dependsOnReactRouter(mod)
738+
or
739+
dependedOnByReactRouterClient(any(Module m | m.getAnImportedModule() = mod))
740+
}
741+
726742
/**
727743
* A reference to the DOM location obtained through `react-router-dom`
728744
*
@@ -740,7 +756,7 @@ private class ReactRouterLocationSource extends DOM::LocationSource::Range {
740756
this = reactRouterDom().getAMemberCall("useLocation")
741757
or
742758
exists(ReactComponent component |
743-
dependsOnReactRouter(component.getTopLevel()) and
759+
dependedOnByReactRouterClient(component.getTopLevel()) and
744760
this = component.getAPropRead("location")
745761
)
746762
}
@@ -759,6 +775,8 @@ private DataFlow::SourceNode higherOrderComponentBuilder() {
759775
or
760776
result = DataFlow::moduleMember(["react-hot-loader", "react-hot-loader/root"], "hot").getACall()
761777
or
778+
result = DataFlow::moduleMember("redux-form", "reduxForm").getACall()
779+
or
762780
result = reactRouterDom().getAPropertyRead("withRouter")
763781
or
764782
exists(FunctionCompositionCall compose |

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ typeInferenceMismatch
1111
| array-mutation.js:27:16:27:23 | source() | array-mutation.js:28:8:28:8 | g |
1212
| array-mutation.js:31:33:31:40 | source() | array-mutation.js:32:8:32:8 | h |
1313
| array-mutation.js:35:36:35:43 | source() | array-mutation.js:36:8:36:8 | i |
14+
| array-mutation.js:39:17:39:24 | source() | array-mutation.js:40:8:40:8 | j |
1415
| booleanOps.js:2:11:2:18 | source() | booleanOps.js:4:8:4:8 | x |
1516
| booleanOps.js:2:11:2:18 | source() | booleanOps.js:13:10:13:10 | x |
1617
| booleanOps.js:2:11:2:18 | source() | booleanOps.js:19:10:19:10 | x |

‎javascript/ql/test/library-tests/TaintTracking/array-mutation.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,8 @@ function test(x, y) {
3434
let i = [];
3535
Array.prototype.unshift.apply(i, source());
3636
sink(i); // NOT OK
37+
38+
let j = [];
39+
j[j.length] = source();
40+
sink(j); // NOT OK
3741
}

‎javascript/ql/test/library-tests/frameworks/ReactJS/importedComponent.jsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { MyComponent } from "./exportedComponent";
22

3-
export function render(color) {
3+
export function render({color, location}) {
44
return <MyComponent color={color}/>
55
}

0 commit comments

Comments
 (0)
0