Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jan 27, 2021

Makes window.location-based sources part of RemoteFlowSource by introducing ClientSideRemoteFlowSource, with a getKind() predicate indicating it is (part of) the URL, path, query, fragment, or window name. (see qldoc for full details)

It's meant to address a few issues

  1. Using RemoteFlowSource is now a reasonable default taint source for a new query; we don't have to teach users about DOM::locationRef() as well.
  2. The framework models for Angular/React router contributed some remote flow sources for parameters extracted from the path. RemoteFlowSource thus contained some, but not all, client-side URL sources, which was a bit inconsistent.
  3. For some queries we want to treat sources based on the path differently, because they cannot contain ../ sequences (the browser normalizes the path). In particular, request forgery has some FPs where the source is from the path. This PR disables that particular source for request forgery.

There are a few downsides to this approach:

  • new URL(window.location) is not modelled very precisely.
  • Queries that aren't interested in client-side URL sources should now explicitly opt out, and there isn't a particularly pretty way to do so. We could add something like ServerSideRemoteFlowSource but a truly good name for that class eludes me as we can't generally ensure that a given source occurs in a server (e.g. it could be in a library shared between client and server, or it could just be a CLI app that's not a web server).

Evaluation:

@asgerf asgerf added the JS label Jan 27, 2021
@asgerf asgerf force-pushed the js/generalized-remote-flow-source branch 3 times, most recently from 4af453e to 427fffb Compare March 4, 2021 13:26
@asgerf asgerf changed the title JS: Add RemoteFlowSource.isFromClientSideUrl JS: Add ClientSideRemoteFlowSource Mar 8, 2021
@asgerf asgerf force-pushed the js/generalized-remote-flow-source branch 3 times, most recently from 4438d66 to 2dab25f Compare March 12, 2021 17:13
@asgerf
Copy link
Contributor Author

asgerf commented Mar 13, 2021

Evaluation finally looks good 🎉

@asgerf asgerf force-pushed the js/generalized-remote-flow-source branch from 2dab25f to 571fca3 Compare March 13, 2021 07:43
@asgerf asgerf marked this pull request as ready for review March 15, 2021 09:23
@asgerf asgerf requested a review from a team as a code owner March 15, 2021 09:23
@asgerf asgerf added the JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis label Mar 15, 2021
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

LGTM.

Just a comment about helper predicates on RemoteFlowSource.

@asgerf asgerf force-pushed the js/generalized-remote-flow-source branch from 1316d91 to 3922c73 Compare March 16, 2021 13:28
erik-krogh
erik-krogh previously approved these changes Mar 16, 2021
@codeql-ci codeql-ci merged commit efeff6f into github:main Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS:changes-sources-or-sinks Changes taint sources/sinks for the JS analysis JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants