Return to Answer

CSRF tokens. You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. See, e.g., Is a rand from glibc's rand secure for a login key?, What are the requirements for a random number generator to a be safe to use in cryptography?, Is a rand from /dev/urandom secure for a login key?.

Failed logins. You ban the user for one month after 10 failed login attempts. This is probably not such a great idea, because it makes it too easy for "griefers" to lock someone out of their account for a month. I suggest reading the following questions on this site: Why do sites implement locking after 3 failed password attempts?, Is denying login after incorrect attempts ineffective?, Appropriate strategy for preventing brute forcing of logins?, How do I secure my login page?, Why re-verify with CAPTCHA on failed form entry?.

CSRF tokens. You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. See, e.g., Is a rand from glibc's rand secure for a login key?, What are the requirements for a random number generator to a be safe to use in cryptography?, Is a rand from /dev/urandom secure for a login key?.

Failed logins. You ban the user for one month after 10 failed login attempts. This is probably not such a great idea, because it makes it too easy for "griefers" to lock someone out of their account for a month. I suggest reading the following questions on this site: Why do sites implement locking after 3 failed password attempts?, Is denying login after incorrect attempts ineffective?, Appropriate strategy for preventing brute forcing of logins?, How do I secure my login page?, Why re-verify with CAPTCHA on failed form entry?.

Clickjacking. Your diagram on preventing clickjacking is awfully vague. I presume you are doing client-side framebusting (using Javascript that runs on the client). Be warned that this is extremely error-prone and most people who write their own framebusting code implement something that is subtly broken. One academic paper surveyed the top 500 most widely used sites (in 2010), and found that every single site that used framebusting, implemented it in a way that was insecure. The attacks were subtle and non-obvious, so the developers probably thought their code was fine -- when actually it was flawed.

If your concern is about forceful browsing attacks, I would suggest simply ensuring that you use proper CSRF defense at all places in the site, including on the checkout.php action.

You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. (Also, you are calling mt_rand(5,15), which means your random number is from the range 5..15. In other words, you have less than 4 bits of randomness. This is totally inadequate and completely broken. Hopefully that was a typo.)

Overall. This is much better. But I still have some comments --

Clickjacking. Your diagram on preventing clickjacking is awfully vague. I presume you are doing client-side framebusting (using Javascript that runs on the client). Be warned that this is extremely error-prone and most people who write their own framebusting code end up with something that is subtly broken. One academic paper surveyed the top 500 most widely used sites (in 2010), and found that every single site that used framebusting, implemented it in a way that was insecure. The attacks were subtle and non-obvious, so the developers probably thought their code was fine -- when actually it was flawed.

If your concern is about forceful browsing attacks, I would suggest simply ensuring that you use proper CSRF defense at all places in the site, including on the checkout.php page and all form targets on that page.

CSRF tokens. You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. See, e.g., Is a rand from glibc's rand secure for a login key?, What are the requirements for a random number generator to a be safe to use in cryptography?, Is a rand from /dev/urandom secure for a login key?.

Also, you are calling mt_rand(5,15), which means your random number is from the range 5..15. In other words, you have less than 4 bits of randomness. This is totally inadequate and completely broken. Hopefully that was a typo, or maybe I misunderstood.

Failed logins. You ban the user for one month after 10 failed login attempts. This is probably not such a great idea, because it makes it too easy for "griefers" to lock someone out of their account for a month. I suggest reading the following questions on this site: Why do sites implement locking after 3 failed password attempts?, Is denying login after incorrect attempts ineffective?, Appropriate strategy for preventing brute forcing of logins?, How do I secure my login page?, Why re-verify with CAPTCHA on failed form entry?.

Cookies. Make sure you set the secure flag on all cookies (so that they will only be sent back over HTTPS connections). I suggest that you enable HTTP Strict Transport Security, to prevent sslstrip-style attacks.

General comment. A lot of the stuff you are building is bog-standard. In the future you might consider choosing a web application development framework that provides support for these mechanisms (e.g., secure session management, CSRF defense, authorization checks). Also I encourage you to look at OWASP's resources on web application security. They cover a lot of the topics you're asking about pretty well.

Clickjacking. Your diagram on preventing clickjacking is awfully vague. I presume you are doing client-side framebusting (using Javascript that runs on the client). Be warned that this is extremely error-prone and most people who write their own framebusting code implement something that is subtly broken. One academic paper surveyed the top 500 most widely used sites (in 2010), and found that every single site that used framebusting, implemented it in a way that was insecure. The attacks were subtle and non-obvious, so the developers probably thought their code was fine -- when actually it was flawed.

I suggest that you read the following paper, which outlines approaches that are flawed and also describes how to do framebusting properly:

• Busting frame busting: a study of clickjacking vulnerabilities at popular sites. Gustav Rydstedt, Elie Bursztein, Dan Boneh, and Collin Jackson. W2SP 2010.

Workflow enforcement. I did not understand your discussion about ensuring that checkout.php shouldn't be accessed before review.php. It is not clear to me why you list this as a security property, or what security value this adds. It does not sound like a security issue to me. Also, your mechanism may break having multiple copies of your site open in multiple tabs.

If your concern is about forceful browsing attacks, I would suggest simply ensuring that you use proper CSRF defense at all places in the site, including on the checkout.php action.

CSRF defense. Note that you need to use a CSRF token to protect all side-effecting actions. If your site is properly architected, this means that a CSRF token should be used to protect all POST requests.

Clickjacking. Your diagram on preventing clickjacking is awfully vague. I presume you are doing client-side framebusting (using Javascript that runs on the client). Be warned that this is extremely error-prone and most people who write their own framebusting code implement something that is subtly broken. One academic paper surveyed the top 500 most widely used sites (in 2010), and found that every single site that used framebusting, implemented it in a way that was insecure. The attacks were subtle and non-obvious, so the developers probably thought their code was fine -- when actually it was flawed.

I suggest that you read the following paper, which outlines approaches that are flawed and also describes how to do framebusting properly:

• Busting frame busting: a study of clickjacking vulnerabilities at popular sites. Gustav Rydstedt, Elie Bursztein, Dan Boneh, and Collin Jackson. W2SP 2010.

Workflow enforcement. I did not understand your discussion about ensuring that checkout.php shouldn't be accessed before review.php. It is not clear to me why you list this as a security property, or what security value this adds. It does not sound like a security issue to me. Also, your mechanism may break having multiple copies of your site open in multiple tabs.

If your concern is about forceful browsing attacks, I would suggest simply ensuring that you use proper CSRF defense at all places in the site, including on the checkout.php action.

CSRF defense. Note that you need to use a CSRF token to protect all side-effecting actions. If your site is properly architected, this means that a CSRF token should be used to protect all POST requests.

You use mt_rand() to generate the CSRF token. This is not secure and represents bad practice. You need to use a cryptographically strong PRNG (e.g., read from /dev/urandom), to ensure that the attacker cannot predict this value. mt_rand() is not cryptographically strong. (Also, you are calling mt_rand(5,15), which means your random number is from the range 5..15. In other words, you have less than 4 bits of randomness. This is totally inadequate and completely broken. Hopefully that was a typo.)