JavaScript: Add new query for ZipSlip (CWE-022). #889

jcreedcmu · 2019-02-05T20:29:58Z

A first stab at a query to find uses of tar-stream that risk clobbering files in unexpected places if the tar file has malicious ../../../relative/paths in it.

ghost · 2019-02-05T20:30:09Z

All committers have signed the CLA.

aschackmull · 2019-02-06T08:56:49Z

This query is called ZipSlip in C# and Java, but at least the Java version also covers other archive formats, including tar. So should we perhaps also name this query ZipSlip to have a bit more uniformity across languages? Obviously this query would then need to also cover zip in order not to have a misleading name.

jcreedcmu · 2019-02-06T14:26:44Z

Indeed, I hesitated over exactly that naming choice.

For context, I was directed to decompress-zip as a javascript library to consider writing a query against, and I thought at first its advertised interface appears to be rather different from the C# and Java cases --- both of them (as well as tar-stream) give you archive entries and expect the caller to actually carry out the extraction, but it looked from the documentation that decompress-zip just carries out the extraction itself.

On second look, I see that DecompressZip.prototype.readFileEntries does exist, but it's not clear to me if that is either meant to be used by library users, or used in practice enough to justify writing a query about it.

jcreedcmu · 2019-02-06T16:17:17Z

Talked to @xiemaisi out of band, probably npm package unzip is a fine target for modelling an api that actually returns file entries as data for the caller to process, so that resolves my uncertainty - I'll just make a zip version and call the whole thing ZipSlip for better truth in advertising.

jcreedcmu · 2019-02-11T16:39:49Z

I've made the query apply to zip (specifically the npm library unzip) instead of tar and tried to use the standard library more effectively.

javascript/ql/src/Security/CWE-022/ZipSlip.ql

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

