Skip to main content
Code Review

Return to Answer

replaced http://il1.php.net with https://www.php.net
Source Link
Community Bot
Community Bot
  • 1

I can think of several things that can be improved:

  1. Separate logic from presentation
  2. Don't use mysql_*
  3. MD5 for hashing is not safe
  4. Trying to escape/validate everything the same way is impractical and dangerous.

Unrelated to security:

  1. Don't use tables for layout

Now the explanations:

Separate logic from presentation:

Your logic (processing) and presentation (HTML, etc) should not be on the same page. They should be on different parts of your application. So for example, separate the part where you handle the database, and the part where you display the results:

<form action="process_registrations.php" method="post" ...>

And then handle the registration on process_registrations.php, then after processing is done, redirect to the correct page ("Thank You!", "Error!" whatever).

Don't use mysql_*

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

Please, for the love of God, stop.

MD5 for hashing is not safe

MD5 was shown to be trivially broken by brute force with modern hardware. Also, you are not using a salt, which makes your code vulnerable to Rainbow Tables attacks.

If you are using 5.5 or above, use password_hash()password_hash() and password_verify()password_verify().

If you are not using 5.5 or above, and you are using 5.3 or above, use the Password Compat library by ircmaxell (who is the same guy who wrote the password_* functions for php 5.5), and use the same functions.

If you are using 5.2 or below, shame on you upgrade immediately, no excuses.

Trying to escape/validate everything the same way is impractical and dangerous.

Each field has its own validation rules. You can't validate all of the fields the same way. Your GetSQLValueString function is redundant and dangerous. Use prepared statements instead of escaping for database. Also, check validation rules specifically for each field.

Don't use tables for layout

We are in 2014. Tables for layout were great in the 90s when no better alternatives existed. We have better alternatives today.

All in all, you have a lot of way to go. I'm not saying anything about "Start using OOP!" or "You should encapsulate your code!" But those are the major concerns with your code.

I can think of several things that can be improved:

  1. Separate logic from presentation
  2. Don't use mysql_*
  3. MD5 for hashing is not safe
  4. Trying to escape/validate everything the same way is impractical and dangerous.

Unrelated to security:

  1. Don't use tables for layout

Now the explanations:

Separate logic from presentation:

Your logic (processing) and presentation (HTML, etc) should not be on the same page. They should be on different parts of your application. So for example, separate the part where you handle the database, and the part where you display the results:

<form action="process_registrations.php" method="post" ...>

And then handle the registration on process_registrations.php, then after processing is done, redirect to the correct page ("Thank You!", "Error!" whatever).

Don't use mysql_*

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

Please, for the love of God, stop.

MD5 for hashing is not safe

MD5 was shown to be trivially broken by brute force with modern hardware. Also, you are not using a salt, which makes your code vulnerable to Rainbow Tables attacks.

If you are using 5.5 or above, use password_hash() and password_verify().

If you are not using 5.5 or above, and you are using 5.3 or above, use the Password Compat library by ircmaxell (who is the same guy who wrote the password_* functions for php 5.5), and use the same functions.

If you are using 5.2 or below, shame on you upgrade immediately, no excuses.

Trying to escape/validate everything the same way is impractical and dangerous.

Each field has its own validation rules. You can't validate all of the fields the same way. Your GetSQLValueString function is redundant and dangerous. Use prepared statements instead of escaping for database. Also, check validation rules specifically for each field.

Don't use tables for layout

We are in 2014. Tables for layout were great in the 90s when no better alternatives existed. We have better alternatives today.

All in all, you have a lot of way to go. I'm not saying anything about "Start using OOP!" or "You should encapsulate your code!" But those are the major concerns with your code.

I can think of several things that can be improved:

  1. Separate logic from presentation
  2. Don't use mysql_*
  3. MD5 for hashing is not safe
  4. Trying to escape/validate everything the same way is impractical and dangerous.

Unrelated to security:

  1. Don't use tables for layout

Now the explanations:

Separate logic from presentation:

Your logic (processing) and presentation (HTML, etc) should not be on the same page. They should be on different parts of your application. So for example, separate the part where you handle the database, and the part where you display the results:

<form action="process_registrations.php" method="post" ...>

And then handle the registration on process_registrations.php, then after processing is done, redirect to the correct page ("Thank You!", "Error!" whatever).

Don't use mysql_*

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

Please, for the love of God, stop.

MD5 for hashing is not safe

MD5 was shown to be trivially broken by brute force with modern hardware. Also, you are not using a salt, which makes your code vulnerable to Rainbow Tables attacks.

If you are using 5.5 or above, use password_hash() and password_verify().

If you are not using 5.5 or above, and you are using 5.3 or above, use the Password Compat library by ircmaxell (who is the same guy who wrote the password_* functions for php 5.5), and use the same functions.

If you are using 5.2 or below, shame on you upgrade immediately, no excuses.

Trying to escape/validate everything the same way is impractical and dangerous.

Each field has its own validation rules. You can't validate all of the fields the same way. Your GetSQLValueString function is redundant and dangerous. Use prepared statements instead of escaping for database. Also, check validation rules specifically for each field.

Don't use tables for layout

We are in 2014. Tables for layout were great in the 90s when no better alternatives existed. We have better alternatives today.

All in all, you have a lot of way to go. I'm not saying anything about "Start using OOP!" or "You should encapsulate your code!" But those are the major concerns with your code.

Source Link
Madara's Ghost
Madara's Ghost
  • 4.8k
  • 26
  • 46

I can think of several things that can be improved:

  1. Separate logic from presentation
  2. Don't use mysql_*
  3. MD5 for hashing is not safe
  4. Trying to escape/validate everything the same way is impractical and dangerous.

Unrelated to security:

  1. Don't use tables for layout

Now the explanations:

Separate logic from presentation:

Your logic (processing) and presentation (HTML, etc) should not be on the same page. They should be on different parts of your application. So for example, separate the part where you handle the database, and the part where you display the results:

<form action="process_registrations.php" method="post" ...>

And then handle the registration on process_registrations.php, then after processing is done, redirect to the correct page ("Thank You!", "Error!" whatever).

Don't use mysql_*

Please, don't use mysql_* functions in new code. They are no longer maintained and are officially deprecated. Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

Please, for the love of God, stop.

MD5 for hashing is not safe

MD5 was shown to be trivially broken by brute force with modern hardware. Also, you are not using a salt, which makes your code vulnerable to Rainbow Tables attacks.

If you are using 5.5 or above, use password_hash() and password_verify().

If you are not using 5.5 or above, and you are using 5.3 or above, use the Password Compat library by ircmaxell (who is the same guy who wrote the password_* functions for php 5.5), and use the same functions.

If you are using 5.2 or below, shame on you upgrade immediately, no excuses.

Trying to escape/validate everything the same way is impractical and dangerous.

Each field has its own validation rules. You can't validate all of the fields the same way. Your GetSQLValueString function is redundant and dangerous. Use prepared statements instead of escaping for database. Also, check validation rules specifically for each field.

Don't use tables for layout

We are in 2014. Tables for layout were great in the 90s when no better alternatives existed. We have better alternatives today.

All in all, you have a lot of way to go. I'm not saying anything about "Start using OOP!" or "You should encapsulate your code!" But those are the major concerns with your code.