Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

tim's answer covers the most important issues, but I would like to put a bit more emphasis.

First of all you mastered the first step towards secure code. You obviously know that you need to sanitize user input, that is already very good.

Input sanitation

#Input sanitation HoweverHowever, while your code may not have sanitation issues right now, it probably will not be once you extend/modify it. Arbitrarily throwing sanitation methods at input is not the solution. The solution is to use clean abstraction interfaces for output (e.g. template system) and database (e.g. prepared statements) and sanitize at the input to the abstraction. This way it is more feasible to actually get it right for non-trivial sites.

Randomness / hash

#Randomness / hash mt_rand is not suitable for security-related randomness. This, and suggested alternatives, are also documented. Note: It seems the alternatives suggested by the php manual are not easily available... [insert php rant here].

The arbitrary restriction to ~30 bits (1,000,000,000) is certainly not helpful. There is also no reason to hash this value here, and most certainly not by chaining two weak hash functions. Remember that hash functions map arbitrary size data to fixed size. They do not generate entropy. You can use a hash function if you have non-alphanumeric random data.

So lets say you generate a random token of sufficient entropy (30 bites are NOT!) and send that token to the user. Then you can store the hash of the token in the database. This way, when an attacker obtains a database dump somehow, he cannot reset passwords. This works the same way as with passwords, although its more important to apply it to passwords. You can use password_hash.

Protect against phishing

#Protect against phishing EmailsEmails with links are in general problematic, as both links (what is shown to the user vs. where the link goes to) and emails (sender address) can be easily manipulated. If you send an email to the user, especially when it contains a link to click on, it is customary to personally address him by name. This is at least some assurance, that it is not a phishing mail. It is of course not perfect as the name can often be devised from the email address.

An alternative to sending a link, is to send only the token (in your case hash) by mail and redirect the user to a form where he can enter the token directly after he requests the email.

Code structure

#Code structure AsAs Tim mentioned, your code structure should be improved for clarity. This also has security impact.

SQL efficiency

#SQL efficiency IfIf you do not intend to use the result of an SQL row, don't SELECT * it, but SELECT COUNT(*) instead.

Use example.com

#Use example.com TheThe domain example.com is reserved for this very use. Use it instead of a domain that is actually owned and used (domain.com).

tim's answer covers the most important issues, but I would like to put a bit more emphasis.

First of all you mastered the first step towards secure code. You obviously know that you need to sanitize user input, that is already very good.

#Input sanitation However, while your code may not have sanitation issues right now, it probably will not be once you extend/modify it. Arbitrarily throwing sanitation methods at input is not the solution. The solution is to use clean abstraction interfaces for output (e.g. template system) and database (e.g. prepared statements) and sanitize at the input to the abstraction. This way it is more feasible to actually get it right for non-trivial sites.

#Randomness / hash mt_rand is not suitable for security-related randomness. This, and suggested alternatives, are also documented. Note: It seems the alternatives suggested by the php manual are not easily available... [insert php rant here].

The arbitrary restriction to ~30 bits (1,000,000,000) is certainly not helpful. There is also no reason to hash this value here, and most certainly not by chaining two weak hash functions. Remember that hash functions map arbitrary size data to fixed size. They do not generate entropy. You can use a hash function if you have non-alphanumeric random data.

So lets say you generate a random token of sufficient entropy (30 bites are NOT!) and send that token to the user. Then you can store the hash of the token in the database. This way, when an attacker obtains a database dump somehow, he cannot reset passwords. This works the same way as with passwords, although its more important to apply it to passwords. You can use password_hash.

#Protect against phishing Emails with links are in general problematic, as both links (what is shown to the user vs. where the link goes to) and emails (sender address) can be easily manipulated. If you send an email to the user, especially when it contains a link to click on, it is customary to personally address him by name. This is at least some assurance, that it is not a phishing mail. It is of course not perfect as the name can often be devised from the email address.

An alternative to sending a link, is to send only the token (in your case hash) by mail and redirect the user to a form where he can enter the token directly after he requests the email.

#Code structure As Tim mentioned, your code structure should be improved for clarity. This also has security impact.

#SQL efficiency If you do not intend to use the result of an SQL row, don't SELECT * it, but SELECT COUNT(*) instead.

#Use example.com The domain example.com is reserved for this very use. Use it instead of a domain that is actually owned and used (domain.com).

