Remove async version of csrfToken() & use a fast non-blocking PRNG #883

Merged
merged 1 commit into from Sep 2, 2013

Conversation

Projects
None yet
5 participants
Contributor

pesho commented Sep 2, 2013

As discussed in #881: Removed async version of csrfToken(). Used Math.random() as PRNG for the salt generation (which removes the need for async).

Removed async version of csrfToken(). Used a fast non-blocking PRNG f…
…or the salt generation (which removes the need for async).

tj added a commit that referenced this pull request Sep 2, 2013

Merge pull request #883 from pesho/master
Remove async version of csrfToken() & use a fast non-blocking PRNG

@tj tj merged commit a287186 into senchalabs:master Sep 2, 2013

1 check passed

default The Travis CI build passed
Details
Member

tj commented Sep 2, 2013

thanks man

talg commented Nov 8, 2013

Hey guys, I've been trying to understand the security of this thing and encountered a few issues.

issue 1. the update for the node documentation would be really helpful, as someone noted, the docs currently say.

crypto.pseudoRandomBytes(size, [callback])#
Generates non-cryptographically strong pseudo-random data. The data returned will be unique if it is sufficiently long, but is not necessarily unpredictable. For this reason, the output of this function should never be used where unpredictability is important, such as in the generation of encryption keys.

if this is actually just calling down to /dev/urandom, it would be really helpful to know that, initially when I read the code in uid that was calling this, then the docs', I was concerned. so, yeah... nuff said... I believe this was already raised in another thread, either explanatory comments here or in the node crypto stuff would be great.

issue 2. Your taking a non-crytographically strong function (math.Random) with a long period, then essentially adding that to your secret (so the initial output is about 192 bits worth of quality random) , and getting something cryptographically strong looking by running it through your secure hash function. And, the reason your doing this is to provide a short lived, non-repeating, cryptographically strong token with minimal cpu overhead. But, its still going to be periodic, with the period of the non-strong function being used. So, what is the period of Math.Random in V8? Also, there is the issue of birthday attacks to consider.

issue 3. I'm not sure what your threat model is, I saw BREACH attacks mentioned somewhere. What is this code supposed to be accomplishing? How frequently are collisions in the token stream considered acceptable under your threat model?

In case this is a concern, I think there are a couple ways you could go to strengthening this, such as choosing a efficient PRF with a longer period and adding a counter and refreshing your secret periodically.

My apologies if I'm overlooking something.

Contributor

jonathanong commented Nov 8, 2013

from what i remember from the BREACH attack, the CSRF tokens do not have to be cryptographically strong. they just have to be different on every request so that every request is different (i.e. GETing the same page twice should give you different responses). as long as your session is secure, it should be fine.

Contributor

dougwilson commented Nov 8, 2013

Right, @jonathanong, if the CSRF token is different for every request, BREACH cannot function (it also needs the responses to be compressed, so if the server never compresses responses, it doesn't matter, but almost all servers these days compress responses). The CSRF itself doesn't have to be different with every response, just if the CSRF token is not changed with every response, then the site cannot have any page that is always the same with every response, so it's easier just to change the CSRF every time.

Contributor

pesho commented Nov 8, 2013

@talg, regarding issue 1, pseudoRandomBytes() is just as secure as randomBytes() on all *nixes with /dev/urandom and on Windows (i.e. all platforms which really matter). On other platforms, pseudoRandomBytes() would be insecure, but randomBytes() wouldn't work at all without complex seeding code (like an entropy-gathering daemon).

(pseudo)RandomBytes doesn't actually call /dev/urandom each time. It's read only once on startup, to initialize OpenSSL's own CSPRNG with 128 bits of entropy.

Regarding issue 2, V8's Math.random() period is 2^32 IIRC. But that shouldn't be a problem when used for salts.

talg commented Nov 8, 2013

@jonathanong @dougwilson - if our goal is for the token to be unique in every request, what we want is a counter.

in psuedo code.

getToken() {
if counter not initialized or counter == MAX_INT {
secret = getrandombytes()
counter = 0
}
token = sha(secret + counter) //assume + means string concatenation, as in javascript.
counter = counter + 1
return token
}

math.Random is trying to produce a sequence with certain statistical properties which is not what we are after, I assume.

talg commented Nov 8, 2013

@pesho Cool, that was my understanding. Is there some way to get the node crypto documentation updated to reflect this so that folks (like me) trying to understand crypto related code won't have to scratch our heads and go digging deeper.

Contributor

jonathanong commented Nov 8, 2013

yeah, but where are you going to store the counter? that's an extra db get/retrieve or cookie save on every request. better just to make a random salt. plus, a counter is predictable.

talg commented Nov 8, 2013

math.Random is basically just a 32bit shared variable, being incremented in a pseudorandom but still deterministic order i.e. its also "predictable" - just more cumbersome to reason about than a counter, at least a 64bit shared counter will "never" produce duplicates. That said, I don't fully grok how BREACH works.

I would like to understand why you think your counter measure is sufficient and how you reached this conclusion. In general math.Random is not very random in any meaningful sense in a security context, so it tends to raise red flags.

Member

tj commented Nov 8, 2013

I think you guys might be chasing a unicorn :p, IMO just because it's pseudo random doesn't mean it's going to be trivial to attack with BREACH, if at all possible. I can't imagine how you'd reverse engineer that with even Math.random()'s entropy

talg commented Nov 9, 2013

I think I agree. It would be nice to have a few more comments explaining that the salt is to prevent BREACH attacks, and a brief rationale for its effectiveness.

talg commented Nov 9, 2013

thanks all for the quick feedback.

Member

tj commented Nov 9, 2013

yeah true some docs would be good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment