Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jun 11, 2020

CVE-2017-16003: TP/TN

A very closely related issue to look for http urls in 'package.jsonfiles, or<script />tags withhttp` urls, but that is not a path-problem, so it feels weird mashing that into the same query.
Maybe we should get back to that later?

It was suggested at one point that I could add an integrity-check as a sanitizer, but I don't know what that would look like?

The query only supports the http and ftp protocols for now. Suggestions for additional protocols are very welcome.

Performance evaluation waits until after this PR has been merged into the sprint branch.

@erik-krogh erik-krogh added the JS label Jun 11, 2020
@erik-krogh erik-krogh requested a review from a team June 11, 2020 15:09
@erik-krogh erik-krogh marked this pull request as ready for review June 12, 2020 09:02
@erik-krogh erik-krogh requested a review from mchammer01 as a code owner June 12, 2020 09:02
@erik-krogh erik-krogh removed the request for review from mchammer01 June 12, 2020 09:03
@erik-krogh erik-krogh changed the title JS: add simple query for detecting sensitive files downloaded over unsecu… JS: add simple query for detecting sensitive files downloaded over insecure connection Jun 12, 2020
@erik-krogh erik-krogh marked this pull request as draft June 15, 2020 11:05
@erik-krogh erik-krogh marked this pull request as ready for review June 15, 2020 11:45
@erik-krogh erik-krogh requested a review from a team June 15, 2020 11:46
*/
string unsafeExtension() {
result =
["exe", "dmg", "pkg", "tar.gz", "zip", "sh", "bat", "cmd", "app", "apk", "msi", "dmg",
Copy link
Contributor

Choose a reason for hiding this comment

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

What about .jar and .war files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, those after often used as executeables.

Co-authored-by: Asger F <asgerf@github.com>
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.

LGTM

@mchammer01 another doc review for you 😄

Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

@erik-krogh - this LGTM ✨
Happy for this to be merged once you had a look at my suggestions and comments (there are a few typos, but apart from that, it's looking really good).
Hope it helps!

@intrigus-lgtm
Copy link
Contributor

I'm not sure how relevant this is in practice:
When I looked at a similar query for java, I found instances where the download link looked like this:
http://foo.foo/latestVersion (Notice: No extension). This would then be saved to a version.exe file.

So it may be useful to also to track such cases. Or maybe not 🤷‍♂️

@erik-krogh
Copy link
Contributor Author

I'm not sure how relevant this is in practice:
When I looked at a similar query for java, I found instances where the download link looked like this:
http://foo.foo/latestVersion (Notice: No extension). This would then be saved to a version.exe file.

So it may be useful to also to track such cases. Or maybe not

I think it is useful to track such cases.

And you got a good point, thanks 👍
But it is a big change that is better suited for a later PR.

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
@erik-krogh
Copy link
Contributor Author

Happy for this to be merged once you had a look at my suggestions and comments (there are a few typos, but apart from that, it's looking really good).
Hope it helps!

It did, I agree with all your suggested changes 👍

Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants