Release Proposal: v4.1.1

[trevnorris]

Contains fix that prevents ArrayBuffers from possibly returning non zero filled memory.

• [d63e02e08d] - buffer: don't set zero fill for zero-length buffer (Trevor Norris) #2931
• [5905b14bff] - build: fix icutrim when building small-icu on BE (Stewart Addison) #2602
• [2600fb8ae6] - deps: upgraded to [email protected] in npm (Kat Marchán) #2958
• [793aad2d7a] - deps: upgrade to npm 2.14.4 (Kat Marchán) #2958
• [f7edbab367] - doc: clarify description of assert.ifError() (Rich Trott) #2941
• [b2ddf0f9a2] - doc: refine process.kill() and exit explanations (Rich Trott) #2918
• [f542e74c93] - http: guard against response splitting in trailers (Ben Noordhuis) #2945
• [bc9f629387] - http_parser: do not dealloc during kOnExecute (Fedor Indutny) #2956
• [d4cd5ac407] - readline: fix tab completion bug (Matt Harrison) #2816
• [2034f68668] - src: honor --abort_on_uncaught_exception flag (Evan Lucas) #2776
• [0b1ca4a9ef] - src: Add ABORT macro (Evan Lucas) #2776
• [816f609c8b] - test: use tmpDir instead of fixtures in readdir (Sakthipriyan Vairamani) #2587
• [2084f52585] - test: test more http response splitting scenarios (Ben Noordhuis) #2945
• [fa08d1d8a1] - test: add test-spawn-cmd-named-pipe (Alexis Campailla) #2770
• [71b5d80682] - test: make cluster tests more time tolerant (Michael Dawson) #2891
• [3e09dcfc32] - test: update cwd-enoent tests for AIX (Imran Iqbal) #2909

@nodejs/release

[rvagg]

suggesting we defer this until Monday, it's late in the week and while #2931 fixes a very important issue, the impact isn't likely to be super large - you have to be:

1. using TypedArrays
2. relying on them being zero-filled
3. be also creating zero-length buffers

One of my primary concerns is in the perception that such a rapid series of releases so close to v4 creates, particularly for people not accustomed with how we've done things with io.js.

[ChALkeR]

@rvagg Correct. Though, here a view from the other side, for balance.
To exploit this, the attacker should be able to:

1. Trigger a creation of a zero-length Buffer.
   A presense of an empty file in /public server dir might (or might not) be enough for that (no time to check now). There could also be a way to trigger that solely by the means of HTTP requests (Buffers are heavily used in Node.js network stack).
2. Trigger a creation of a TypedArray that doesn't have at least some bytes manually set at least in some situations that he later receives data from. This one is project-dependant and may be tricky.
3. The server has to run Node.js 4.1.0 (exactly). This is also not likely, because it has been just released.

Note that 1-2 do not necessary have to be done in a single request, an attacker could launch two parallel tasks — one that creates zero-length Buffers and one that retrives the data. Due to the server be highly async, this could work.

IMO, it would be better to fix that asap just to be on the safe side. This is a data leak.

[ChALkeR]

@nodejs/tsc

[rvagg]

@ChALkeR there's another step in there which is doing something with the TypedArray which is likely to be even more tricky than the other steps you've mentioned. I'm not viewing this as a vulnerability, it's just a bug.

[ChALkeR]

@rvagg Doing what something? Check the testcase. Just creating and recieving data from it (which I mentioned being tricky already) is enough.

Atm, 2+3 almost guarantee that nothing is affected. But if and when a new (affected) version is deployed on more systems, just 2 won't be enough.

TypedArrays not being zero-filled in some cases (especially those that are triggerable by user actions) is a security issue no matter how one views it.

Quick steps to check (for this case):

1. Is this against the docs? Yes. So, it's a bug.
2. Could there be installs where this bug triggers a security vulnerability (i.e. data leak) in otherwise solid app? Yes. So, this bug is a security issue.

[rvagg]

TypedArrays not being zero-filled in some cases (especially those that are triggerable by user actions) is a security issue no matter how one views it.

