-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
There was a problem hiding this 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
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Show resolved
Hide resolved
javascript/ql/test/query-tests/Security/CWE-377/InsecureTemporaryFile.expected
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Outdated
Show resolved
Hide resolved
javascript/ql/lib/semmle/javascript/security/dataflow/InsecureTemporaryFileCustomizations.qll
Outdated
Show resolved
Hide resolved
@erik-krogh and me just had a small follow-up call; I asked whether |
Co-authored-by: Stephan Brandauer <kaeluka@github.com>
> `~/myDir/tmp/foo` is not technically safer
It is safer, because another user doesn't have access to that path.
The global temp dir is shared between all users, and that's why bad things
can happen.
Ah, silly me! Of course :) glad I asked, though
|
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. |
I think I'll merge it. |
There was a problem hiding this 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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looks good!
To safely create a temporary file in the OS temp dir in a multi-user environment you must:
0o600
permissions or similar.O_EXCL
andO_CREAT
flags)(An exploit requires extremely precise timing by inserting a file between an
fs.exists(..)
and afs.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 stringpath
.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: usetmp
. 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
.