HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Checking PHP security on my file

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
securityphpcheckingfile

Problem

I have created a PHP to change the user's login password:


 

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:

  • Separate logic from presentation



  • Don't use mysql_*



  • MD5 for 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.