Insecure comparison

[iliakan]

Without that fix, a path /my-secret is consedered fine for the root /my.

Maybe the issue is autofixed by the next parts assignment? Not sure exactly if the vulnerability exists, but the comparison is insecure.

[dougwilson]

You must provide a test case that fails without the change for it to be accepted.

[iliakan]

Tests could test if the problem exists, yes.

But even if the tests do not fail, but the code is bad, should we leave the code at place?

[dougwilson]

How is the code bad? The code makes assumptions based on the code prior, which is why it works. Unless you can prove that it doesn't work, then the code is not going to change.

[dougwilson]

The two lines of code right above the if makes the check valid. You're basically arguing that deferencing a pointer would cause a null pointer exception, without looking up to see that there was code validating that it wasn't a null pointer.

[dougwilson]

I'll reopen your PR because I do see a way, but it would have been nice if you had provided a test.

[dougwilson]

Your code also fails anyway, because it will end up adding double path separators at the end in other circumstances.

[dougwilson]

In the future, please try to report security exploits not in the public, so there is a chance to get a fix out to everyone before people are aware of it.

[iliakan]

Hi @dougwilson, I was going to submit the test, but I see the issue is fixed. Fine.
I believe the vulnerability (if any) is not very dangerous here, of course I'd send a private message if I see something really bad.

[dougwilson]

No problem. The main reason I didn't use your change was because the it broke setting the root option with a trailing slash (like root: '/some/dir/'). If the trailing slash was there, the code would thing every path was malicious since you were comparing '/some/dir/f' === '/some/dir//', AFAIK

[dougwilson]

Anyway, I want to say thank you for the report. I'm sorry I assumed the report was invalid at first, but I couldn't tell quickly since it had lacked any tests to demonstrate the issue.

[olecom]

In https://nodesecurity.io/advisories/send-directory-traversal it is said that "Vulnerable: < 0.8.4", but it seems that "version": "0.6.0" is fine before 6a49bf7

[iliakan]

Thank you for a quick fix, @dougwilson. If I knew your reaction would be so fast, I'd prepare the test beforehands.

kudos

[dougwilson]

@olecom I'm not sure where you are coming up with 0.6.0 not being vulnerable; if you run the test in commit 9c6ca9b, you will see it fails on 0.6.0 because it is vulnerable.

[dougwilson]

I just tested all the way down to 0.5.0 and I can guarantee you 0.5.0 to 0.8.3 are vulnerable. If you want to verify any version older than 0.5.0, you can as the commit includes the test you can use (make sure your fixtures directory is setup properly).

[dougwilson]

@iliakan no problem :) I try to react quickly to any issue, let alone a potential security issue :)

[olecom]

tested: up to 0.2.0 'should not allow root transversal' is OK

[dougwilson]

Sweet :) So it sounds like the vulnerability affects 0.2.0 to 0.8.3

[olecom]

$ diff -U 8 send_orig.js send.js

Test modification to be visible if was ran

