Skip to content

Conversation

@max-schaefer
Copy link
Contributor

@max-schaefer max-schaefer commented Dec 9, 2020

Fixes #4785.

With this PR, you can add one or more JSON files named codeql-javascript-remote-flow-sources.json to your database (either by including them somewhere in the source tree or by extracting and importing them separately) to specify additional remote flow sources, which will give rise to new RemoteFlowSources at the QL level.

As an example of the syntax, this JSON file

{
    "user input": [
        "window.user.name",
        "window.user.address",
        "window.dob"
    ],
    "uncontrolled path": [
        "window.upload"
    ]
}

marks window.user.name, window.user.address, window.dob, and window.upload as remote flow sources, the former three with source type "user input", the last one with source type "uncontrolled path".

We use inter-procedural data flow to identify accesses to the new remote flow sources, so a property access of the form getWindow().upload where getWindow() returns a reference to the global object is also recognised as a remote flow source.

@max-schaefer
Copy link
Contributor Author

I haven't evaluated performance or written proper documentation yet, but putting this up anyway to facilitate discussion. (@asgerf)

@muglug, would this format work for you?

The one thing that's currently missing is restricting the scope of a remote flow source to a single source file. I wasn't sure we want that, since the sources are (at the moment anyway) global variables anyway. Do you have specific use cases where you want different sets of (global) flow sources for different files?

@muglug
Copy link

muglug commented Dec 10, 2020

That format would work great!

@muglug
Copy link

muglug commented Dec 10, 2020

Is there also a way to list TypeScript types as sources?

e.g.

export type TeamSettings = {
  teamName: string;  // RemoteFlowSource: user input
  logoUrl: string;
  accentColor: string;
};

These types are typically mapped to JSON requests inside application code.

@max-schaefer
Copy link
Contributor Author

Ah, good question. I think that should be possible, but may need a few more extensions.

@max-schaefer max-schaefer force-pushed the js/external-remote-flow-sources branch from fd1f932 to be35e85 Compare December 12, 2020 11:43
@max-schaefer
Copy link
Contributor Author

Performance looks unproblematic (internal link).

I'd suggest we tackle the extension to TypeScript in a separate PR.

Still need to add documentation in the appropriate places, though.

@max-schaefer max-schaefer marked this pull request as ready for review December 14, 2020 08:49
@max-schaefer max-schaefer requested a review from a team as a code owner December 14, 2020 08:49
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Implementation LGTM. 👍

@max-schaefer
Copy link
Contributor Author

I have added a brief write-up in docs/codeql/codeql-language-guides.

@mchammer01, could you take a look? (And let me know if this isn't the right location or the right format.)

@mchammer01
Copy link
Contributor

Hi @max-schaefer 👋🏻 - Unfortunately, I don't have time to look at this (heads down with documentation work for GHES 3.0) but someone else in the team will review this for you, hopefully shortly 🙂

@jf205
Copy link
Contributor

jf205 commented Dec 15, 2020

I have added a brief write-up in docs/codeql/codeql-language-guides.

@mchammer01, could you take a look? (And let me know if this isn't the right location or the right format.)

Hey @max-schaefer

MC is heads down on GitHub docs so I'll take a look at this for you

@jf205
Copy link
Contributor

jf205 commented Dec 15, 2020

I've just pushed a commit that adds a short introduction to the article, which is also used in the table of contents on the CodeQL for JavaScript landing page. Please feel free to tweak the wording if it's not accurate. I'll review the rest of the text shortly.

@sj
Copy link
Collaborator

sj commented Dec 15, 2020

Awesome work team! Let's ship this as a beta for the CodeQL JavaScript/TypeScript analysis and learn from how people use it. We'll then consider whether (and how) we can integrate this into the CodeQL analysis for other languages.

@jf205 / @max-schaefer: could you make sure to include the "beta" label in the documentation please?

Thanks all!

jf205
jf205 previously approved these changes Dec 15, 2020
Copy link
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

I've added a beta note and reviewed the rest of the text. Looks pretty much good to go 👍🏻

Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
@max-schaefer
Copy link
Contributor Author

Thanks, @jf205!

We may need to iterate a bit more on

You can model potential sources of untrusted user input in external files

I think this could be misinterpreted as referring to untrusted input coming from external files, which of course isn't what this is about.

Copy link
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

Good point. Here's another attempt.

Co-authored-by: James Fletcher <42464962+jf205@users.noreply.github.com>
@max-schaefer
Copy link
Contributor Author

Perfect, thanks!

Copy link
Contributor

@jf205 jf205 left a comment

Choose a reason for hiding this comment

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

Docs LGTM!

@codeql-ci codeql-ci merged commit 9ae8880 into github:main Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Interchange format for listing JavaScript sinks

7 participants