JavaScript: Add tar-stream extraction to ZipSlip query.#1118
JavaScript: Add tar-stream extraction to ZipSlip query.#1118semmle-qlci merged 5 commits intogithub:masterfrom
Conversation
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
LGTM, but needs a change note.
Here are some suggestions for other packages to investigate, they all seem to use a fairly similar API:
- https://www.npmjs.com/package/adm-zip
- https://www.npmjs.com/package/yauzl
- https://www.npmjs.com/package/decompress
- https://www.npmjs.com/package/unzipper
https://www.npmjs.com/package/tar has a slightly different API and only seems vulnerable if you explicitly set the preservePaths option, so not sure whether that's worth modelling. It does seem very popular, though.
Let us know if you'll have the time to look into modelling these, otherwise one of us will pick it up.
| } | ||
|
|
||
| /** A zip archive entry path access, as a source for unsafe zip extraction. */ | ||
| /** Gets a property that is used to get the filename part of an archive entry. */ |
There was a problem hiding this comment.
I assume it's "path" for unzip and "name" for tar-stream? This may be worth clarifying in the comment (but of course it's fine to allow both for both packages as you do here.)
There was a problem hiding this comment.
Good point, done.
|
Let me know if the change note is formatted properly according to convention. I tried looking around the other recent change notes for reference.
these two look worth modelling, but I don't think I'm going to get around to it right now. I'm looking for a part of this api surface that lets you get file contents in order to write yourself, and I only see 'extract everything for me', just like This one was easy to add, since it's a drop in replacement for |
jcreedcmu
force-pushed
the
jcreed/tarslip
branch
from
d2b5f20 to
aa9ba95
Compare
| |--------------------------------|------------------------------|---------------------------------------------------------------------------| | ||
| | Expression has no effect | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. | | ||
| | Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. | | ||
| | ZipSlip | More results | This rule now considers more libraries, including tar as well as zip. | |
There was a problem hiding this comment.
The first field should be the @name of the query, so "Arbitrary file write during zip extraction ("Zip Slip")" in this case.
| * Gets a node that can be a parsed archive. | ||
| */ | ||
| private DataFlow::SourceNode parsedArchive() { | ||
| result = DataFlow::moduleImport("unzipper").getAMemberCall("Parse") |
There was a problem hiding this comment.
Could you add a test case for this one as well?
3 participants
No description provided.