I built a PHP login script - is this secure?

unclesasamon

Hello everyone - I am making a web portal for my mom's company and I am trying to make a login script. Can someone check the script to make sure it is secure?

<!doctype html public "-//W3C//DTD HTML 4.0 //EN">
<html>
<head>
       <title>Login to Cordium Links' Web Portal </title>
</head>
<body>
<?php

/* Key 1 ("Username") has the username; Key 2 ("Password") has the password */
$web_user = $_POST["Username"]; $web_pass = $_POST["Password"];

$host = "localhost";
$user = "user";
$password = "pass";
$dbname = "cordium_links_portal";

$cxn = mysqli_connect($host,$user,$password,$dbname);

$sql = "SELECT * FROM login WHERE username = '$web_user' AND password = '$web_pass' LIMIT 1";
$result = mysqli_query($cxn,$sql); 

while($row = mysqli_fetch_row($result))
{
	$data = extract($row);
}

if(isset($data))
{
	echo "Correct";
}
else
{
	die("Incorrect Password");
}
// Assuming correct login information, the user will be logged in at this point
?>
</body>
</html>

bradgrafelman

Just thought I'd comment both on security as well as semantic improvements:

I altered your post to include [noparse]
[/noparse] BBcode tags (as opposed to the CODE tags you had used) - this makes your code easier to read and analyze; please use these in the future when posting PHP code.
This line:
```
$web_user = $_POST["Username"]; $web_pass = $_POST["Password"];
```
will cause E_NOTICE level errors if the data wasn't POST'ed. You should first use [man]isset/man to check if the array indexes exist before you try to use them.
Your SQL query appears to be vulnerable to SQL injection attacks. User-supplied data should never be placed directly into a SQL query. Instead it should be sanitized with a function such as [man]mysqli_real_escape_string/man.
You could change the 'SELECT *' to only SELECT the columns that you actually need data from.
If invalid login credentials are sent, the die() will cut off the ending HTML code.
You seem to be doing a plain-text password comparison. Are passwords not encrypted/hashed in your database??

Weedpacket

Since you're limiting the number of results returned to one, there's no point writing a whole loop for the task: a simple if() statement would suffice.

$result = mysqli_query($cxn, $sql);
$data = mysqli_fetch_row($result);
if($data)
{
	...
}
else
{
	...
}

unclesasamon

How about this?

<!doctype html public "-//W3C//DTD HTML 4.0 //EN">
<html>
<head>
       <title>Login to Cordium Links' Web Portal </title>
</head>
<body>
<?php

/* Key 1 ("Username") has the username; Key 2 ("Password") has the password */



if (isset($_POST["Username"]) && isset($_POST["Password"]))
{

$web_user = $_POST["Username"]; $web_pass = $_POST["Password"];

$web_pass = md5($web_pass);

$host = "localhost";
$user = "phpuser";
$password = "zxc";
$dbname = "cordium_links_portal";

$cxn = mysqli_connect($host,$user,$password,$dbname);

$web_user = mysql_real_escape_string($web_user);
$web_pass = mysql_real_escape_string($web_pass);
$sql = "SELECT username FROM login WHERE username = '$web_user' AND password = '$web_pass' LIMIT 1";

$result = mysqli_query($cxn,$sql); 

$row = mysqli_fetch_row($result);

if(isset($row))
{
	echo "Correct";
}
else
{
	echo("Incorrect Password");
}
}
// Assuming correct login information, the user will be logged in at this point
?>
</body>
</html>

Weedpacket

$row is always going to be set; to false, if nothing else.

unclesasamon

strange - the script works though.

If the $result from the previous line is false, then wouldn't the $row not be set then?

NogDog

$row would be set, with a value of boolean false. You want to do one of the following (though they're not necessarily the only options):

if(isset($row) and $row !== false) {

// ...or... //

if(!empty($row)) {

laserlight

If the $result from the previous line is false, then wouldn't the $row not be set then?

For $result to be false, it means that the database connection failed, or the correct database was not selected, or the SQL statement was incorrect. If any of these are true, then then mysqli_fetch_row($result) is a mistake to begin with.

As such, instead of checking if $row is set, you should check the number of rows returned (and check that $result is not false to begin with), or change the query to select COUNT(*) instead.

By the way, since you are using the MySQLi extension, you should escape with mysqli_real_escape_string(), not mysql_real_escape_string(). Better yet, you could use prepared statements.

unclesasamon

ok, I think I got it - thanks for the help!