--- send_orig.js        2014-09-20 16:02:20.080173975 +0300
+++ send.js     2014-09-20 16:02:53.148174066 +0300
@@ -1133,17 +1133,17 @@
       it('should not allow root transversal', function(done){
         var app = http.createServer(function(req, res){
           send(req, req.url, {root: __dirname + '/fixtures/name.d'})
           .pipe(res);
         });

         request(app)
         .get('/../name.dir/name.txt')
-        .expect(403, done)
+        .expect(403, '!', done)
       })
     })

     describe('when missing', function(){
       it('should consider .. malicious', function(done){
         var app = http.createServer(function(req, res){
           send(req, fixtures + req.url)
           .pipe(res);

List versions:

olecom@bubucik:/usr/local/lib/node_modules$ echo send*
send@0.1.4 send@0.2.0 send@0.3.0 send@0.4.0
olecom@bubucik:/usr/local/lib/node_modules$ 

Run tests

for s in send@*;
do
  cd $s && echo "test: $s";
  ../.bin/mocha 2>&1 | sed '/75)/{n;p};d';
  cd ..;
done

0.3.0 and 0.4.0 don't run those tests:

test: send@0.1.4
  75) send(file, options) root when given should not allow root transversal:
     Error: expected '!' response body, got 'Forbidden'
test: send@0.2.0
  75) send(file, options) root when given should not allow root transversal:
     Error: expected '!' response body, got 'Forbidden'
test: send@0.3.0
test: send@0.4.0
(live)olecom@bubucik:/usr/local/lib/node_modules$
(live)olecom@bubucik:/usr/local/lib/node_modules$ cd send@0.3.0/
(live)olecom@bubucik:/usr/local/lib/node_modules/send@0.3.0$ ../.bin/mocha
  .....................

  17 passing (382ms)
  4 failing
(live)olecom@bubucik:/usr/local/lib/node_modules/send@0.3.0$ cd ../send@0.2.0/
(live)olecom@bubucik:/usr/local/lib/node_modules/send@0.2.0$ ../.bin/mocha
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․

  36 passing (3s)
  76 failing

[dougwilson]

@olecom your post doesn't show how you are setting up the fixtures directory. I just did a test manually on 0.1.0 and it's affected as well, so the issue goes back at least to 0.1.0. As far as I can tell, it has existed forever, as the initial commit (347cb7c) contains the bad indexOf check that caused the issue in the first place.

[olecom]

your post doesn't show how you are setting up the fixtures directory

i can't really say, they are all the same... Would you try 0.2.0 as it is almost the same as 0.1.4 (which is used by me)?

(live)olecom@bubucik:/usr/local/lib/node_modules$
for s in send@*; do cd $s && echo "test: $s"; ../.bin/mocha 2>&1 | sed '/trans/{n;p};d'; cd ..; done
test: send@0.1.4
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․trans:/../name.dir/name.txt
․․
  27) send(file).pipe(res) with Range request should set Content-Length to the # of octets transferred:
     Error: expected '34' response body, got 'Not Found'
  75) send(file, options) root when given should not allow root transversal:
     Error: expected '!' response body, got 'Forbidden'
test: send@0.2.0
  ․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․․trans:/../name.dir/name.txt
․․
  27) send(file).pipe(res) with Range request should set Content-Length to the # of octets transferred:
     Error: expected '34' response body, got 'Not Found'
  75) send(file, options) root when given should not allow root transversal:
     Error: expected '!' response body, got 'Forbidden'
test: send@0.3.0
test: send@0.4.0
test: send@0.5.0
  17) send(file, options) root when given should not allow root transversal:
     Error: expected '!' response body, got 'tobi'
  34) send(file, options) root when given should not allow root transversal:
     Error: expected 403 "Forbidden", got 200 "OK"
(live)olecom@bubucik:/usr/local/lib/node_modules$ echo send*
send@0.1.4 send@0.2.0 send@0.3.0 send@0.4.0 send@0.5.0
(live)olecom@bubucik:/usr/local/lib/node_modules$

[dougwilson]

Yes, just tried 0.2.0 and it's affected for sure. The fixtures directly changes a lot between the different versions, so they wouldn't be the same unless you're copy and pasting the entire directory.

Oddly, your post above shows you running the command ../.bin/mocha for 0.2.0, which doesn't even work, because you have to add the --require=should command line in that version.

Here is the patch to show the issue occurring in 0.2.0:

From 9619cab3d1ba9e3a5605781d09a44b450e3fd4c9 Mon Sep 17 00:00:00 2001
From: Douglas Christopher Wilson <doug@somethingdoug.com>
Date: Sat, 20 Sep 2014 06:27:23 -0700
Subject: [PATCH] 0.2.0-test

---
 test/fixtures/pets-copy/index.html | 1 +
 test/send.js                       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
 create mode 100644 test/fixtures/pets-copy/index.html

diff --git a/test/fixtures/pets-copy/index.html b/test/fixtures/pets-copy/index.html
new file mode 100644
index 0000000..f3cb29b
--- /dev/null
+++ b/test/fixtures/pets-copy/index.html
@@ -0,0 +1 @@
+wat
\ No newline at end of file
diff --git a/test/send.js b/test/send.js
index 5d07bfa..bbc84c6 100644
--- a/test/send.js
+++ b/test/send.js
@@ -384,7 +384,7 @@ describe('send(file, options)', function(){
         });

         request(app)
-        .get('/pets/../../send.js')
+        .get('/pets/../pets-copy/')
         .expect('Forbidden')
         .end(done);
       })
-- 
1.9.2.msysgit.0

Even through the root is restricted to the pets dir, I can still access the pets-copy dir, which is outside the pets dir, which is the issue.

[dougwilson]

I just double-checked all the versions below 0.2.0 and the report you mentioned is 100% accurate: all versions below 0.8.4 are affected by this.

[dougwilson]

Also, it's important to note that you are only vulnerable depending on how you are declaring your "root" option: if you simply add a trailing slash, you will have mitigated the issue in all versions and do not need to upgrade.

Vulnerable:

send(req, path, {root: '/some/path'})

Mitigated:

send(req, path, {root: '/some/path/'})