Session Security and uid2

[skeggse]

@visionmedia switched the csrf and session middleware over to the uid2 module in 35c8f7b. Unless I'm mistaken, the uid2 module uses crypto.pseudoRandomBytes to generate the identifier, which according to the Node.js crypto documentation "Generates non-cryptographically strong pseudo-random data."

It seems like sessions should be cryptographically random, as they may protect highly sensitive information. Sometimes sessions contain identifiers like IP addresses which can mitigate session hijacking, but an IP address is not necessarily unique to each user.

Looking at some related stackoverflow material, it looks like it might not matter a whole lot, but for the sake of forward-compatibility it makes the most sense to use randomBytes, not pseudoRandomBytes.

@pesho in #881:

[the session secret] definitely has to be securely random.

I guess what I'm saying is, it doesn't look like the uid2 module uses the right crypto function, but it's not on a forum that makes it easy to discuss this issue (okay, well it does exist on GitHub, but it's not active). Furthermore, the uid2 module does not purport to generate secure identifiers, so it might not be the right fit, especially because using pseudoRandomBytes was a conscious decision.

At least for sessions, I don't think it makes sense use uid2, and we should use "something else" instead.

[pesho]

In practice, pseudoRandomBytes() and randomBytes() are exactly the same on all platforms which matter. The only situation when randomBytes() would block is when the OpenSSL CSPRNG hasn't been seeded with at least 128 bits of entropy. However, OpenSSL automatically seeds itself on startup on Windows (using CryptGenRadom) and on Linux and all unices which support /dev/urandom (using /dev/urandom). So I wouldn't bother about this. PseudoRandomBytes() is just as secure in practice.

The uid2 module had other issues when I looked at it before. It doesn't generate characters with uniform distribution (i.e. some characters are more likely to occur than others).

[skeggse]

The uid2 module had other issues when I looked at it before. It doesn't generate characters with uniform distribution (i.e. some characters are more likely to occur than others).

See coreh/uid2#3, the proposed fix to that doesn't even fix it.

In practice, pseudoRandomBytes() and randomBytes() are exactly the same on all platforms which matter. The only situation when randomBytes() would block is when the OpenSSL CSPRNG hasn't been seeded with at least 128 bits of entropy. However, OpenSSL automatically seeds itself on startup on Windows (using CryptGenRadom) and on Linux and all unices which support /dev/urandom (using /dev/urandom). So I wouldn't bother about this. PseudoRandomBytes() is just as secure in practice.

In practice, yes. In theory, and as far as the documentation is concerned, pseudoRandomBytes is less secure. I'm simply questioning whether it's wise to use a function which is documented against what we're using it for. As for blocking, that should only affect a small subset of platforms (as you said) and on those it does callbacks are your friend, though that would likely become a non-problem seconds after starting the Node process.

Also TIL Ctrl+Enter Submits the comment.

[pesho]

In practice, yes. In theory, and as far as the documentation is concerned, pseudoRandomBytes is less secure. I'm simply questioning whether it's wise to use a function which is documented against what we're using it for. As for blocking, that should only affect a small subset of platforms (as you said) and on those it does callbacks are your friend, though that would likely become a non-problem seconds after starting the Node process.

With regards to theory, my 2c: On platforms where OpenSSL doesn't auto-seed, the developer already has to implement his own secure seeding mechanism (which usually means an entropy-gathering daemon). Unless this is done, randomBytes() will block forever (or return errors forever, I'm not sure). Whether this is preferable to generating non-secure randoms in these situations, I don't know.

[skeggse]

On platforms where OpenSSL doesn't auto-seed, the developer already has to implement his own secure seeding mechanism (which usually means an entropy-gathering daemon). Unless this is done, randomBytes() will block forever (or return errors forever, I'm not sure).

I'm assuming that means that on said platforms pseudoRandomBytes will not block and therefore be completely insecure, yes? Some developers would be confused because they would see session conflicts and in trying to dig into the issue would discover that all their sessions are the same. That would be an odd bug to track down.

[pesho]

I'm assuming that means that on said platforms pseudoRandomBytes will not block and therefore be completely insecure, yes? Some developers would be confused because they would see session conflicts and in trying to dig into the issue would discover that all their sessions are the same. That would be an odd bug to track down.

Yes. PseudoRandomBytes() would work, but be predictable. RandomBytes() wouldn't work at all.

My personal opinion is that the best course of action would be to keep working, but print a scary warning message on stderr that the random generator needs to be seeded. The proper place to implement this would be Node core, though.

[skeggse]

The proper place to implement this would be Node core, though.

Definitely.

On a related note, it seems like the documentation should reflect the nuances for randomBytes and pseudoRandomBytes. Assuming, of course, that they are intentional. I would be much happier using pseudoRandomBytes if these nuances were documented, rather than assuming they will stay that way. Relying on undocumented features doesn't really seem like a good idea. I'll open a new issue on Node's repository that references this issue and nodejs/node-v0.x-archive#4907.

[jonathanong]

@skeggse what are you looking to do? i don't think this is an issue because we're probably talking about 10e-30 vs 10e-32 chance of collision (i don't know the exact numbers).

most of the current issues in connect are sessions. i'd prefer if someone created a robust and customizable session module and have connect use it with sane defaults. would get rid of so many issues!

[skeggse]

what are you looking to do?

My wish is that we used the tool that makes sense from a documentation perspective. As indutny says, "we might want to change implementation in future." Relying on undocumented functionality in the code hardly seems like a good idea.

i'd prefer if someone created a robust and customizable session module and have connect use it with sane defaults. would get rid of so many issues!

Now you're talking. I'll open up a new issue for that, instead. Maybe we can discuss what the new module should do and look like--plenty of issues to draw inspiration from!