Skip to content

JS: Functionality from untrusted sources query (CWE-830) #8014

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.

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
merged 17 commits into from
Feb 23, 2022
Merged

JS: Functionality from untrusted sources query (CWE-830) #8014

merged 17 commits into from
Feb 23, 2022

Conversation

kaeluka
Copy link

@kaeluka kaeluka commented Feb 14, 2022

This adds a query that finds HTML <script src="http://somepage.com/foo.js"></script> elements where the src is only a http URL (not https) and there's no integrity checking. It also finds <iframe>s where the src is a http URL.

@kaeluka kaeluka requested a review from a team as a code owner February 14, 2022 10:38
@kaeluka kaeluka marked this pull request as draft February 14, 2022 10:38
@kaeluka kaeluka marked this pull request as ready for review February 14, 2022 11:05
@esbena
Copy link
Contributor

esbena commented Feb 14, 2022

(For completeness: even the use of HTTPS is problematic without an integrity check. A compromised JavaScript CDN that is accessed through HTTPS can easily respond with malicious content.)

@kaeluka
Copy link
Author

kaeluka commented Feb 15, 2022

(For completeness: even the use of HTTPS is problematic without an integrity check. A compromised JavaScript CDN that is accessed through HTTPS can easily respond with malicious content.)

Yes, that is true; but in practice, the integrity check is almost never used — it would create a lot of noise to demand that.

@esbena
Copy link
Contributor

esbena commented Feb 15, 2022

Hmm. Is there a significant difference in the result quality from the two queries that prevents us from having a single unified query?

@kaeluka
Copy link
Author

kaeluka commented Feb 15, 2022

No, but the queries are completely different — one is a taint tracking query, and the other is working on the level of HTML elements. That was the reason I built two independent queries.

@kaeluka
Copy link
Author

kaeluka commented Feb 16, 2022

I have now:

  • refactored the taint tracking query to use type tracking
  • merged the two resulting queries and updated the change log file and the tests
  • removed flagging of // urls
  • made the integrity attribute mandatory for <script/> elements that load from CDNs which recommend using the attribute — eg, by including the attribute in their code snippets.
  • removed flagging of the ternaries (added a test for that, too)
  • polished the qlhelp documentation some more

Is anything missing, @esbena and @asgerf?

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.

Just a few comments from me.

Another point is that it would be nice to be consistent in how we format the words http, https and url.

My suggestion would be:

  • http: and https:. In the alert message, surround with single quotes, in qhelp surround with <code>.
  • URL. No other formatting.

@kaeluka
Copy link
Author

kaeluka commented Feb 16, 2022

I went through the experiment results with @esbena — they look good (except some failures that seem to be caused by DCA or github actions).

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.

I like the general structure of the query, I think this can be a good query.

I got a lot of minor comments, mostly about the documentation.

@kaeluka
Copy link
Author

kaeluka commented Feb 21, 2022

Thank you, @erik-krogh, for the thorough review! I went through your comments and hope to have fixed them all. Are you happy with the fixes?

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.

A few more comments before I call it a day.
Your changes look good 👍

There are still two comments from my previous round that you haven't addressed.

Also, the autoformatter is failing.

@erik-krogh
Copy link
Contributor

Looks good, lets get a doc review on this.

@erik-krogh erik-krogh added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Feb 22, 2022
Copy link
Contributor

@hubwriter hubwriter left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few editorial comments.

@kaeluka
Copy link
Author

kaeluka commented Feb 23, 2022

Thank you, @hubwriter! I've added a new commit.

@kaeluka kaeluka merged commit a664e02 into github:main Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation JS ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants