2

When we use dreamweaver to create an auto-generated Login Logout - How secure it is?

while doing this dreamweaver creates 3 folders:-

_mmServerScripts
_notes
Connections

and adds this on the login.php page:-

<?php require_once('../Connections/da.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;
}
}
?>
<?php
// *** Validate request to login to this site.
if (!isset($_SESSION)) {
  session_start();
}

$loginFormAction = $_SERVER['PHP_SELF'];
if (isset($_GET['accesscheck'])) {
  $_SESSION['PrevUrl'] = $_GET['accesscheck'];
}

if (isset($_POST['username'])) {
  $loginUsername=$_POST['username'];
  $password=$_POST['password'];
  $MM_fldUserAuthorization = "";
  $MM_redirectLoginSuccess = "../index.php";
  $MM_redirectLoginFailed = "index.php";
  $MM_redirecttoReferrer = false;
  mysql_select_db($database_da, $da);

  $LoginRS__query=sprintf("SELECT user_id, user_password FROM users WHERE user_id=%s AND user_password=%s",
    GetSQLValueString($loginUsername, "int"), GetSQLValueString($password, "text")); 

  $LoginRS = mysql_query($LoginRS__query, $da) or die(mysql_error());
  $loginFoundUser = mysql_num_rows($LoginRS);
  if ($loginFoundUser) {
     $loginStrGroup = "";

    if (PHP_VERSION >= 5.1) {session_regenerate_id(true);} else {session_regenerate_id();}
    //declare two session variables and assign them
    $_SESSION['MM_Username'] = $loginUsername;
    $_SESSION['MM_UserGroup'] = $loginStrGroup;       

    if (isset($_SESSION['PrevUrl']) && false) {
      $MM_redirectLoginSuccess = $_SESSION['PrevUrl'];  
    }
    header("Location: " . $MM_redirectLoginSuccess );
  }
  else {
    header("Location: ". $MM_redirectLoginFailed );
  }
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>
</head>

<body>
<form name="login" action="<?php echo $loginFormAction; ?>" method="POST" target="_self">
<input name="username" type="text" />
<input name="password" type="password" />
<input name="login_button" type="submit" />
</form>
</body>
</html>

Also it uses the mysql_pconnect for mysql connection:-

<?php
# FileName="Connection_php_mysql.htm"
# Type="MYSQL"
# HTTP="true"
$hostname_da = "localhost";
$database_da = "database";
$username_da = "root";
$password_da = "password";
$dacreint = mysql_connect($hostname_da, $username_da, $password_da) or trigger_error(mysql_error(),E_USER_ERROR); 
?>

Is it 100% efficient and secure? And if not can we do some modifications to it to make it as secure as possible... Because this really makes things fast.

2

2 Answers 2

3

Edit: The accepted answer to the dupe of this question (by the same author? Why?) points out two vulnerabilities that I missed: PHP_SELF is vulnerable to XSS, and falling back to mysql_escape_string() shouldn't be necessary (although that is not a real world concern either way.) rather than steal the points from MrCode, I encourage everyone to closevote this, and upvote MrCode's better answer :)

It's not very beautiful code, but security- and efficiency-wise, it looks pretty okay. It takes into consideration that magic quotes are phased out in PHP 6, it seems to escape all incoming form data properly, and tests for the presence of variables before accessing them.

The only thing that looks like it could cause problems is the superfluous line break here:

<?php require_once('../Connections/da.php'); ?>   <---- here
<?php

that will mess with the header redirect in certain situations. I would get rid of it and just do

<?php require_once('../Connections/da.php'); 
      if (!function_exists("GetSQLValueString")) { 

also, instead of

  .... or die(mysql_error());

one could use

  ....  or trigger_error(mysql_error(), E_USER_ERROR);

to prevent SQL error messages from being shown in production environments.

8
  • Do really making two different php code areas make any difference?? I am unable to understand, could you please explain a bit more... Mar 22, 2012 at 17:16
  • @Abhilash you can't output any data before sending a header() command. Those two lines send data (the line break) and that can lead to the Cannot modify header information - headers already sent by ... error message
    – Pekka
    Mar 22, 2012 at 17:19
  • just removing that piece of code i.e. ?> and <?php can solve the problem? If it is so thanks for this, because now i have something to research on :) Mar 22, 2012 at 17:24
  • 1
    @Abi yes, that would fix the problem.
    – Pekka
    Mar 22, 2012 at 17:24
  • 1
    @Abi combine MrCode's and my advice and I think you will have a pretty much waterproof solution.
    – Pekka
    Mar 22, 2012 at 17:35
1

This is maybe not such a great idea:

mysql_query($LoginRS__query, $da) or die(mysql_error());

This reveals the mysql error explanation to the client, possibly revealing details of the DB schema of your user table.

3
  • actually it is backed by error handling code... it reads a php file having code like this define('MYSQL_NOT_EXISTS', create_error("Your PHP server doesn't have the MySQL module loaded or you can't use the mysql_(p)connect functions.")); Mar 22, 2012 at 17:19
  • i removed the persistent connection Mar 22, 2012 at 17:19
  • 1
    php.net/manual/en/function.mysql-error.php is a php function that returns the last mysql error.
    – Tim
    Mar 22, 2012 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.