

Node v0.10.21 Stable has critical security fix - aaronblohowiak
http://blog.nodejs.org/2013/10/18/node-v0-10-21-stable/

======
meritt
HTTP Pipelining is designed so a client can send multiple requests without
waiting on a response and then the server sends all the responses in order.
This is helpful on high latency links since it can combine numerous HTTP
requests into fewer packets. The exploit is the client never stops to read and
just writes requests nonstop. Meanwhile node's http.Server continually
populates a response buffer which is never consumed.

Node uses Stream[1] objects for reading/writing streams of data. The Stream
object has a 'needsDrain' boolean which is set once its internal buffer
surpasses the highWaterMark (defaults to 16kb). Subsequent writes will return
false[2] and code should wait until the 'drain' event is emitted, signaling
it's safe to write again[3]. The documentation even warns about this scenario:

> _However, writes will be buffered in memory, so it is best not to do this
> excessively. Instead, wait for the drain event before writing more data._

http.Server[4] uses a writeable stream to send responses to a client. Until
this patch[5] it was ignoring the needsDrain/highWaterMark status and just
writing to the stream. It fills up the buffer of the writeable stream, far
beyond the high water mark and eventually runs out of memory.

The patch resolves this by checking when needsDrain is set, then it stops
writing and stops reading/parsing incoming data. It then waits until the
'drain' event is fired and then proceeds as normal.