In the browser, that's why they are zero-filled by default in the browser because it's a sandbox, which Node is not and which is we don't zero-fill buffers, this is a break in assumptions and therefore a bug. By "doing something" I'm saying that you'd need to get the TypedArray data out of the server, so it's indeed an extra step, just because it exists in memory on the server doesn't make it an exploit.

[trevnorris]

Note that 1-2 do not necessary have to be done in a single request, an attacker could launch two parallel tasks — one that creates zero-length Buffers and one that retrives the data.

If the attacker had the ability to launch tasks to perform this type of operation then they could just check the contents of a Buffer. If the attack were along the lines of the http module then this would be near impossible because a Buffer would be instantiated by core internals, which would have reset the flag. Remember, the calls have to precisely follow each other for this to work.

Bug patches, even non armageddon security patches, are often not released immediately after patching. Based on the practical and possible impact this has waiting until Monday shouldn't be an issue.

[trevnorris]

I might be willing to concede this is a security issue based on a hypothetical and highly unlikely scenario along the lines of the dev partially filling a typed array with a crypto key, and depending on the remaining bits to be zero filled. Somehow then allowing false identification to the server. Did I mention the planets would need to be aligned for this to work?

So even though it might possibly be a security patch, the impact doesn't come close enough to warrant releasing today instead of Monday.

[rvagg]

Here's my main concern here: we're at the beginning of converged Node, effectively we're proving ourselves as the new crew that's shipping Node.js and we have many eyes on us, many of whom are excited about what we're now doing, some of whom are wanting us to fail but even more who are watching on the sidelines waiting to see how it pans out–many of these are enterprise users of Node and prospective enterprise users of Node who are an important audience for the sustainability of our platform. So, what we have here is a balancing act between the perception of being cowboys with code, vs cowboys with security issues. Having a 4.1.0 9 days after 4.0.0 is going to be a big enough deal for many people, having a 4.1.1 straight after 4.1.0 is going to be read by a lot of people that we can't be trusted to ship quality code.

With regard to this specific issue, it's my personal judgement that it's a non-trivial bug, something we should get fixed as soon as practical, but it's far from a clear security exploit vector, i.e. it's very much in the realm of hypothetical. So, weighing that against the cost of releasing a new version ASAP (i.e. releases aren't free, they have a cost associated with them for the reasons outlined above), combined with the hassles of Friday & weekend releases suggest that we should just defer until early next week.

But I'm also inviting critique of all of this, this certainly isn't a final say, I can only suggest. I'm happy to change my mind in light of evidence or expertise to the contrary and even without that others' can push for a release if they think it's important enough.

[rvagg]

Combine #2931 with #2945 and we have an interesting 4.1.1 that's worth making a bit of noise about.

[ChALkeR]

Note that 1-2 do not necessary have to be done in a single request, an attacker could launch two parallel tasks — one that creates zero-length Buffers and one that retrives the data.

If the attacker had the ability to launch tasks to perform this type of operation then they could just check the contents of a Buffer. If the attack were along the lines of the http module then this would be near impossible because a Buffer would be instantiated by core internals, which would have reset the flag. Remember, the calls have to precisely follow each other for this to work.

@trevnorris You are wrong here. I explained it above.

[kzc]

Meta release methodology comment: This bug fix aside, before merging any feature to the stable release shouldn't it sit on master or the unstable release branch for at least a week? This Buffer optimization was first introduced a few days before release and wasn't really used in the wild before going into stable. It is very easy to miss subtleties in code reviews.

But then there's the counter argument that no one would actually test it unless it was an official release.

Related question - is iojs 3.x or node 5.x considered to be the latest "unstable" release branch? Or just master?

[piscisaureus]

+1 on deferring to next week, and rolling it up with a few other fixes.

[ChALkeR]

@piscisaureus I sent you an example over gitter =).
@rvagg @trevnorris I sent you an example over email.

[piscisaureus]

@ChALkeR I've seen that you have written a server where this bug has security implications; nonetheless, it seems a little far-fetched to expect that any actual servers are currently vulnerable. Therefore I don't think we actually do anyone a service by rushing out a release before or during the weekend.

