Page 1 of 1

Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 9:19 am
by gavshouse
hi, im new to php been playing around for around 5months now and ive written a login script which id like some experts to look at if its not too much trouble

basic idea
create a one of passphrase that changes upon each login which can be written to a cookie to then re-login once the session has expired.

ive spent time commenting it so it should be easier to understand, its not finished yet but the basic's are there. so can this be made safer is it safe ?.

Thanks

Code: Select all

if($_REQUEST['code'] == md5(date('d'))){//checks if days match up
    session_start();//starts session
    
    if(isset($_COOKIE['logintry'])){
        $x = $_COOKIE['logintry'];
        $x++;
    }else{
        $x=1;
    }
    
    //check username
    $username = trim($_POST['username']);//removes whitespace
    $username = strtolower($username);//converts to lowercase   
    if(strlen($username) >= 33)://mysql can only handle 32 so they have entered somthing wrong here
        header( 'Location: http://'.$_SERVER['HTTP_HOST'].'/?msg=103' );
        setcookie("logintry", $x, time()+2505600);//writes number of login trys to cookie
        exit;
    endif;
    
    //check password
    $password = trim($_POST['password']);//removes whitespace
    $password = stripslashes($password);//removes slashes
    $password = md5($password);//encrypt in md5
    
    mysql_connect("", "", "") or die(mysql_error());
    mysql_select_db("") or die(mysql_error());
    
    //mysql query
    $query = sprintf("SELECT id,username,password FROM user_table WHERE username='%s' AND password='%s' LIMIT 1",
        mysql_real_escape_string($username),
        mysql_real_escape_string($password));
    $result = mysql_query($query);//excuts query
    
    if(!$result){//if query fails
        header( 'Location: http://'.$_SERVER['HTTP_HOST'].'/?msg=102' );
        setcookie("logintry", $x, time()+2505600);//writes number of login trys to cookie
        exit;
    }else{//if query is ok
        if(mysql_num_rows($result) != 0){//if it doesnt return 0
            $row = mysql_fetch_assoc($result);//makes information useable
            $uid = $row['id'];//saves for later
            $username = ucfirst($row['username']);//saves for later
            $password = $row['password'];//saves for later
            $time = date("F j, Y, g:i a");//current time
            $ip = $_SERVER['REMOTE_ADDR'];
            $passphrase = crypt($username.$password.$ip);//makes passphrase
            
            mysql_query("UPDATE user_table SET passphrase='$passphrase' WHERE id='$uid'");//updates the passphrase with current one
            mysql_query("UPDATE user_table SET last_login='$time' WHERE id='$uid'");//updates last login with current one
            
            $_SESSION['passphrase'] = $passphrase;//writs passphrase to session
            $_SESSION['username'] = $username;//writes username to session
            $_SESSION['uid'] = $uid;//writes user id to session
            setcookie("passphrase", $passphrase, time()+2505600);//writes passphrase to cookie
            
            if(isset($_COOKIE['logintry']))://if there is a logintry cookie lets remove it
                setcookie("logintry", NULL, time()-2605600);//removes login cookie
            endif;          
            
            header( 'Location: '.$_REQUEST['refer'].'?msg=login' );//we are done send back to where you came from
            exit;//stops script
        }else{//if returns 0
            header( 'Location: http://'.$_SERVER['HTTP_HOST'].'/?msg=102' );//sends to homepage with error msg
            setcookie("logintry", $x, time()+2505600);//writes number of login trys to cookie
            exit;//stops script
        }
    }
}else{//if dates dont match then send them to homepage with error msg
    header( 'Location: http://'.$_SERVER['HTTP_HOST'].'/?msg=101' );
    setcookie("logintry", $x, time()+2505600);//writes number of login trys to cookie
    exit;
}

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 9:32 am
by kaisellgren
Is that secure? Can I increase the security? Those are what you are asking for.

There are a couple of things I like to say.

- Do not use $_REQUEST. Use either $_GET or $_POST depending on which one you are expecting to receive.
- Stripping slashes from passwords makes no sense. Why can't I use this password: "mypass\word" ?
- Line 23 says "encrypt in md5". MD5 is a hashing function, nothing to do with encryption.
- Line 60 could be potentially vulnerable to XSS attacks since the data of $_REQUEST['refer'] can come from GPC.
- What is the purpose of writing login tries into cookies?

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 9:39 am
by gavshouse
kaisellgren wrote:Is that secure? Can I increase the security? Those are what you are asking for.

