Checking PHP security on my file

Question

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

<?php require_once('Connections/conexion.php'); ?>
<?php
if (!function_exists("GetSQLValueString")) {
function GetSQLValueString($theValue, $theType, $theDefinedValue = "", $theNotDefinedValue = "") 
{
  if (PHP_VERSION < 6) {
    $theValue = get_magic_quotes_gpc() ? stripslashes($theValue) : $theValue;
  }

  $theValue = function_exists("mysql_real_escape_string") ? mysql_real_escape_string($theValue) : mysql_escape_string($theValue);

  switch ($theType) {
    case "text":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;    
    case "long":
    case "int":
      $theValue = ($theValue != "") ? intval($theValue) : "NULL";
      break;
    case "double":
      $theValue = ($theValue != "") ? doubleval($theValue) : "NULL";
      break;
    case "date":
      $theValue = ($theValue != "") ? "'" . $theValue . "'" : "NULL";
      break;
    case "defined":
      $theValue = ($theValue != "") ? $theDefinedValue : $theNotDefinedValue;
      break;
  }
  return $theValue;
}
}

$editFormAction = $_SERVER['PHP_SELF'];
if (isset($_SERVER['QUERY_STRING'])) {
  $editFormAction .= "?" . htmlentities($_SERVER['QUERY_STRING']);
}

if ((isset($_POST["MM_update"])) && ($_POST["MM_update"] == "form1")) {
  $updateSQL = sprintf("UPDATE tbusuarios SET password=%s WHERE idUsuario=%s",
                       GetSQLValueString(md5($_POST['password']), "text"),
                       GetSQLValueString($_POST['idUsuario'], "int"));

  mysql_select_db($database_conexion, $conexion);
  $Result1 = mysql_query($updateSQL, $conexion) or die(mysql_error());

  $updateGoTo = "clavecambiada.php";
  if (isset($_SERVER['QUERY_STRING'])) {
    $updateGoTo .= (strpos($updateGoTo, '?')) ? "&" : "?";
    $updateGoTo .= $_SERVER['QUERY_STRING'];
  }
  header(sprintf("Location: %s", $updateGoTo));
}

$colname_Recordset1 = "-1";
if (isset($_GET['id'])) {
  $colname_Recordset1 = $_GET['id'];
}
mysql_select_db($database_conexion, $conexion);
$query_Recordset1 = sprintf("SELECT * FROM tbusuarios WHERE email = %s", GetSQLValueString($colname_Recordset1, "text"));
$Recordset1 = mysql_query($query_Recordset1, $conexion) or die(mysql_error());
$row_Recordset1 = mysql_fetch_assoc($Recordset1);
$totalRows_Recordset1 = mysql_num_rows($Recordset1);
?>
<!DOCTYPE html> 
<html>
<head>
<meta charset="UTF-8">
<title>Quini-Mex CD JUAREZ</title>
<link href="jquery-mobile/jquery.mobile.theme-1.0.min.css" rel="stylesheet" type="text/css"/>
<link href="jquery-mobile/jquery.mobile.structure-1.0.min.css" rel="stylesheet" type="text/css"/>
<script src="jquery-mobile/jquery-1.6.4.min.js" type="text/javascript"></script>
<script src="jquery-mobile/jquery.mobile-1.0.min.js" type="text/javascript"></script>
<script type="text/javascript" language="JavaScript">
<!--
//--------------------------------
// This code compares two fields in a form and submit it
// if they're the same, or not if they're different.
//--------------------------------
function checkPassword(theForm) {
    if (theForm.password.value != theForm.password2.value)
    {
        alert('Las contraseñas no coinciden!');
        theForm.password.value = "";
        theForm.password2.value = "";
        return false;
    } else {
        return true;
    }
}
//-->
</script>
<style type="text/css">

body {
    background: url(fondo.jpg) !important;
    background-attachment:fixed;
    background-size:cover;
}

.ui-page, .ui-content, .ui-btn {
    background: transparent;
}
</style>
</head> 
<body> 
<?php  mysql_select_db($database_conexion, $conexion);
$ipcliente = $_SERVER['REMOTE_ADDR'];
date_default_timezone_set('America/Los_Angeles'); 
$date = date('Y-m-d H:i:s');
mysql_query("INSERT
            INTO tbactividad
            (hora,ip,motivo)
            VALUES
            ('$date','$ipcliente','Recuperar Password 1')") or die(mysql_error());?>

<div data-role="page" id="page" data-theme="a">
    <div data-role="header">
        <h1>Escriba su nueva contraseña</h1>
    </div>
    <div data-role="content">
     <p>&nbsp;</p>
     <form method="post" name="form1" action="<?php echo $editFormAction; ?>" onsubmit="return checkPassword(this);" data-ajax="false">
       <table align="center">
         <tr valign="baseline">
           <td nowrap align="right">Contraseña:</td>
           <td><input type="password" name="password" value="" size="32"></td>
         </tr>
         <tr valign="baseline">
           <td nowrap align="right">Repita la Contraseña:</td>
           <td><input type="password" name="password2" value="" size="32"></td>
         </tr>
         <tr valign="baseline">
           <td nowrap align="right">&nbsp;</td>
           <td><input type="submit" value="Aceptar contraseña"></td>
         </tr>
       </table>
       <input type="hidden" name="MM_update" value="form1">
       <input type="hidden" name="idUsuario" value="<?php echo $row_Recordset1['idUsuario']; ?>">
     </form>
     <p>&nbsp;</p>
<ul data-role="listview">
      <li><a href="login.php" data-ajax="false">Regresar</a></li>
    </ul>
    </div>
    <div data-role="footer" data-position="fixed">
        <h4><img src="imagenes/mex.png" width="35" height="23"> <img src="imagenes/bra.png" width="35" height="23"> <img src="imagenes/cro.png" width="35" height="23"> <img src="imagenes/cmr.png" width="35" height="23"></h4>
    </div>
</div>

<div data-role="page" id="page2">
    <div data-role="header">
        <h1>Page Two</h1>
    </div>
    <div data-role="content">   
        Content     
    </div>
    <div data-role="footer">
        <h4>Page Footer</h4>
    </div>
</div>

<div data-role="page" id="page3">
    <div data-role="header">
        <h1>Page Three</h1>
    </div>
    <div data-role="content">   
        Content     
    </div>
    <div data-role="footer">
        <h4>Page Footer</h4>
    </div>
</div>

<div data-role="page" id="page4">
    <div data-role="header">
        <h1>Page Four</h1>
    </div>
    <div data-role="content">   
        Content     
    </div>
    <div data-role="footer">
        <h4>Page Footer</h4>
    </div>
</div>

</body>
</html>
<?php
mysql_free_result($Recordset1);
?>

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.

Community · Accepted Answer · 2023-11-17 19:51:26Z

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:

<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.

Stack Exchange Network

Checking PHP security on my file

1 Answer 1

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.

Don't use tables for layout

You must log in to answer this question.

Hot Network Questions

Checking PHP security on my file

1 Answer 1

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.

Don't use tables for layout

You must log in to answer this question.

Related

Hot Network Questions

Don't use `mysql_*`