Posted by Andy Hassall on 12/21/05 16:56

On Wed, 21 Dec 2005 02:37:51 GMT, Pete Horm <petehorm@hotmail.com> wrote:

>If anyone could take a look at the following
>html and php script, and let me know if this is the right way of doing it
>or if there is a better way, I would greatly appreciate it.

<snip the HTML which looks pretty much OK>

Basically the PHP is using $_POST correctly, but it's missing error handling
and has a major security hole:

>$connection = mysql_connect("localhost","user","password");

Whenever you make mysql_* calls you should check the return value; you've done
this in the mysql_select_db below but not here.

>$db = "mailman";
>
>mysql_select_db($db,$connection) or die("Could not open $db");

mysql_error() can give more informative error messages, although it's up to
you whether you want to send the raw MySQL error message to the user or not.

>$username = ($_POST['username']);
>$password = ($_POST['password']);

The brackets aren't necessary, but don't do any harm.

>$sql = "Select * from users where username = '$username' and password =
>'$password'";

Serious trouble here - do a search for "sql injection attacks".

If $password contains quotes, then this will cause an error in the SQL. From
there, you can start putting in specific values that change the condition in
the SQL, for example you could send:

'' or 'x'='x

... as password, which results in:

Select * from users where username = 'username' and password = '' or 'x'='x'

This will return all the data in the table, so the page can be tricked in this
way to thinking it's got a valid login, when actually it hasn't.

Use mysql_escape_string() on all values before they get put into SQL.

Another approach is to use a database abstraction library, my favourite being
ADOdb (http://adodb.sourceforge.net), which can take away the worry of having
to remember to escape values. You can then write statements like:

$result = $db->Execute(
'select * from users where username = ? and password = ?',
array($username, $password)
);

The library then handles whatever is required to get the values into the
database, substituting the "?" placeholders with values that are escaped and
quoted if necessary (or other databases, such as Oracle, bind values separately
to running the statement), which makes avoiding SQL injection attacks much
easier.

>$result = mysql_query($sql,$connection) or die("Could not execute sql:
>$sql");
>
>$num_rows = mysql_num_rows($result);

You ought to fetch the row and check it matches at least the username you
supplied, and if $num_rows > 1 that'd be suspicious.

>if ($num_rows > 0 ) {
> header("Location: mailman_main.php");

Location headers have to go to absolute URLs, e.g.
http://example.com/mailman_main.php

Relative URLs aren't allowed in the HTTP specifications, although most
browsers correct for this common mistake.

>}else {
> header("Location: failedlogon.html");
>}
>?>

--
Andy Hassall :: andy@andyh.co.uk :: http://www.andyh.co.uk
http://www.andyhsoftware.co.uk/space :: disk and FTP usage analysis tool

[Back to original message]