There are a couple of things I like to say.

- Do not use $_REQUEST. Use either $_GET or $_POST depending on which one you are expecting to receive.
- Stripping slashes from passwords makes no sense. Why can't I use this password: "mypass\word" ?
- Line 23 says "encrypt in md5". MD5 is a hashing function, nothing to do with encryption.
- Line 60 could be potentially vulnerable to XSS attacks since the data of $_REQUEST['refer'] can come from GPC.
- What is the purpose of writing login tries into cookies?
hmm ok the request i can fix, i was told to stripping slashes on my passwords by another forum but if theres no point then i will remove it. lol yes the comments are a little wrong there. i was writing the login to cookies to then check if you have tried to login and after 5 stop it but also they could just delete the cookie.

as for the XSS attack not sure what that is but if i change the $_REQUEST['refer'] to $_POST['refer'] will that fix the problem ?

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 9:53 am
by kaisellgren
gavshouse wrote:as for the XSS attack not sure what that is but if i change the $_REQUEST['refer'] to $_POST['refer'] will that fix the problem ?
I am not sure how your code would work if it has $_POST['ref']? It is not submitting the ref, is it? You should do $_GET['ref'] and filter the contents of it.

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 9:56 am
by gavshouse
kaisellgren wrote:
gavshouse wrote:as for the XSS attack not sure what that is but if i change the $_REQUEST['refer'] to $_POST['refer'] will that fix the problem ?
I am not sure how your code would work if it has $_POST['ref']? It is not submitting the ref, is it? You should do $_GET['ref'] and filter the contents of it.
ive changed the html form to

Code: Select all

    $form = '<form action="login.php" method="post">';
    $form .= '<input type="hidden" name="refer" value="'.$refer.'"/>';
    $form .= '<input type="hidden" name="code" value="'.md5(date("d")).'"/>';
    $form .= '<input type="text" name="username"/>';
    $form .= '<input type="password" name="password"/>';
    $form .= '<input type="submit" name="submit" value="Login"/>';
    $form .= '</form>'; 
and then

Code: Select all

header( 'Location: '.$_POST['refer'].'?msg=login' );//we are done send back to where you came from

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 10:00 am
by kaisellgren
Although it is not very straightforward to expoit that, it is still not secure. You need to filter the variables you pass into header(). Always.

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 10:05 am
by gavshouse
kaisellgren wrote:Although it is not very straightforward to expoit that, it is still not secure. You need to filter the variables you pass into header(). Always.
filter the variables i pass into the header, hmm not sure what you mean.

to solve the refer problem i could simple do this

Code: Select all

 
if($_POST['refer'] == $_SERVER['HTTP_REFERER']){
//ok
}else{
//
}

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 10:16 am
by kaisellgren
gavshouse wrote:filter the variables i pass into the header, hmm not sure what you mean.

to solve the refer problem i could simple do this

Code: Select all

 
if($_POST['refer'] == $_SERVER['HTTP_REFERER']){
//ok
}else{
//
}
And what if the web browser did not send HTTP_REFERER? :P

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 10:29 am
by gavshouse
kaisellgren wrote: And what if the web browser did not send HTTP_REFERER? :P
true, so what should i do ?

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 10:58 am
by kaisellgren
gavshouse wrote:true, so what should i do ?
kaisellgren wrote:You should do $_GET['ref'] and filter the contents of it.
If you are unexperienced, maybe start off by searching google? Let me help you: http://www.google.com/search?hl=en&q=ph ... filter+url

Make sure the location you pass into the header() is a URL and nothing else (e.g. HTTP response splitting).

Re: Is my code safe (mysql injection)

Posted: Sat Feb 28, 2009 11:06 am
by gavshouse
sorry i didnt even know the function existed

im going to use

Code: Select all

<?php
$url = "http://www.example.com";
 
if(!filter_var($url, FILTER_VALIDATE_URL))
 {
 echo "URL is not valid";
 }
else
 {
 echo "URL is valid";
 }
?>
Thanks alot