Yep, Another login script

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

singleString
Forum Newbie
Posts: 6
Joined: Fri Feb 13, 2009 5:23 pm

Yep, Another login script

Post by singleString »

Now my knowledge of php security is not very vast, and I'm just beginning with php, so I was just thought if you could take a quick look over my login script for a website I'm working on even though you are probably tired to death of them.

The script is just to log a user in so they can access a control panel for the site.

It's pretty self explanatory I think, but if not you can ask about anything not that I didn't think you would.

Little comments are better than no comments.

Thanks in advance. (I hope)

Code: Select all

<?php
//set counter for number of times a person has logged in unsuccesfully
//loginLimit();
 
//sets mysql variables
include("/srv/www/mysql.php");
loginSql();
 
$result = $GLOBALS["result"];
$connection = $GLOBALS["connection"];
 
//checks if user is remembered
if(isset($_COOKIE["username"]) && isset($_COOKIE["password"]) && isset($_COOKIE["group"]))
{
    $user = $_COOKIE["username"];
    $pass = $_COOKIE["password"];
    $group = $_COOKIE["group"];
    $checkSql = "SELECT * FROM `login` WHERE user='$user' AND `password`='$pass' AND `group`='$group'";
    print $checkSql;
    print "<br />";
    $checkResult = mysql_query($checkSql, $connection);
    
    if($checkResult == TRUE)
    {
        session_name("CP");
        session_start();
        $_SESSION['user'] = $_COOKIE["username"];
        $_SESSION['pass'] = $_COOKIE["password"];
        $_SESSION['group'] = $_COOKIE["group"];
        session_write_close();
                            
        writeLog();
        header('Location: CPanel.php');
        exit();
    }
    else
    {
        header('Location: index.php');
        exit();
    }
}
 
//checks if username and or password was input  
if ($_REQUEST["username"] == "")
{
    if($_REQUEST["password"] == "")
    {
        writeLog();
        loginLimit();
        header('Location: index.php?nUoP=1');
        //print('No User Name or Password');
    }
    else
    {
        writeLog();
        loginLimit();
        header('Location: index.php?nU=1');
        //print('No User');
    }
}
else if ($_REQUEST["password"] == "")
{
    writeLog();
    loginLimit();
    header('Location: index.php?nP=1');
    //print('No Password');
}
else
{
    checkLogin();
}
 
//checks if username and password inputed were correct
function checkLogin()
{
    global $result;
    $user = $_REQUEST["username"];
    $totalRows = mysql_num_rows($result);
    $count = 0;
    
    while($login = mysql_fetch_row($result))
    {
        if($user == $login[0])
        {
            if (md5($_REQUEST["password"]) == "$login[1]")
                {
                    $pass = md5($_REQUEST["password"]);
                    
                    session_name("CP");
                    session_start();
                    $_SESSION['user'] = $user;
                    $_SESSION['pass'] = $login[1];
                    $_SESSION['group'] = $login[2];
                    session_write_close();
                    
                    if($_POST["remember"] == "YES")
                    {
                        setcookie("username", $user, time()+60*60*24*100);  
                        setcookie("password", $login[1], time()+60*60*24*100);
                        setcookie("group", $login[2], time()+60*60*24*100); 
                    }       
                                        
                    writeLog();
                    header('Location: CPanel.php');
                }
                else
                {
                    writeLog();
                    loginLimit();
                    header('Location: index.php?iP=1');
                }
        }
        else
        {
            $count++;
        }
        
        if($count == $totalRows)
        {
            writeLog();
            loginLimit();
            header('Location: index.php?iU=1');
        }
        else
        {
        }
    }
}
 
//writes login attempt to log
function writeLog()
{
    $connection = $GLOBALS["connection"];
    
    $user = $_REQUEST["username"];
    $date = date("l dS \of F Y h:i:s A");
    $ip = $_SERVER['REMOTE_ADDR'];
 
    $sql = "INSERT INTO `login log` VALUES ('$user', '$date', '$ip')";
    mysql_query($sql, $connection);
}
 
//set tries cookie
function loginLimit()
{
    if(isset($_REQUEST["tries"]))
    {
        $tries = $_REQUEST["tries"];
        $tries++;
    }
    else
    {
        $tries = 1;
    }
    setcookie("tries", $tries, time() + 500);
    
}
?>
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

Code: Select all

include("/srv/www/mysql.php");
Since you are loading it directly, how about putting the file in /srv/ ?

Line 18 is vulnerable to SQL injections.

I think line 19 is for debugging purposes, but if you leave it there, you are vulnerable to XSS.

You should replace the line 23 with mysql_num_rows().

writeLog() is vulnerable to SQL injections.

Does CPanel.php make sure that the user is authenticated?

What is the point of the login limit if you do not call it prior to line 39?

The login tries can be altered easily.

You should not use $_REQUEST, use either $_POST or $_GET.

Do not use MD5. I recommend SHA-256, SHA-512 or Whirlpool.
singleString
Forum Newbie
Posts: 6
Joined: Fri Feb 13, 2009 5:23 pm

Re: Yep, Another login script

Post by singleString »

Regarding the SQL injections, how could I change it to make them make them uninjectable, and what is SQL injection?

The Cpanel.php page does check the user authentication.

Your right about the login limit, I'll take some time into seeing another way to do that another time.

Thank you for pointing out the use of $_REQUEST in here, I thought I had changed it early but it seems I hadn't.

Just out of curiosity, what is the difference between MD5 and the other you mentioned.
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

singleString wrote:Regarding the SQL injections, how could I change it to make them make them uninjectable, and what is SQL injection?
I just don't understand why people are imcapable of googling... http://en.wikipedia.org/wiki/SQL_injection
singleString wrote:Just out of curiosity, what is the difference between MD5 and the other you mentioned.
Plenty of differences. See my blog entry about hashing. The main reason why not to use MD5 is safety.
singleString
Forum Newbie
Posts: 6
Joined: Fri Feb 13, 2009 5:23 pm