asger-semmle · 2019-02-11T18:08:07Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+    UnzipEntrySource() {
+      exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on |
+        pipe.getMethodName() = "pipe"
+        and pipe.getArgument(0) = DataFlow::moduleImport("unzip").getAMemberCall("Parse")


The line above doesn't match indirections through variable assignments, for example:

let unzipper = unzip.Parse(); fs.createReadStream(x).pipe(unzipper).on(..)

There are two equivalent ways to handle this. One uses getALocalSource:

pipe.getArgument(0).getALocalSource() = DataFlow::moduleImport("unzip").getAMemberCall("Parse")

and the other uses flowsTo and is written the other way around:

DataFlow::moduleImport("unzip").getAMemberCall("Parse").flowsTo(pipe.getArgument(0))

I chose .getALocalSource(). Are there other considerations that might make me (dis)prefer .flow() in the future? Is performance the same either way in practice?

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

aschackmull · 2019-02-12T15:12:12Z

You might want to run the ql code through the autoformatter (Ctrl+Shift+F in Eclipse or target/jars/qlformat from the commandline).

jcreedcmu · 2019-02-13T19:03:02Z

You might want to run the ql code through the autoformatter (Ctrl+Shift+F in Eclipse or target/jars/qlformat from the commandline).

Awesome! Done.

jcreedcmu · 2019-02-13T19:06:00Z

I left three separate commits in case that's easier to read. Happy to preemptively squash if that's preferred, and I expect to squash before it gets merged into master anyhow.

asger-semmle

Thanks for addressing my comments; I have another batch of comments. We should aim to get this landed this week.

asger-semmle · 2019-02-25T11:57:56Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+   * filepath will not refer to parent directories.
+   */
+  string getAParentDirName() {
+    result = any(string s | s = ".." or s = "../")


The any is not necessary, just do:result = ".." or result = "../"

Yeah, oops, dunno how I ended up with that redundancy.

asger-semmle · 2019-02-25T12:09:24Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+   * `x` flows to `.pipe(x)`
+   */
+  predicate flowsThroughPipe(DataFlow::Node node1, DataFlow::Node node2) {
+    stepsThroughPipe*(node1, node2)


Unfortunately this relation is huge. DataFlow::localFlowStep is very large, which means stepsThroughPipe is also large, and we're only interested in a very small subset of this graph. Materializing this graph is a bit expensive, let alone computing its transitive closure.

In cases like this I think it's better to use explicit recursion: (untested)

DataFlow::SourceNode parsedStream() { result = DataFlow::moduleImport("unzip").getAMemberCall("Parse") or exists (DataFlow::MethodCallNode pipe | pipe.getMethodCall() = "pipe" and pipe.getArgument(0).getALocalSource() = parsedStream() and result = pipe ) )

You can also write this with the transitive closure operator if you prefer, but it should not be over a relation that contains localFlowStep.

Ah, great, thanks, this really helps me fill out my mental model of how the evaluator is likely to deal with these kinds of patterns. I was trying to formulate a question of the form "ok so I think what I want is to customize local data flow the same way Configurations let you customize interprocedural data flow, and this is all I could think to arrive at given I couldn't find any extension points" but the performance considerations you're describing make sense.

xiemaisi

This is looking very promising, thanks @jcreedcmu! I have one minor comment and agree with @asger-semmle's comment about the handling of .pipe.

I remember your PR originally included a model of the tar-stream package. Considering how popular that package is, could you add it back in again? (No need to generalise the query name or description to reference .tar, I think.)

We'll also need to do a quick profile before this is merged; you can leave that to us (i.e., @Semmle/js) unless you are interested in learning how to do it yourself.

xiemaisi · 2019-02-25T12:24:05Z

javascript/ql/src/Security/CWE-022/ZipSlip.ql

+ * @kind path-problem
+ * @id js/zipslip
+ * @problem.severity error
+ * @precision high


Let's start this out as medium precision. That way it'll be run on LGTM, but results won't be displayed by default yet. After the next dist upgrade, we can take a look at the results and then decide whether we want to display by default or not.

Sounds good; thanks for pointing out that side-effect, which caused me to find the wiki page documenting the whole precision x severity table.

Do you mind if I postpone the tarslip case to another PR, so comments are less tangled? I do have the commit sitting in my back pocket on a local branch so I can just rebase it.

I am interested in performance profiling --- this is using dist-compare, right?

Sure. (Btw, we don't generally insist on squashing PRs down to a single commit if that's what you're worried about.)

I am interested in performance profiling --- this is using dist-compare, right?

Correct. I'll ping you on slack about this.

ghost

I will be the pedantic one in this review.
The docstrings are not as idiomatic as we like them to be, please see my suggestions.

javascript/ql/src/Security/CWE-022/ZipSlip.qhelp

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

xiemaisi

A few more thoughts, but overall this is shaping up nicely.

javascript/ql/src/Security/CWE-022/ZipSlipBad.js

xiemaisi · 2019-02-27T14:31:11Z

javascript/ql/src/Security/CWE-022/ZipSlip.qhelp

+<sample src="ZipSlipBad.js" />
+
+<p>To fix this vulnerability, we can to check that the path does not
+contain any <code>".."</code> in it.


Suggested change

contain any <code>".."</code> in it.

contain any <code>".."</code> elements in it.

(For consistency with above.)

xiemaisi · 2019-02-27T14:32:27Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+    exists(DataFlow::MethodCallNode pipe |
+      pipe.getMethodName() = "pipe" and
+      parsedArchive().flowsTo(pipe.getArgument(0))
+    )


This disjunct doesn't seem to constrain result.

Oops, fixed.

xiemaisi · 2019-02-27T14:33:27Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+            .getAMemberCall("on")
+            .getCallback(1)
+            .getParameter(0)
+            .getAPropertyRead("path")


Do we want to check somewhere that the on-ed event is "entry", or is that unnecessary since we're looking for accesses to path anyway?

That got lost somewhere during refactoring. I think it's a good idea to have it. Have put it back in.

xiemaisi · 2019-02-27T14:35:25Z

@mc-semmle (or anyone else from @Semmle/doc), do you think you will find time to do a doc review for this query before feature freeze? Otherwise we'll just go ahead and merge this when it's ready and open another PR to patch up the docs later.

jf205 · 2019-02-27T15:11:25Z

@mc-semmle will be back in the office tomorrow morning to have a look at this.

felicitymay

(Reviewing on behalf of @mc-semmle.)

This looks good and it sounds as if it will be a great addition to the query set. Just a few comments on the text.

javascript/ql/src/Security/CWE-022/ZipSlip.qhelp

javascript/ql/src/Security/CWE-022/ZipSlipBad.js

javascript/ql/src/Security/CWE-022/ZipSlipGood.js

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

felicitymay · 2019-02-28T09:23:40Z

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

+
+  /**
+   * Gets a string which suffices to search for to ensure that a
+   * filepath will not refer to parent directories.


The meaning of this sentence isn't entirely clear to me.

Ok, I tried rewording it.

felicitymay

Thanks for the doc updates. All LGTM apart from one doc comment.

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

jcreedcmu · 2019-02-28T20:18:46Z

Ok, does this LGTE then, modulo performance profiling to be done?

ghost · 2019-02-28T20:31:33Z

My comments have been addressed.

xiemaisi

LGTM. As discussed, we'll merge now to get this into the release. Preliminary profiling looks good, but we'll do another, more extensive, round of profiling over the weekend just to make sure.

Also, let's remember to

add path.basename as a sanitiser (which came up in the preliminary results)
add a change note.

This can be done in a separate PR, perhaps together with any other fixes motivated by results from the profiling run.

xiemaisi · 2019-03-04T08:47:05Z

Evaluation (internal link) shows no result differences on default suite.

The relative runtime differences are not, in this case, particularly revealing, so I've added another comparison showing the absolute differences, which is just the runtime of the new query, which looks entirely reasonable.

So apart from the improvements mentioned above I think this is good. 👍

jcreedcmu requested a review from a team as a code owner February 5, 2019 20:29

xiemaisi added JS WIP This is a work-in-progress, do not merge yet! labels Feb 6, 2019

jcreedcmu force-pushed the jcreed/tarslip branch 2 times, most recently from bb2c853 to 28fc18d Compare February 11, 2019 16:38

jcreedcmu force-pushed the jcreed/tarslip branch from 28fc18d to 81f7350 Compare February 11, 2019 16:41

jcreedcmu changed the title ~~JavaScript: Add new query TarSlip (CWE-022).~~ JavaScript: Add new query for ZipSlip (CWE-022). Feb 11, 2019

jcreedcmu force-pushed the jcreed/tarslip branch from 81f7350 to 1e7442a Compare February 11, 2019 16:43

asger-semmle reviewed Feb 12, 2019

View reviewed changes

jcreedcmu force-pushed the jcreed/tarslip branch from 1e7442a to 3ea0460 Compare February 13, 2019 19:04

jcreedcmu force-pushed the jcreed/tarslip branch from 30855e2 to 12f4183 Compare February 22, 2019 20:04

asger-semmle reviewed Feb 25, 2019

View reviewed changes

xiemaisi suggested changes Feb 25, 2019

View reviewed changes

ghost suggested changes Feb 25, 2019

View reviewed changes

jcreedcmu force-pushed the jcreed/tarslip branch from 12f4183 to 98e8de7 Compare February 26, 2019 13:15

xiemaisi suggested changes Feb 27, 2019

View reviewed changes

felicitymay reviewed Feb 28, 2019

View reviewed changes

xiemaisi added this to the 1.20 milestone Feb 28, 2019

felicitymay reviewed Feb 28, 2019

View reviewed changes

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll Show resolved Hide resolved

ghost previously approved these changes Feb 28, 2019

View reviewed changes

jcreedcmu and others added 12 commits February 28, 2019 15:45

JS: Add new query for ZipSlip (CWE-022)

baa4f08

JS: Address review comments

abd2644

JS: Run auto-formatter

32d48ba

JS: Unbreak TaintedPath

23d37c7

JS: Better local flow through .pipe chaining

b0636dd

JS: More efficient reasoning through pipe

09b9a57

JS: Address review comments

2fc2a39

JS: Fix incorrect sample link

caebdd2

JS: Address review comments

674d279

JS: Actually use fileName in examples

c5e57da

JS: Documentation fixes

c1b218a

JS: Add ZipSlip query to security suite

86bbb5f

jcreedcmu dismissed ghost ’s stale review via 86bbb5f February 28, 2019 20:47

jcreedcmu force-pushed the jcreed/tarslip branch from fd39f85 to 86bbb5f Compare February 28, 2019 20:47

xiemaisi approved these changes Mar 1, 2019

View reviewed changes

xiemaisi merged commit 8dcd871 into github:master Mar 1, 2019

jcreedcmu deleted the jcreed/tarslip branch March 4, 2019 09:05

	contain any <code>".."</code> in it.
	contain any <code>".."</code> elements in it.

JavaScript: Add new query for ZipSlip (CWE-022). #889

JavaScript: Add new query for ZipSlip (CWE-022). #889

Uh oh!

Conversation

jcreedcmu commented Feb 5, 2019

Uh oh!

ghost commented Feb 5, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented Feb 6, 2019

Uh oh!

jcreedcmu commented Feb 6, 2019

Uh oh!

jcreedcmu commented Feb 6, 2019

Uh oh!

jcreedcmu commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschackmull commented Feb 12, 2019

Uh oh!

jcreedcmu commented Feb 13, 2019

Uh oh!

jcreedcmu commented Feb 13, 2019

Uh oh!

asger-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiemaisi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 5, 2019 •

edited by ghost

Loading

jcreedcmu commented Feb 11, 2019 •

edited

Loading

felicitymay left a comment •

edited

Loading