Nobody is proposing that we defer releasing a fix for months or even weeks - @rvagg is proposing to wait a couple of days - that seems entirely reasonable.

[ChALkeR]

@piscisaureus Did you check the code? The only unlikely thing that it does is leaking an Uint8Array to the client. Are you sure that there are no actual servers that do that?

But ok, I understand your reasoning and will not escalate this discussion further.

@rvagg

be also creating zero-length buffers

This code is enough to trigger that:

http.createServer(function(req, res) {
    var chunks = [];
    req.on('data', function(chunk) {
        // chunk is a Buffer
        chunks.push(chunk);
    });
    req.on('end', function() {
        // This is a common way of collecting the request body.
        var data = Buffer.concat(chunks);
        res.end();
    });
});

Reading an empty file is also enough.

[trevnorris]

@kzc You're correct that it should have baked in master a little while longer. Initial reason I wanted to pop it in is because we are hurting for performance with Buffers now that we're using Typed Arrays. Conceptually the change is straight forward, so I didn't see a big risk factor as I have with other performance changes I've made in the past.

@ChALkeR My point is the practicality of the exploit. I have a hard time believing this sort of thing exists in the wild. One reason being the streaming a string of null bytes to the client. As I said, it's theoretically possible but highly unlikely to exist in the wild. Also with your example code it depends on a Typed Array being created as the next allocation operation. The practical timing of that is also highly unlikely.

[vielmetti]

I know enough creative security people, @trevnorris , that "practicality of the exploit" is not usually an impediment.

Is there a test case in the test suite that triggers this defect? That would be a nice first step.

[trevnorris]

@vielmetti There's a test included in the commit that triggers the issue. My comment about practicality is to address weighing the need to rush out a build today vs waiting until Monday. IMO the statistical likelihood of this being exploited does not outweigh waiting for a few days before releasing a new build.

[misterdjules]

I would like to add #2959 to that release as well.

[ChALkeR]

Also with your example code it depends on a Typed Array being created as the next allocation operation. The practical timing of that is also highly unlikely.

Not true. On a high-loaded server, this is very likely even by chance (not only intentionally caused by an attacker). Just firing many TypedArray creation operations and an endpoint like in #2943 (comment) will cause errors.

[sathishsoundharajan]

Is it possible to have this release with latest version of npm.. Now that it is out of beta ?https://github.com/npm/npm/blob/master/CHANGELOG.md

[orangemocha]

npm@3 out of beta! I am really eager to get that integrated, as it fixes a long-standing problem with long nested paths on Windows. But would such a big change really be appropriate for a patch release?

[Fishrock123]

But would such a big change really be appropriate for a patch release?

Not really. I think we already agreed it will go into node@5?

[mikeal]

Not really. I think we already agreed it will go into node@5?

Yup, it has to land in a major. It should land on master, along with the new v8 I saw shoot by.

[Fishrock123]

Updated OP with current commit list. Not sure what semver e0c3d2a is.

[ChALkeR]

@Fishrock123 Changes in e0c3d2a are in files that are not packaged, they are used only by the tests. I'd say that's fine in a patch release.

[rvagg]

I've had to take a few days properly off and am only just resyncing with the inevitable massive backlog that accrues when I do that and haven't been able to put anything for this together today like I intended. I'll attempt to work on it tomorrow if I get enough time and unless someone else in @nodejs/release gets to it first. We probably need to take a good look at what else on master can be pushed out for this one too, although I'd personally prefer if we kept it to a semver-patch bump for simpler messaging.

[thefourtheye]

Any takers for #2966, #2967, #2968?

• doc: mention the behaviour if URL is invalid #2966 is about documenting a weird behavior we have in the current HTTP and HTTPS modules.
• http{s}: fix connecting to localhost if URL is invalid #2967 is about fixing the above mentioned weird behavior instead of documenting it. (We need to decide if this will be a major change or not.)
• repl: backslash bug fix #2968 is about a REPL bug which bites anyone who use backslash in string literal. There has been a report that someone copy pasted the example in our site in REPL and it errored. So pushing this fix out sooner might be a good thing.

[rvagg]

Continued @ #2995