Security

[Ryan Fait]

I know posting chunks of code is rather wearisome, but if there are some super-PHP people here that can look and tell you if it's good or bad, I'd really appreciate some feedback. This is a a two-parter login and session checking script. The first part is the actual login script. Someone submits a username and password. The second is the session or cookie reader. If there is a cookie, but no session, it contacts the database and the sets the information from there if it's valid. Anyway, the code is more specific.

Oh, and this is for a CMS I'm developing, so it's gotta be good

PHP: Select all

if(isset($_POST["user"]))

{

    if($_POST["user"] != "")

    {

        $query = mysql_query("SELECT username, password, admin, last_login, locked FROM accounts WHERE username='".$_POST["user"]."'");

        $r = mysql_fetch_array($query);

        if(md5($_POST["pass"]) == $r["password"])

        {

            if($r["locked"] == 0)

            {

                $_SESSION["logged"] = $_POST["user"]."__".md5($_POST["pass"]);

                $_SESSION["username"] = $r["username"];

                mysql_query("UPDATE accounts SET last_login='".time()."' WHERE username='".$_POST["user"]."'");

                if(isset($_POST["remember"])) setcookie("logged", $_POST["user"]."__".md5($_POST["pass"]), time() + 1814400);

                if($s["maintenance"] == 1) $_SESSION["maintenance"] = 1;

                if(!$conn_id = ftp_connect($ftp_host, $ftp_port)) $_SESSION["ftp_die"] = 1;

                if(!ftp_login($conn_id, $ftp_user, $ftp_pass)) $_SESSION["ftp_die"] = 1;

                if(!ftp_chdir($conn_id, $ftp_root)) $_SESSION["ftp_die"] = 1;

                ftp_close($conn_id);

                header("Location: .?".$home);

                exit();

            } else $invalid = 1;

        } else $invalid = 2;

    } else $invalid = 3;

} 

And to check to make sure the session or cookie is valid.

PHP: Select all

if(!isset($_SESSION["logged"]))

{

    if(isset($_COOKIE["logged"]))

    {

        $cookie_data = explode("__", $_COOKIE["logged"]);

        $query = mysql_query("SELECT username, password, admin, last_login, locked FROM accounts WHERE username='".$cookie_data[0]."'");

        $r = mysql_fetch_array($query);

        if($cookie_data[1] == $r["password"] && $r["locked"] == 0) {



            $_SESSION["logged"] = $cookie_data[0]."__".$cookie_data[1];



            mysql_query("UPDATE accounts SET last_login='".time()."' WHERE username='".$cookie_data[0]."'");



            if(!$conn_id = ftp_connect($ftp_host, $ftp_port))

                $_SESSION["ftp_die"] = 1;

            if(!ftp_login($conn_id, $ftp_user, $ftp_pass))

                $_SESSION["ftp_die"] = 1;

            if(!ftp_chdir($conn_id, $ftp_root))

                $_SESSION["ftp_die"] = 1;

            ftp_close($conn_id);

        } else {

            $quit = 1;

        }

    } else {

        $quit = 1;

    }

}

elseif(!isset($mysql_die))

{

    $session_data = explode("__", $_SESSION["logged"]);

    $query = mysql_query("SELECT password, locked FROM accounts WHERE username='".$session_data[0]."'");

    $s = mysql_fetch_array($query);

    if($session_data[1] == $s["password"] && $s["locked"] == 0)

        $_SESSION["logged"] = $session_data[0]."__".$session_data[1];

    else

        $quit = 1;

} 

I think everything should be self explanatory, but just for reference, a users account is "locked", it's suspended and he/she won't be able to use the site or log in.

[masonbarge]

Code: Select all

if(isset($_POST["user"]))
{
    if($_POST["user"] != "")
    {

Certainly functional but a little awkward. I assume you did this to to allow "0". IMO it would be more transparent in the style

Code: Select all

if ($x || ($x=="0")) {

assuming that does what you want. But really, why bother with a username "0" in the first place?

Code: Select all

 $query = mysql_query("SELECT username, password, admin, last_login,
 locked FROM accounts WHERE username='".$_POST["user"]."'");

I wouldn't have even been sure that this would work, but I'd set $user first so that the code would be cleaner and easier to read, like this:

Code: Select all

 WHERE username = '$user'";

I don't know if it's me or generally accepted practice, but I really like to get complex expressions reduced to variables when they need to go in another expression.

Code: Select all

$r = mysql_fetch_array($query);

I was expecting to see a doublecheck for uniqueness here, although that might be unnecessary. Also, the username is a good place for a regex doublecheck of some sort, at least to eliminate html tags and a few characters like "-".

Code: Select all

if(md5($_POST["pass"]) == $r["password"])
        {
            if($r["locked"] == 0)

You might want an echo of some sort if it's locked.

Code: Select all

  {
  *        *            *
            } else $invalid = 1;
        } else $invalid = 2;
    } else $invalid = 3;
}

Ah I see, you have an error reporting message system I'm not used to. I assume you generate an error statement for the innocent fool, like "please doublecheck the spelling . . . "

The session/cookie section is something I don't know enough about to make even stupid comments. Wish I could be of more help. My general comments would be that:
1. The script would benefit from a layer of validation. Hackers are smarter than I am and I like to give them as much trouble as possible.
2. That the style of concatenating array variables inside a query is easier to deal with if it's cleaned up, e.g.

Code: Select all

mysql_query("SELECT password, locked FROM accounts WHERE 
username='".$session_data[0]."'");

A lot of people seem to do it. Actually, do you need the double quotes? (I don't know, since I don't do this.) And come to think of it, does it even parse? It would just aggravate me to have to deal with

Code: Select all

."'";

, especially if I had to edit it later on.
3. A small tip: when you post code, preview it and then break lines to eliminate the horizontal scroll bar. It makes it a lot easier to read.

[masonbarge]

Here's a nice cheat sheet if you don't have it, from PHP Manual. I tried to copy it but formatting is impossible:

http://us3.php.net/manual/en/types.comparisons.php

[Ryan Fait]

Thanks! In regards to your first quote, I do that so it's completely error free. If you check for $_POST["username"] and it doesn't exist, it'll fire a small error. As for the second one, it's just to ensure that a username was actually submitted, not just an empty string.

$user and $_POST["user"] are two different variables. It works on some configurations, but not mine. I use the $_POST version because it lets me know where the information is coming from when I look at it.

The uniqueness of the username isn't an issue. It's not possible to register a username that already exists, and if someone is fooling around with the database by hand, they'd end up screwing the system up anyway.

Yeah, the $invalid variable triggers an error message later on in the page.

Finally, ."'" is indeed unnecessary. Thanks for pointing that one out, but it does parse, however pointless it is.

Thanks for the cheat sheet! I haven't seen that one on their site before. That's nice. Again, thanks for having a peak

[Gee Bee]

This line

mysql_query("UPDATE accounts SET last_login='".time()."' WHERE username='".$_POST["user"]."'");

is open for sql injection attacks and XSS attacks. You should filter the input data from the user and use at least

mysql_real_escape_string()

if you are using mysql? And use htmlentities(), strip_tags().


Check this site out for help on security issues

http://phpsec.org/projects/guide/

Its an easy read and well worth it.

[Ryan Fait]

Cool, thanks!