Re: Yep, Another login script

Post by singleString »

So, based on the article you kindly google for me, this should prevent the Sql Injection.

Correct?

Code: Select all

$illegalCharacters = array("\"", "\\", "/", "*", "'", "=", "-", "#", ";", "<", ">", "+", "%"), "$";
 
$user  = str_replace($illegalCharacters, "", $user);
$password = str_replace($illegalCharacters, "", $password);
$group = str_replace($illegalCharacters, "", $group);
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

Are you using MySQL? If so, just use the built-in function mysql(i)_real_escape_string().
singleString
Forum Newbie
Posts: 6
Joined: Fri Feb 13, 2009 5:23 pm

Re: Yep, Another login script

Post by singleString »

Yes I am, and was unaware of this function, thank you for pointing it out.
temidayo
Forum Contributor
Posts: 109
Joined: Fri May 23, 2008 6:17 am
Location: Nigeria

Re: Yep, Another login script

Post by temidayo »

kaisellgren wrote: You should not use $_REQUEST, use either $_POST or $_GET.
I want to believe that statement is about optimization rather than security.
Or is there a security risk attached to using $_REQUEST?
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

temidayo wrote:I want to believe that statement is about optimization rather than security.
Or is there a security risk attached to using $_REQUEST?
It implies risks and in some cases they become serious.

For instance, let's say blogspot.com uses the following code:

Code: Select all

if ($_REQUEST['username'] != ... || $_REQUEST['password'] != ...)
{
 increase_failed_logins();
}
Everytime the user logins with wrong credentials, it increases a counter, which will forbid access to the site once it has reached 3 failed attempts.

Now, if I make my own blog at attacker.blogspot.com to set a cookie for blogspot.com with username=asd, then the user will automatically get banned and will never access the main site.

$_REQUEST involves $_GET, $_POST and $_COOKIES.

Whenever you accept unnecessary alternatives, you increase the number of risks in your application.

No, it has nothing to do with optimization.
vivekanandanpcn8
Forum Newbie
Posts: 1
Joined: Wed Apr 15, 2009 9:13 pm

Re: Yep, Another login script

Post by vivekanandanpcn8 »

For Login First you have to make use of SSl protocol and then for faster and more security have the session in th database which more reliable and used in te bussinedd critical application.more info http://www.gnudeveloper.com
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

vivekanandanpcn8 wrote:For Login First you have to make use of SSl protocol and then for faster and more security have the session in th database which more reliable and used in te bussinedd critical application.more info http://www.gnudeveloper.com
To me it seems you do not know what you are talking about. Are you here just to advertise your new board?
temidayo
Forum Contributor
Posts: 109
Joined: Fri May 23, 2008 6:17 am
Location: Nigeria

Re: Yep, Another login script

Post by temidayo »

kaisellgren wrote:Whenever you accept unnecessary alternatives, you increase the number of risks in your application.
No, it has nothing to do with optimization.
That point is taken.
But that does not mean $_REQUEST should be avoided like a curse.
For instance, you may want to create APIs or a kind of interface on your website
and you do not want to worry either the data is submitted through POST or GET or COOKIES.

I still think one neat way of doing it is to use $_REQUEST instead of testing if it is GET or POST or otherwise
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

temidayo wrote:But that does not mean $_REQUEST should be avoided like a curse.
The way I look at $_REQUEST is that it is like a disease. For sure you may be able to handle it, but why to risk?
temidayo wrote:For instance, you may want to create APIs or a kind of interface on your website and you do not want to worry either the data is submitted through POST or GET or COOKIES.
You must worry where the data comes from. Another thing is that your application must know to where it outputs the data. Data I/O is an important aspect of security.
temidayo wrote:I still think one neat way of doing it is to use $_REQUEST instead of testing if it is GET or POST or otherwise
If your application needs $_REQUEST to work, it is not designed well. POST is for data manipulation, GET is not. COOKIES are for "persistent" client side data "storage".
temidayo
Forum Contributor
Posts: 109
Joined: Fri May 23, 2008 6:17 am
Location: Nigeria

Re: Yep, Another login script

Post by temidayo »

kaisellgren wrote: The way I look at $_REQUEST is that it is like a disease. For sure you may be able to handle it, but why to risk?
I don't know whether to agree or not :)
kaisellgren wrote: If your application needs $_REQUEST to work, it is not designed well. POST is for data manipulation, GET is not. COOKIES are for "persistent" client side data "storage".
You strike out $_REQUEST. Now you have something against $_GET :) . What is GET for, if not for 'data manipulation'?
When a website allows you to send parameters through url, how do you collect it? Is it not through $_GET or $_REQUEST?
User avatar
kaisellgren
DevNet Resident
Posts: 1675
Joined: Sat Jan 07, 2006 5:52 am
Location: Lahti, Finland.

Re: Yep, Another login script

Post by kaisellgren »

temidayo wrote:You strike out $_REQUEST. Now you have something against $_GET :) . What is GET for, if not for 'data manipulation'?
When a website allows you to send parameters through url, how do you collect it? Is it not through $_GET or $_REQUEST?
I have nothing against GET.

http://www.w3.org/MarkUp/html-spec/html ... tml#SEC8.2

GET:
If the processing of a form is idempotent (i.e. it has no lasting observable effect on the state of the world), then the form method should be `GET'. Many database searches have no visible side-effects and make ideal applications of query forms.
POST:
If the service associated with the processing of a form has side effects (for example, modification of a database or subscription to a service), the method should be `POST'.
Post Reply