Is it safe to use System.Random to generate session tokens?

[SmartHypercube]

yesod/yesod-core/src/Yesod/Core/Dispatch.hs

Line 96 in 5d8566a

defaultGen = Random.getStdRandom Random.next

yesod/yesod-core/src/Yesod/Core/Internal/Request.hs

Line 132 in 5d8566a

Nothing -> Right $ fmap Just . randomString 40

Yesod currently uses System.Random as the default random number generator, and uses this to generate session tokens. This looks very dangerous to me, because System.Random is only a pseudo-random number generator and has not been designed with security in mind. An attacker may easily collect enough information about the internal state of the generator and predict all future outputs.

I suggest using System.Entropy.

[snoyberg]

That's probably a good call. Are you interested in sending such a pull request?

[SmartHypercube]

I'd like to. However, I still have some confusion about session logic:
Is this token (reqToken) directly exposed to end user? Or is it encrypted and signed using server's key before sending to users? Is the answer depends on session backend?

If it is exposed to end user and used for authentication, it should be generated using a secure entropy source and has at least 128 bits (I would suggest using 256 bits for all kinds of secrets).
If it is encrypted and signed before sending to users, we do not need a token here. Simply an incrementing integer will do. In this case I suggest explaining the security considerations in the comment.

[SmartHypercube]

Oh, I just realized this token is not session token. Yesod core does not have the "session token" concept. This is CSRF token and only used by defaultCsrfMiddleware...
Given that this middleware is optional, isn't it strange to include the CSRF token in YesodRequest? A better design may be storing CSRF tokens in session data. The token generation logic can also be moved into setCsrfCookieWithCookie where it belongs.
CSRF tokens should still be securely generated though. It is user-facing and not protected by encryption.

[snoyberg]

I don't really remember the original design around CSRF tokens, and I'm not particularly interested in changing the way it works right now unless there's a concrete reason. Improving how they're generated for proper protection does still seem worthwhile though.