search

krakenjs / lusca

Application security for express apps.
Other
1.79k stars 123 forks source link

Crypto could be improved for generating CSRF tokens #43

Open aredridel opened 9 years ago

aredridel commented 9 years ago

https://github.com/krakenjs/lusca/blob/master/lib/token.js#L18 in particular uses pseudoRandomBytes, which generates low quality output until warmed up.

Kraken should probably warm up randomBytes at app boot (not emitting the start event until it gets data) so that the PRNG is seeded well. At that point, pseudoRandomBytes is the same as randomBytes, and randomBytes will never block/throw anyway.

At https://github.com/krakenjs/lusca/blob/master/lib/token.js#L35, we generate and use a salt, but it's combined with the secret by appending, not HMAC, so it would possibly be extensible; that said, a salt should get us exactly nothing, and we should use a fully random token since we're just doing an equality check anyway. There's no use in salting to prevent a rainbow table, because nobody generates a rainbow table for random data anyway; that's for passphrases, not randomness.

jasisk commented 9 years ago

Cool with the pseudoRandomBytes stuff (though it's still better than the original Express implementation which used Math.random, iirc).

Regarding the the concatenation, we just emulated the Express implementation. I'd support us breaking that in a major release if need be.