Skip to content

Commit 02578cf

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5112 from erik-krogh/forms
Approved by asgerf
2 parents b749112 + 91f7d33 commit 02578cf

File tree

6 files changed

+411
-85
lines changed

6 files changed

+411
-85
lines changed

‎javascript/change-notes/2021-02-08-xss-through-dom-forms.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
lgtm,codescanning
2+
* The `js/xss-through-dom` query now recognizes form inputs as sources.
3+
Affected packages are
4+
[formik](https://www.npmjs.com/package/formik) and
5+
[react-final-form](https://www.npmjs.com/package/react-final-form) and
6+
[react-hook-form](https://www.npmjs.com/package/react-hook-form)

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,25 @@ module DOM {
357357
}
358358
}
359359

360+
/**
361+
* Gets a reference to a DOM event.
362+
*/
363+
private DataFlow::SourceNode domEventSource() {
364+
// e.g. <form onSubmit={e => e.target}/>
365+
exists(JSXAttribute attr | attr.getName().matches("on%") |
366+
result = attr.getValue().flow().getABoundFunctionValue(0).getParameter(0)
367+
)
368+
or
369+
// node.addEventListener("submit", e => e.target)
370+
result = domValueRef().getAMethodCall("addEventListener").getABoundCallbackParameter(1, 0)
371+
or
372+
// node.onSubmit = (e => e.target);
373+
exists(DataFlow::PropWrite write | write = domValueRef().getAPropertyWrite() |
374+
write.getPropertyName().matches("on%") and
375+
result = write.getRhs().getAFunctionValue().getParameter(0)
376+
)
377+
}
378+
360379
/** Gets a data flow node that refers directly to a value from the DOM. */
361380
DataFlow::SourceNode domValueSource() { result instanceof DomValueSource::Range }
362381

@@ -368,6 +387,9 @@ module DOM {
368387
t.start() and
369388
result = domValueRef().getAMethodCall(["item", "namedItem"])
370389
or
390+
t.startInProp("target") and
391+
result = domEventSource()
392+
or
371393
exists(DataFlow::TypeTracker t2 | result = domValueRef(t2).track(t2, t))
372394
}
373395

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

Lines changed: 1 addition & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ private import semmle.javascript.dataflow.InferredTypes
1111
*/
1212
module XssThroughDom {
1313
import Xss::XssThroughDom
14+
private import XssThroughDomCustomizations::XssThroughDom
1415
private import semmle.javascript.security.dataflow.Xss::DomBasedXss as DomBasedXss
15-
private import semmle.javascript.dataflow.InferredTypes
1616
private import semmle.javascript.security.dataflow.UnsafeJQueryPluginCustomizations::UnsafeJQueryPlugin as UnsafeJQuery
1717

1818
/**
@@ -40,88 +40,4 @@ module XssThroughDom {
4040
DomBasedXss::isOptionallySanitizedEdge(pred, succ)
4141
}
4242
}
43-
44-
/**
45-
* Gets an attribute name that could store user-controlled data.
46-
*
47-
* Attributes such as "id", "href", and "src" are often used as input to HTML.
48-
* However, they are either rarely controlable by a user, or already a sink for other XSS vulnerabilities.
49-
* Such attributes are therefore ignored.
50-
*/
51-
bindingset[result]
52-
string unsafeAttributeName() {
53-
result.regexpMatch("data-.*") or
54-
result.regexpMatch("aria-.*") or
55-
result = ["name", "value", "title", "alt"]
56-
}
57-
58-
/**
59-
* A source for text from the DOM from a JQuery method call.
60-
*/
61-
class JQueryTextSource extends Source, JQuery::MethodCall {
62-
JQueryTextSource() {
63-
(
64-
this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
65-
or
66-
this.getMethodName() = "attr" and
67-
this.getNumArgument() = 1 and
68-
forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
69-
this.getArgument(0).mayHaveStringValue(unsafeAttributeName())
70-
) and
71-
// looks like a $("<p>" + ... ) source, which is benign for this query.
72-
not exists(DataFlow::Node prefix |
73-
DomBasedXss::isPrefixOfJQueryHtmlString(this.getReceiver()
74-
.(DataFlow::CallNode)
75-
.getAnArgument(), prefix)
76-
|
77-
prefix.getStringValue().regexpMatch("\\s*<.*")
78-
)
79-
}
80-
}
81-
82-
/**
83-
* A source for text from the DOM from a DOM property read or call to `getAttribute()`.
84-
*/
85-
class DOMTextSource extends Source {
86-
DOMTextSource() {
87-
exists(DataFlow::PropRead read | read = this |
88-
read.getBase().getALocalSource() = DOM::domValueRef() and
89-
read.mayHavePropertyName(["innerText", "textContent", "value", "name"])
90-
)
91-
or
92-
exists(DataFlow::MethodCallNode mcn | mcn = this |
93-
mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
94-
mcn.getMethodName() = "getAttribute" and
95-
mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
96-
)
97-
}
98-
}
99-
100-
/**
101-
* A test of form `typeof x === "something"`, preventing `x` from being a string in some cases.
102-
*
103-
* This sanitizer helps prune infeasible paths in type-overloaded functions.
104-
*/
105-
class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
106-
override EqualityTest astNode;
107-
Expr operand;
108-
boolean polarity;
109-
110-
TypeTestGuard() {
111-
exists(TypeofTag tag | TaintTracking::isTypeofGuard(astNode, operand, tag) |
112-
// typeof x === "string" sanitizes `x` when it evaluates to false
113-
tag = "string" and
114-
polarity = astNode.getPolarity().booleanNot()
115-
or
116-
// typeof x === "object" sanitizes `x` when it evaluates to true
117-
tag != "string" and
118-
polarity = astNode.getPolarity()
119-
)
120-
}
121-
122-
override predicate sanitizes(boolean outcome, Expr e) {
123-
polarity = outcome and
124-
e = operand
125-
}
126-
}
12743
}

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

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
/**
2+
* Provides default sources for reasoning about
3+
* cross-site scripting vulnerabilities through the DOM.
4+
*/
5+
6+
import javascript
7+
8+
/**
9+
* Sources for cross-site scripting vulnerabilities through the DOM.
10+
*/
11+
module XssThroughDom {
12+
import Xss::XssThroughDom
13+
private import semmle.javascript.dataflow.InferredTypes
14+
private import semmle.javascript.security.dataflow.Xss::DomBasedXss as DomBasedXss
15+
16+
/**
17+
* Gets an attribute name that could store user-controlled data.
18+
*
19+
* Attributes such as "id", "href", and "src" are often used as input to HTML.
20+
* However, they are either rarely controlable by a user, or already a sink for other XSS vulnerabilities.
21+
* Such attributes are therefore ignored.
22+
*/
23+
bindingset[result]
24+
string unsafeAttributeName() {
25+
result.regexpMatch("data-.*") or
26+
result.regexpMatch("aria-.*") or
27+
result = ["name", "value", "title", "alt"]
28+
}
29+
30+
/**
31+
* A source for text from the DOM from a JQuery method call.
32+
*/
33+
class JQueryTextSource extends Source, JQuery::MethodCall {
34+
JQueryTextSource() {
35+
(
36+
this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
37+
or
38+
this.getMethodName() = "attr" and
39+
this.getNumArgument() = 1 and
40+
forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
41+
this.getArgument(0).mayHaveStringValue(unsafeAttributeName())
42+
) and
43+
// looks like a $("<p>" + ... ) source, which is benign for this query.
44+
not exists(DataFlow::Node prefix |
45+
DomBasedXss::isPrefixOfJQueryHtmlString(this.getReceiver()
46+
.(DataFlow::CallNode)
47+
.getAnArgument(), prefix)
48+
|
49+
prefix.getStringValue().regexpMatch("\\s*<.*")
50+
)
51+
}
52+
}
53+
54+
/**
55+
* A source for text from the DOM from a DOM property read or call to `getAttribute()`.
56+
*/
57+
class DOMTextSource extends Source {
58+
DOMTextSource() {
59+
exists(DataFlow::PropRead read | read = this |
60+
read.getBase().getALocalSource() = DOM::domValueRef() and
61+
read.mayHavePropertyName(["innerText", "textContent", "value", "name"])
62+
)
63+
or
64+
exists(DataFlow::MethodCallNode mcn | mcn = this |
65+
mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
66+
mcn.getMethodName() = "getAttribute" and
67+
mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
68+
)
69+
}
70+
}
71+
72+
/**
73+
* A test of form `typeof x === "something"`, preventing `x` from being a string in some cases.
74+
*
75+
* This sanitizer helps prune infeasible paths in type-overloaded functions.
76+
*/
77+
class TypeTestGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
78+
override EqualityTest astNode;
79+
Expr operand;
80+
boolean polarity;
81+
82+
TypeTestGuard() {
83+
exists(TypeofTag tag | TaintTracking::isTypeofGuard(astNode, operand, tag) |
84+
// typeof x === "string" sanitizes `x` when it evaluates to false
85+
tag = "string" and
86+
polarity = astNode.getPolarity().booleanNot()
87+
or
88+
// typeof x === "object" sanitizes `x` when it evaluates to true
89+
tag != "string" and
90+
polarity = astNode.getPolarity()
91+
)
92+
}
93+
94+
override predicate sanitizes(boolean outcome, Expr e) {
95+
polarity = outcome and
96+
e = operand
97+
}
98+
}
99+
100+
/**
101+
* A module for form inputs seen as sources for xss-through-dom.
102+
*/
103+
module Forms {
104+
/**
105+
* A reference to an import of `Formik`.
106+
*/
107+
private DataFlow::SourceNode formik() {
108+
result = DataFlow::moduleImport("formik")
109+
or
110+
result = DataFlow::globalVarRef("Formik")
111+
}
112+
113+
/**
114+
* An object containing input values from a form build with `Formik`.
115+
*/
116+
class FormikSource extends Source {
117+
FormikSource() {
118+
exists(JSXElement elem |
119+
formik().getAPropertyRead("Formik").flowsToExpr(elem.getNameExpr())
120+
|
121+
this =
122+
elem.getAttributeByName(["validate", "onSubmit"])
123+
.getValue()
124+
.flow()
125+
.getAFunctionValue()
126+
.getParameter(0)
127+
)
128+
or
129+
this =
130+
formik()
131+
.getAMemberCall("withFormik")
132+
.getOptionArgument(0, ["validate", "handleSubmit"])
133+
.getAFunctionValue()
134+
.getParameter(0)
135+
or
136+
this = formik().getAMemberCall("useFormikContext").getAPropertyRead("values")
137+
}
138+
}
139+
140+
/**
141+
* An object containing input values from a form build with `react-final-form`.
142+
*/
143+
class ReactFinalFormSource extends Source {
144+
ReactFinalFormSource() {
145+
exists(JSXElement elem |
146+
DataFlow::moduleMember("react-final-form", "Form").flowsToExpr(elem.getNameExpr())
147+
|
148+
this =
149+
elem.getAttributeByName("onSubmit")
150+
.getValue()
151+
.flow()
152+
.getAFunctionValue()
153+
.getParameter(0)
154+
)
155+
}
156+
}
157+
158+
/**
159+
* An object containing input values from a form build with `react-hook-form`.
160+
*/
161+
class ReactHookFormSource extends Source {
162+
ReactHookFormSource() {
163+
exists(API::Node useForm |
164+
useForm = API::moduleImport("react-hook-form").getMember("useForm").getReturn()
165+
|
166+
this =
167+
useForm.getMember("handleSubmit").getParameter(0).getParameter(0).getAnImmediateUse()
168+
or
169+
this = useForm.getMember("getValues").getACall()
170+
)
171+
}
172+
}
173+
}
174+
}

0 commit comments

Comments
 (0)