tim's answer covers the most important issues, but I would like to put a bit more emphasis.

First of all you mastered the first step towards secure code. You obviously know that you need to sanitize user input, that is already very good.

Input sanitation

However, while your code may not have sanitation issues right now, it probably will not be once you extend/modify it. Arbitrarily throwing sanitation methods at input is not the solution. The solution is to use clean abstraction interfaces for output (e.g. template system) and database (e.g. prepared statements) and sanitize at the input to the abstraction. This way it is more feasible to actually get it right for non-trivial sites.

Randomness / hash

mt_rand is not suitable for security-related randomness. This, and suggested alternatives, are also documented. Note: It seems the alternatives suggested by the php manual are not easily available... [insert php rant here].

The arbitrary restriction to ~30 bits (1,000,000,000) is certainly not helpful. There is also no reason to hash this value here, and most certainly not by chaining two weak hash functions. Remember that hash functions map arbitrary size data to fixed size. They do not generate entropy. You can use a hash function if you have non-alphanumeric random data.

So lets say you generate a random token of sufficient entropy (30 bites are NOT!) and send that token to the user. Then you can store the hash of the token in the database. This way, when an attacker obtains a database dump somehow, he cannot reset passwords. This works the same way as with passwords, although its more important to apply it to passwords. You can use password_hash.

Protect against phishing

Emails with links are in general problematic, as both links (what is shown to the user vs. where the link goes to) and emails (sender address) can be easily manipulated. If you send an email to the user, especially when it contains a link to click on, it is customary to personally address him by name. This is at least some assurance, that it is not a phishing mail. It is of course not perfect as the name can often be devised from the email address.

An alternative to sending a link, is to send only the token (in your case hash) by mail and redirect the user to a form where he can enter the token directly after he requests the email.

Code structure

As Tim mentioned, your code structure should be improved for clarity. This also has security impact.

SQL efficiency

If you do not intend to use the result of an SQL row, don't SELECT * it, but SELECT COUNT(*) instead.

Use example.com

The domain example.com is reserved for this very use. Use it instead of a domain that is actually owned and used (domain.com).

Source Link
Zulan
  • 782
  • 3
  • 12

tim's answer covers the most important issues, but I would like to put a bit more emphasis.

First of all you mastered the first step towards secure code. You obviously know that you need to sanitize user input, that is already very good.

#Input sanitation However, while your code may not have sanitation issues right now, it probably will not be once you extend/modify it. Arbitrarily throwing sanitation methods at input is not the solution. The solution is to use clean abstraction interfaces for output (e.g. template system) and database (e.g. prepared statements) and sanitize at the input to the abstraction. This way it is more feasible to actually get it right for non-trivial sites.

#Randomness / hash mt_rand is not suitable for security-related randomness. This, and suggested alternatives, are also documented. Note: It seems the alternatives suggested by the php manual are not easily available... [insert php rant here].

The arbitrary restriction to ~30 bits (1,000,000,000) is certainly not helpful. There is also no reason to hash this value here, and most certainly not by chaining two weak hash functions. Remember that hash functions map arbitrary size data to fixed size. They do not generate entropy. You can use a hash function if you have non-alphanumeric random data.

So lets say you generate a random token of sufficient entropy (30 bites are NOT!) and send that token to the user. Then you can store the hash of the token in the database. This way, when an attacker obtains a database dump somehow, he cannot reset passwords. This works the same way as with passwords, although its more important to apply it to passwords. You can use password_hash.

#Protect against phishing Emails with links are in general problematic, as both links (what is shown to the user vs. where the link goes to) and emails (sender address) can be easily manipulated. If you send an email to the user, especially when it contains a link to click on, it is customary to personally address him by name. This is at least some assurance, that it is not a phishing mail. It is of course not perfect as the name can often be devised from the email address.

An alternative to sending a link, is to send only the token (in your case hash) by mail and redirect the user to a form where he can enter the token directly after he requests the email.

#Code structure As Tim mentioned, your code structure should be improved for clarity. This also has security impact.

#SQL efficiency If you do not intend to use the result of an SQL row, don't SELECT * it, but SELECT COUNT(*) instead.

#Use example.com The domain example.com is reserved for this very use. Use it instead of a domain that is actually owned and used (domain.com).

lang-php

AltStyle によって変換されたページ (->オリジナル) /