freckle / yesod-auth-oauth2

OAuth2 authentication for yesod
MIT License
71 stars 53 forks source link

RNG used for CSRF tokens is seeded using system time #132

Closed lf- closed 4 years ago

lf- commented 4 years ago

NOTE: I am not a cryptographer! Just someone studying this code to develop another auth library and found this mildly concerning.

I noticed that this package uses random using the StdGen interface, which after some further digging in the source to random, and initSMGen, it appears that the random numbers used for this are using an RNG seeded by the system time (admittedly including a picoseconds CPU time component). It might be possible to mount an offline attack against this.

I am not sure if the random generator is correct for this use case. mwc-random (also explicitly not cryptographically secure) or cryptonite may be more appropriate for the purpose, and neither uses system time as a seed except as a last-resort fallback.

pbrisbin commented 4 years ago

Thank you for bringing this up! I will switch to one of those other options as soon as possible.

It feels like some time should be dedicated to understanding this more deeply, but I'm not sure I have the bandwidth right now. I wouldn't want to be too alarmist about the versions in the wild, if there's no actual attack possible (or if successful attack is very unlikely), but I also don't want to silently fix an actual security issue without appropriate notification to users (or, for example, pulling versions down from Hackage). If anyone has thoughts or suggestions about this, please let me know.

lf- commented 4 years ago

I ended up implementing mine with base64 encoded randomness from the system provider via cryptonite, to absolutely make sure it's not a problem. getRandomBytes in an IO monad I think? :)

Same trouble on my side with the lack of background to appropriately evaluate options. I simply do not know if this attack is plausible or the scope of exposure, just that the RNG is definitely weak.

lf- commented 4 years ago

Also, mwc-random is not "cryptographically secure" but it's unclear whether that means it's still susceptible to hypothetical attacks, how many outputs do such possible attacks require, what's the exposure, etc? I'm not sure.

pbrisbin commented 4 years ago

Yeah, so I was (imperfectly) trying to assess things as:

The first point is clear enough to fix directly (which either option does), but the second point is nebulous. What is the delta between "cryptographically secure" and "sufficiently random"? I elected to take the more easily-dropped-in / fewer-transitive-deps option.

I'm less into that choice after thinking about it more, though. So maybe I'll slot cryptonite into that PR next and see how it goes.

lf- commented 4 years ago

I really wish there was a tiny package that just offered good bindings to the OS random infrastructure, which is what I think I heard the best practice is. It's unfortunate that the current ecosystem makes the wrong choice the easy choice due to the dependency graph and popularity issues with doing it right.

pbrisbin commented 4 years ago

good bindings to the OS random infrastructure

When looking at mwc-random, it said it reads /dev/urandom, which is definitely the best practice OS-wise.

That makes me feel pretty good about this package for this use-case actually :shrug:

It's unfortunate that the current ecosystem makes the wrong choice the easy choice due

Strong agree!

lf- commented 4 years ago

Yeah, they have their seeding done correctly but it is unknown whether there's weaknesses that let the seed be found by observing multiple requests. I seem to recall the best practice might have been using the OS random number generator for the entire token (what I did with cryptonite), reducing the possibility of implementation issues on the Haskell side.

pbrisbin commented 4 years ago

I seem to recall the best practice might have been using the OS random number generator for the entire token

That makes sense. I think you could do it (albeit ill-performantly) with either library. In my PR I would replace

withSystemRandom $ replicateM 30 $ ...

with

replicateM 30 $ withSystemRandom ...
lf- commented 4 years ago

You could use acquireSeedSystem directly: https://hackage.haskell.org/package/mwc-random-0.14.0.0/docs/System-Random-MWC-SeedSource.html

pbrisbin commented 4 years ago

Fixed (with cryptonite) in yesod-auth-oauth2-0.6.1.3.