injection problem

Discussions of secure PHP coding. Security in software is important, so don't be afraid to ask. And when answering: be anal. Nitpick. No security vulnerability is too small.

Moderator: General Moderators

Post Reply
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

injection problem

Post 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');
}
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post 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?
User avatar
shiznatix
DevNet Master
Posts: 2745
Joined: Tue Dec 28, 2004 5:57 pm
Location: Tallinn, Estonia
Contact:

Post 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
thegreatone2176
Forum Contributor
Posts: 102
Joined: Sun Jul 11, 2004 1:27 pm

Post 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
User avatar
Joe
Forum Regular
Posts: 939
Joined: Sun Feb 29, 2004 1:26 pm
Location: UK - Glasgow

Post by Joe »

Oh and strip asterisks. I noticed in searches etc that you can create a wildcard type of injection.
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

strip nothing, but use mysql_real_escape_string...
User avatar
fresh
Forum Contributor
Posts: 259
Joined: Mon Jun 14, 2004 10:39 am
Location: Amerika

Post 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
jlawrence
Forum Newbie
Posts: 2
Joined: Wed Feb 02, 2005 3:36 am

Post 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. :)
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post 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);
User avatar
shiflett
Forum Contributor
Posts: 124
Joined: Sun Feb 06, 2005 11:22 am

Re: injection problem

Post 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
User avatar
PHPadvisor
Forum Newbie
Posts: 11
Joined: Mon Dec 13, 2004 3:20 pm

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