Page 1 of 1

injection problem

Posted: Tue Jan 25, 2005 10:57 pm
by shiznatix
is this secure? iv tried simple injections on it and they havnt worked. what u say?

Code: Select all

$sql = mysql_query(\"SELECT * FROM users WHERE username='$username' AND password='$password'\"); 
$login_check = mysql_num_rows($sql); 

if($login_check > 0){ 
        if (!isset($_SESSION['username'])) {
		session_register('username'); 
        $_SESSION['username'] = $username; 
		}
		if (!isset ($_SESSION['password'])){
        session_register('password'); 
        $_SESSION['password'] = $password;
		}
		if (!isset ($_SESSION['usersid'])){
		session_register('usersid');
		while ($rdz = mysql_fetch_assoc($sql)) {
		$_SESSION['usersid'] = $rdz['usersid'];
			}
		}
}else{
header('location: login.php?id=85');
}

Posted: Tue Jan 25, 2005 11:15 pm
by feyd
it's as secure as how $username and $password are set. How are they set in this example? What version of php would this run against?

Posted: Fri Jan 28, 2005 7:40 pm
by shiznatix
set with this

Code: Select all

<form action="admin.php" method="post">
Name:<input type="text" name="username" size="20"><br>
Password:<input type="password" name="password" size="20"><br>
<input type="submit" name="submit" value="submit">
</form>
i use php 4.3.9

Posted: Sat Jan 29, 2005 2:51 pm
by thegreatone2176
you should strip ' from the username and password because if you dont it will mess up your query when trying to select.

to be extra safe strip < > also

Posted: Sat Jan 29, 2005 3:20 pm
by Joe
Oh and strip asterisks. I noticed in searches etc that you can create a wildcard type of injection.

Posted: Sat Jan 29, 2005 3:52 pm
by timvw
strip nothing, but use mysql_real_escape_string...

Posted: Tue Feb 01, 2005 12:08 am
by fresh
as of now it is pretty wide open:

' or ''=' .. might mess with it a bit.. not sure as I don't study SQL injections. However, seen that same routine several times before and it has always been easy to break.

regards

Posted: Wed Feb 02, 2005 3:38 am
by jlawrence
...magic_quotes_gpc.....set them or do 'foreach' loops and manually set them..that will solve a lot of injection problems.

if you have the SQL statement set to "username='$username' .." etc, you don't need to strip out the characters that would allow for wild-cards. However "username like '$username' ..." then you would have problems with that. :)

Posted: Wed Feb 02, 2005 4:02 am
by timvw
something like

Code: Select all

function sql()
{
    $args = func_get_args();
    $format = array_shift($args);

    if (!get_magic_quotes_gpc())
    {
        for ($i = 0; $i < count($args); ++$i)
        {
            $args[$i] = mysql_escape_string($args[$i]);
        }
    }

    return vsprintf($format, $args);
}

$sql = sql(\"SELECT * FROM foo WHERE bar = %d\", $id);

Re: injection problem

Posted: Sun Feb 06, 2005 11:37 am
by shiflett
> is this secure? iv tried simple injections on it and they havnt worked.

The only way this could be more insecure is if the entire SQL query was provided by the user (rather than just part of it).

Out of curiosity, what simple injections have you tried? Every one I can think of should work just fine and alert you to the vulnerability.

> $login_check = mysql_num_rows($sql);

As a side note, it's best to name result sets something like $result instead of $sql. The misleading variable name can cause confusion, as this code demonstrates. You can't use mysql_num_rows() on an SQL statement.

> if (!isset($_SESSION['username'])) {
> session_register('username');
> $_SESSION['username'] = $username;
> }

There's no need for session_register(). Also, it's nice to be able to consider session data safe. When you assign tainted data to a session variable, you prevent this.

> header('location: login.php?id=85');

The Location header requires an absolute URL, not a relative one.

To help prevent SQL injection, you need to always filter your data on input and properly escape your data on output. When sending data to a MySQL database, this means using mysql_real_escape_string().

For more information, see:

http://shiflett.org/articles/security-corner-apr2004
http://phpsec.org/projects/guide/3.html#3.2
http://www.unixwiz.net/techtips/sql-injection.html

Posted: Thu Feb 10, 2005 8:29 am
by PHPadvisor
Sometimes it's easier to check what SHOULD be allowed in a string rather than what shouldn't. If your usernames and passwords can/should only be letters, numbers and say dashes... then remove everything that isn't one of these.