Skip to content

JS: add query for detecting insecure temporary files #7626

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 4 commits into from
May 16, 2022

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Jan 18, 2022

To safely create a temporary file in the OS temp dir in a multi-user environment you must:

  • Ensure that the file is created with 0o600 permissions or similar.
  • Ensure that you're not reusing an existing file (Use the O_EXCL and O_CREAT flags)
    (An exploit requires extremely precise timing by inserting a file between an fs.exists(..) and a fs.open(..) call.)

This query checks for the first of these requirements.

The only way to ensure the second requirement is to use fs.open(.., O_EXCL | O_CREAT), which is what e.g. tmp does.
Checking for the second requirement gets complicated. One of the complications is that NodeJS programs very rarely use file handles, and instead just calls the fs module multiple times with the same string path.
So I went the easy route and just look for bad permissions, which is the most important part.
(But the recommendation in the .qhelp basically says: use tmp. Because that package does everything right).

I haven't found any FPs.
But I suspect that FPs might happen when a program safely creates a temp file in a utility method, but the query still sees that utility method as a source.

An evaluation shows no new results, and a constant 30s slowdown (that's the extra time it takes to compile the query, something went wrong in a preparation step).
So the evaluation show no noticeable slowdown.

A test run finds some results.
All the results are either in test code or benign, which is why I've set the precision to medium.

@erik-krogh erik-krogh added Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish WIP This is a work-in-progress, do not merge yet! labels Jan 18, 2022
@erik-krogh erik-krogh removed WIP This is a work-in-progress, do not merge yet! Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Jan 19, 2022
@erik-krogh erik-krogh marked this pull request as ready for review January 19, 2022 12:47
@erik-krogh erik-krogh requested a review from a team as a code owner January 19, 2022 12:47
Copy link
Contributor

@kaeluka kaeluka left a comment

Choose a reason for hiding this comment

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

I found a few typos and minor issues, but nothing all that serious. Rest LGTM

@kaeluka
Copy link
Contributor

kaeluka commented Feb 2, 2022

@erik-krogh and me just had a small follow-up call; I asked whether fs.constants (https://nodejs.org/api/fs.html#fsconstants) is/should be supported; his reply was that it isn't since that using it implies that users are likely doing it right. That argument makes sense to me.

erik-krogh and others added 2 commits February 2, 2022 15:00
@kaeluka
Copy link
Contributor

kaeluka commented Feb 2, 2022 via email

kaeluka
kaeluka previously approved these changes Feb 3, 2022
@kaeluka
Copy link
Contributor

kaeluka commented Feb 3, 2022

I have now approved this change because it's good in my opinion. Take my opinion with a grain of salt. If you want an extra review by someone who's been here longer, that might make sense — I'll leave that decision to you.

@erik-krogh
Copy link
Contributor Author

I'll leave that decision to you.

I think I'll merge it.
After it has gone through a doc review.

@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 3, 2022
mattpollard
mattpollard previously approved these changes Feb 7, 2022
Copy link
Contributor

@mattpollard mattpollard left a comment

Choose a reason for hiding this comment

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

Hello from Docs 👋🏻 I've left a few minor suggestions for you to consider, but otherwise, the changes looks good to me from a documentation perspective!

Co-authored-by: Matt Pollard <mattpollard@users.noreply.github.com>
@erik-krogh erik-krogh dismissed stale reviews from mattpollard and kaeluka via 4c317f5 February 7, 2022 08:43
@erik-krogh erik-krogh removed the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Apr 29, 2022
@erik-krogh erik-krogh requested a review from kaeluka April 29, 2022 13:33
Copy link
Contributor

@kaeluka kaeluka left a comment

Choose a reason for hiding this comment

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

Still looks good!

@erik-krogh erik-krogh merged commit 23981cb into github:main May 16, 2022
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.

3 participants