patternphpMinor
Checking PHP security on my file
Viewed 0 times
securityphpcheckingfile
Problem
I have created a PHP to change the user's login password:
Please check it out and tell me what should be changed to make it a more secure file or what I should do to learn how to make a secure PHP file in the future. I'd also like to know of the most dangerous practices to avoid when writing a PHP file that uses MySQL connections.
Quini-Mex CD JUAREZ
body {
background: url(fondo.jpg) !important;
background-attachment:fixed;
background-size:cover;
}
.ui-page, .ui-content, .ui-btn {
background: transparent;
}
Escriba su nueva contraseña
" onsubmit="return checkPassword(this);" data-ajax="false">
Contraseña:
Repita la Contraseña:
">
Regresar
Page Two
Content
Page Footer
Page Three
Content
Page Footer
Page Four
Content
Page Footer
Please check it out and tell me what should be changed to make it a more secure file or what I should do to learn how to make a secure PHP file in the future. I'd also like to know of the most dangerous practices to avoid when writing a PHP file that uses MySQL connections.
Solution
I can think of several things that can be improved:
Unrelated to security:
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:
And then handle the registration on
Don't use
Please, don't use
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
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
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
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.
- Separate logic from presentation
- Don't use
mysql_*
MD5for hashing is not safe
- Trying to escape/validate everything the same way is impractical and dangerous.
Unrelated to security:
- 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:
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.
Code Snippets
<form action="process_registrations.php" method="post" ...>Context
StackExchange Code Review Q#51785, answer score: 6
Revisions (0)
No revisions yet.