Security Review: Review the code and find the bugs that would otherwise pass a functional code review or tests, but have serious security flaws.
Here’s some code that handles the a two-factor login. There are several bugs in here, and not all of them are strictly security related. Have a look through the code and see if you can find the vulnerabilities.
Sessions may not have been the best choice.
Random hint here
Every time a user attempts and auth action (login, two-factor) the
auth_attempted() function is called. The intent is to rate limit auth actions to 5 attempts and then timelock the user from trying again for an hour. The issue is that it relies on
This isn’t a critical vulnerability in that it allows an easy bypass into the system, so long as the password is implemented securely, the most severe effect is that it allows brute forcing.
The more effective method of rate limiting is to lock out the user based on some piece of information they can’t control, or have difficulty changing. A usual candidate for websites is by IP address.
The second and much worse vulnerability exists in the handling of the two-factor authorization.
Before we go any further let’s remember vulnerability 1, as it applies to 2FA as well. Since the rate limiting is effectively useless, this means that the 2FA is susceptible to brute-force. Now this is a real problem as there are only 1,000,000 2FA codes, and that is well within the realm of brute-forcible.
The attacker would still be limited to five attempts at the 2FA before having to reset their session and submit the username/password again, at which point another 2FA code would be generated and sent to the real user. Hopefully when the real users gets flooded with 2FA codes they’d notice something is up, but that’s hardly something we could rely on.
Suppose however the rate limiting worked correctly and we needed to attack the 2FA some other way. Let’s have a look at the
generate_2fa() function, it:
- sets the 2FA code expiry for 1 minute
- seeds the random number generator (with the time)
- generates a random 000000 – 999999 2FA code
The issue here is the random number generator. This code is using the
rand() function which is not a secure pseudo-random number generator; the implementation is deterministic. Even the PHP docs say not to use this for crypto purposes!
Caution This function does not generate cryptographically secure values, and should not be used for cryptographic purposes.https://php.net/rand
Furthermore, the random number generator has a predictable seed:
time(). Determining the server time is trivial as most servers keep fairly accurate time.
To bypass the 2FA an attacker need only make note of the exact time they login with the username/password (as this is when the 2FA code is generated). Since
rand() is deterministic, it can be replicated locally and seeded with the same time value. Et Viola! So long as the attacker had the correct time, they’ll have the valid 2FA code.
What’s even worse is that
time() only has second resolution, which affords an attacker a generous window. Had the RNG been seeded with
microtime() it would have been far more difficult, but still not secure.
Confusing sessions being opaque with being persistent is an easy mistake to make, and is something many developers may mistakenly do. Understanding how they’re implemented is important. Sure, a user may not have access to the session data, but that doesn’t mean that a user will always be associated with the same session.
The other important bit is that not all random is equal. pseudo-random numbers are not suitable for crytographic purposes as they are deterministic ie. they can be predicted. Ensuring a random number generator is cryptographically secure is very important if it is to be used for crytographic purposes.