Skip to content

JS: add taint-step for URL construction in js/request-forgery #5630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
codeql-ci merged 1 commit into from
Apr 9, 2021
Merged

JS: add taint-step for URL construction in js/request-forgery #5630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions javascript/ql/src/semmle/javascript/security/dataflow/RequestForgery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,9 @@ module RequestForgery {
override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
sanitizingPrefixEdge(source, sink)
}

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
isAdditionalRequestForgeryStep(pred, succ)
}
}
}
10 changes: 10 additions & 0 deletions javascript/ql/src/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,14 @@ module RequestForgery {

override string getKind() { result = kind }
}

/**
* Holds if there is a taint step from `pred` to `succ` for request forgery.
*/
predicate isAdditionalRequestForgeryStep(DataFlow::Node pred, DataFlow::Node succ) {
exists(DataFlow::NewNode url | url = DataFlow::globalVarRef("URL").getAnInstantiation() |
succ = url and
pred = url.getArgument(0)
)
}
}
12 changes: 12 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ nodes
| tst.js:108:17:108:27 | request.url |
| tst.js:109:27:109:29 | url |
| tst.js:109:27:109:29 | url |
| tst.js:115:11:115:42 | url |
| tst.js:115:17:115:42 | new URL ... , base) |
| tst.js:115:25:115:35 | request.url |
| tst.js:115:25:115:35 | request.url |
| tst.js:117:27:117:29 | url |
| tst.js:117:27:117:29 | url |
edges
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
| tst.js:14:9:14:52 | tainted | tst.js:18:13:18:19 | tainted |
Expand Down Expand Up @@ -161,6 +167,11 @@ edges
| tst.js:108:11:108:27 | url | tst.js:109:27:109:29 | url |
| tst.js:108:17:108:27 | request.url | tst.js:108:11:108:27 | url |
| tst.js:108:17:108:27 | request.url | tst.js:108:11:108:27 | url |
| tst.js:115:11:115:42 | url | tst.js:117:27:117:29 | url |
| tst.js:115:11:115:42 | url | tst.js:117:27:117:29 | url |
| tst.js:115:17:115:42 | new URL ... , base) | tst.js:115:11:115:42 | url |
| tst.js:115:25:115:35 | request.url | tst.js:115:17:115:42 | new URL ... , base) |
| tst.js:115:25:115:35 | request.url | tst.js:115:17:115:42 | new URL ... , base) |
#select
| tst.js:18:5:18:20 | request(tainted) | tst.js:14:29:14:35 | req.url | tst.js:18:13:18:19 | tainted | The $@ of this request depends on $@. | tst.js:18:13:18:19 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
| tst.js:20:5:20:24 | request.get(tainted) | tst.js:14:29:14:35 | req.url | tst.js:20:17:20:23 | tainted | The $@ of this request depends on $@. | tst.js:20:17:20:23 | tainted | URL | tst.js:14:29:14:35 | req.url | a user-provided value |
Expand All @@ -183,3 +194,4 @@ edges
| tst.js:92:5:92:33 | JSDOM.f ... ms.foo) | tst.js:92:19:92:28 | ctx.params | tst.js:92:19:92:32 | ctx.params.foo | The $@ of this request depends on $@. | tst.js:92:19:92:32 | ctx.params.foo | URL | tst.js:92:19:92:28 | ctx.params | a user-provided value |
| tst.js:100:5:100:26 | new Web ... ainted) | tst.js:98:29:98:35 | req.url | tst.js:100:19:100:25 | tainted | The $@ of this request depends on $@. | tst.js:100:19:100:25 | tainted | URL | tst.js:98:29:98:35 | req.url | a user-provided value |
| tst.js:109:20:109:30 | new ws(url) | tst.js:108:17:108:27 | request.url | tst.js:109:27:109:29 | url | The $@ of this request depends on $@. | tst.js:109:27:109:29 | url | URL | tst.js:108:17:108:27 | request.url | a user-provided value |
| tst.js:117:20:117:30 | new ws(url) | tst.js:115:25:115:35 | request.url | tst.js:117:27:117:29 | url | The $@ of this request depends on $@. | tst.js:117:27:117:29 | url | URL | tst.js:115:25:115:35 | request.url | a user-provided value |
8 changes: 8 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-918/tst.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,11 @@ new ws.Server({ port: 8080 }).on('connection', function(socket, request) {
const socket = new ws(url);
});
});

new ws.Server({ port: 8080 }).on('connection', function (socket, request) {
socket.on('message', function (message) {
const url = new URL(request.url, base);

@esbena esbena Apr 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this test and the new QL step. Can you elaborate?

My thoughts:

I agree that request.url is user-controlled, but it will only reach this line if there is a route handler that listens at request.url. So I would treat as "mostly sanitized" if the route handler does not listen on * (it does that in his test though).

I can see that this may be vulnerable after all since the new URL(request.url, base) also changes the host of the URL, causing the "mostly sanitized" url to mean something different to a different host. But that subtlety is not reflected in the QL change that do not check for a second argument.

@erik-krogh erik-krogh Apr 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mostly answered your own question.

So I would treat as "mostly sanitized" if the route handler does not listen on * (it does that in his test though).

Yes, it does sound reasonable to treat such urls as mostly sanitized.
But I think it would take some effort to do, as we would need to match handlers with which urls they listen on.
And I think the url can be tainted even if the handler doesn't listen on *, as long as the handler listens on some "blob".
(E.g. "/foo/bar/*" or "/foo/:bar").

But that subtlety is not reflected in the QL change that do not check for a second argument.

I could add a requirement that the second argument is present (we would still flag the CVE).
In the scenario I looked at the problem was that a certain /priv folder on the target server was supposed to be private (all requests were supposed to go to a /pub folder).

But I also think we should keep the one argument var succ = new URL(pred) step, because it is essentially an identity function (that sometimes throws an exception).

I think we might need some advanced flow-label use if we want to keep track of "how unsafe" a given url is.
(Maybe we could do something similar to the js/tainted-path query).

Yes, we risk that the results might become slightly noisy, but I think it seems safe enough to land.

@esbena esbena Apr 8, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I am inclined to accept the risk of the FPs, @asgerf do you have any additional thoughts on these URL subtleties?

@asgerf asgerf Apr 9, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could get FPs from accessing safe parts of the URL, like new URL(taint).port, but since it only affects the request forgery query I'm fine with it.

const target = new URL(url.pathname, base);
const socket = new ws(url);
});
});