[1] [http://nodejs.org/api/stream.html](http://nodejs.org/api/stream.html)

[2]
[http://nodejs.org/api/stream.html#stream_writable_write_chun...](http://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback)

[3]
[http://nodejs.org/api/stream.html#stream_event_drain](http://nodejs.org/api/stream.html#stream_event_drain)

[4]
[http://nodejs.org/api/http.html#http_class_http_server](http://nodejs.org/api/http.html#http_class_http_server)

[5]
[https://github.com/joyent/node/commit/085dd30e93da67362f044a...](https://github.com/joyent/node/commit/085dd30e93da67362f044ad1b3b6b2d997064692)

------
xs_kid
\- No CVE.

\- Distributions weren't contacted prior the release.

\- Everyone can see the diff for the fix in the codebase.

\- There are a PoC as test-case in the code.

\- The release was done in the start of weekend when everyone in America is
leaving the office and everyone in Europe is sleeping.

\- A big part of the community is in two conferences right now.

IMHO that was the worst way to provide a security update.

~~~
glenjamin
The initial report was made publicly, including a proof of concept exploit,
and is currently viewable via google cache. The censored issue report is
referenced with a metasploit pull request containing code that can be used to
exploit.

Given this, I'd say sooner is better than later.

It would be prudent to mention making your load balancer limit the number of
requests than can be pipelined down a single connection should resolve any
issue.

~~~
xs_kid
The metasploit exploit was possible only thanks to the release (see the tweets
of metasploit maintainer @hdmoore), IMO they could have waited until Monday
morning to release it.

------
jared314
Apparently, a trivial DoS vulnerability for any Node serving HTTP:

[https://groups.google.com/forum/#!msg/nodejs/NEbweYB0ei0/gWv...](https://groups.google.com/forum/#!msg/nodejs/NEbweYB0ei0/gWvyzCunYjsJ)

The odd thing about non-disclosure in an open source project is: I can diff
the code bases before and after the fix.

[https://github.com/joyent/node/issues/6214](https://github.com/joyent/node/issues/6214)

[https://github.com/joyent/node/commit/085dd30e93da67362f044a...](https://github.com/joyent/node/commit/085dd30e93da67362f044ad1b3b6b2d997064692)

And, they have a test script:

[https://github.com/joyent/node/blob/085dd30e93da67362f044ad1...](https://github.com/joyent/node/blob/085dd30e93da67362f044ad1b3b6b2d997064692/test/simple/test-
http-pipeline-flood.js)

~~~
jared314
And, now metasploit has two competing modules in the works:

[https://github.com/rapid7/metasploit-
framework/pull/2548](https://github.com/rapid7/metasploit-framework/pull/2548)

[https://github.com/rapid7/metasploit-
framework/pull/2549](https://github.com/rapid7/metasploit-framework/pull/2549)

------
jph
Node team: you're censoring the original ticket, which is unwise IHMO.

Your approach makes it impossible for an honest sysadmin to quickly find a way
to block the attack using a firewall, but your approach doesn't stop an
attacker from building an exploit based on the public commit.

Someone will come up with a proof of concept exploit quickly, and post it,
probably here.

Please do the right thing: un-censor the GitHub ticket so we can understand
what's happening.

~~~
xs_kid
PoC is in codebase, it was published as a test-case for the fix.

~~~
jkrems
Unfortunately that test-case passes against 0.8.25 as well. So I'm not quite
convinced it can be used reliably to reproduce the problem.

~~~
ak217
Unsurprising because the new streams API, which is responsible for this bug,
was introduced in node 0.10. Try with earlier 0.10 versions instead.

~~~
jkrems
Well, in that case lets add confusion about affected versions to the list of
things being suboptimal about this whole thing:
[http://blog.nodejs.org/2013/10/18/node-v0-8-26-maintenance/](http://blog.nodejs.org/2013/10/18/node-v0-8-26-maintenance/)
\- the same warning about the same error and they did backport the fix to 0.8:
[https://github.com/joyent/node/commit/653d4db71f569ddc87a0bc...](https://github.com/joyent/node/commit/653d4db71f569ddc87a0bc21f5ecc5ceaf37f932)

~~~
ak217
You're right, that is confusing. I'm not sure how it's exploitable in 0.8.

------
viraptor
So what they did is: release a new version with security and other changes
mixed, on Friday, and didn't provide any details so you can't check if you're
vulnerable post-upgrade. They could really handle it a bit better.

~~~
revelation
They did provide plenty detail, just not to the guy on call who has to read
the announcement and react quickly.

Meanwhile, they have the changelog and a test PoC for the one looking to
exploit it.

------
dcu
Making public a critical vulnerability on friday night is not a nice movement.

EDIT: Oh and there's an exploit already.

~~~
xs_kid
sure, they published a PoC in the code:
[https://github.com/joyent/node/blob/085dd30e93da67362f044ad1...](https://github.com/joyent/node/blob/085dd30e93da67362f044ad1b3b6b2d997064692/test/simple/test-
http-pipeline-flood.js)

------
rdtsc
>
> [https://github.com/joyent/node/commit/085dd30e93da67362f044a...](https://github.com/joyent/node/commit/085dd30e93da67362f044ad1b3b6b2d997064692)

Looks like a flood of concurrent requests will just fill up the memory

~~~
jgreen10
I was considering using Node.js for a new project, but I quickly backed away
from it when I realized it didn't do basic flow control properly.

~~~
rdtsc
Funny I noticed this on an internal server as well but chucked to an older
version. Hoping it "clearly is fixed in latest code, something so glaringly
obviously broken, wouldn't be hanging around too long with all the hype
surrounding node these days..."

Anyway, I wouldn't stick node out exposed to the outside world. Granted
sticking nginx in front presumely won't help with this issue. Just keep
feeding a 4GB file to it and it will crash the back-end [EDIT: n.m. I am not
sure anymore, someone mentions it is possible to mitigate it that way]

Yikes, this is a bad one. Glad they fixed it. But it leaves me with the same
impression I had after finding out how MongoDB used to have unacknowledged
writes turned on by default, and people's data was silently getting corrupted.

~~~
sillysaurus2
_Granted sticking nginx in front presumely won 't help with this issue. Just
keep feeding a 4GB file to it and it will crash the back-end._

Why does that happen? nginx can't help here?

~~~
rdtsc
See mathrawka's reply (I haven't test it though)

BTW, I just ran memory of my server into swap with this:

    
    
        $ dd if=/dev/zero of=2g bs=1M count=2048
    
        $ curl -F "2g=@2g" <myresource>
    

(EDIT: explanation, this creates 2g file then uploads it <myresource> as a
file upload -- multipart mime. @ sign just insert the named file data into the
form)

~~~
mathrawka
And that is why you should never blindly use bodyParser middleware in
production...

[http://andrewkelley.me/post/do-not-use-bodyparser-with-
expre...](http://andrewkelley.me/post/do-not-use-bodyparser-with-express-
js.html)

------
charliesome
Proof of concept exploit:
[https://github.com/joyent/node/blob/master/test/simple/test-...](https://github.com/joyent/node/blob/master/test/simple/test-
http-pipeline-flood.js)

~~~
xs_kid
Your ruby version looks nicer:

[https://gist.github.com/anonymous/7050542](https://gist.github.com/anonymous/7050542)

~~~
meowface
Jesus.

If that's all it takes, I have no idea how this wasn't found much sooner.

~~~
JoshGlazebrook
Probably because it's so simple.

------
hackula1
Give me the TLDR. Being Friday 9pm on the east coast, do I need to go home
right now and upgrade my production servers?

~~~
AndyKelley
Nah. It's a DoS vulnerability, which, in general, you're always vulnerable to.
It's just that this one can be done by a single person with relatively low
bandwidth.

------
kylequest
It's not just v0.10.21. There's also an update for v0.8.x too (0.8.26):
[http://blog.nodejs.org/2013/10/18/node-v0-8-26-maintenance/](http://blog.nodejs.org/2013/10/18/node-v0-8-26-maintenance/)

------
rdtsc
Would putting node behind a proxy like nginx mitigate it?

~~~
mathrawka
With these nginx settings, it does mitigate it for me.

    
    
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Protocol http;
        proxy_set_header Host $http_host;
        proxy_pass http://127.0.0.1:8000;
        proxy_redirect off;

------
swills
A CVE would be nice.

------
mathrawka
With a pretty default nginx setup as a proxy in front of node, it does not
affect earlier versions for me.

------
lazyjones
Someone should benchmark the infamous for its speed node.js again after this
(it should be slower)...

