Login keeps getting hacked

Question

I have the below login script which I believe is secure. However, someone keeps getting access to the administration section of the site and changing content.

if(isset($_POST['submit'])) {
$error = false;
$user_login = stripslashes(strip_tags(htmlentities($_POST['user_login'])));
$pass_login = stripslashes(strip_tags(htmlentities($_POST['pass_login'])));
if(!empty($user_login) && !empty($pass_login)) {
    $check_details=mysql_query("SELECT * FROM `admin` WHERE email='".$user_login."' AND password='".md5($pass_login)."'");
    $status=mysql_num_rows($check_details);
    if($status >= "1") {
        $error = false;
        $_SESSION['wmmadmin_loggedin'] = "1";
        $_SESSION['wmmadmin_email'] = "".$user_login."";
        header("Location: ./index.php");
    }
    if(!$status || $status == "0") {
        $error = true;
        echo "<div id=\"error\"><strong>Error!</strong><br />Login details were incorrect.</div>\n";
    }
}
if(empty($user_login) || empty($pass_login)) {
    $error = true;
    echo "<div id=\"error\"><strong>Error!</strong><br />Enter your username and password.</div>\n";
}

}

At the top of every script there is a function call:

function checkloggedin() {
if($_SESSION['wmmadmin_loggedin'] == "0" || $_SESSION['wmmadmin_loggedin'] !== "1" ||     $_SESSION['wmmadmin_email'] == "") {
header("Location: login.php");
exit;
}
}

Am I missing something? I need to stop these hackers!

Thanks Pete

You are wide open to [SQL injections](http://stackoverflow.com/q/60174). mystery solved. — John Conde, May 01 '13 at 16:12
You need to rewrite with non-deprecated functions **(NOT `mysql_*)`** — brbcoding, May 01 '13 at 16:13
Phew, `stripslashes(strip_tags(htmlentities($str)))` is an interesting one - where did you get that from? `:-)`. Use `mysql_real_escape_string($str)` instead ([see here](http://php.net/manual/en/function.mysql-real-escape-string.php)). — halfer, May 01 '13 at 16:14
To add some detail to the above calls to switch to a different database library: it's good advice, though you should know that it is entirely possible to run safely on this deprecated system. — halfer, May 01 '13 at 16:15
suggestion: `if($check_details) { if($status===1) { ... } }` — Waygood, May 01 '13 at 16:19
@Waygood what will that do? Doesn't the query need a `like` for the `%` to act as a wildcard? — brbcoding, May 01 '13 at 16:29
Your code is also vulnerable to http://php.net/security.globals I've seen several web hosts with this option enabled. Better check the same — kranthi117, May 01 '13 at 16:33
Why is everything a string? `"1"`, `"0"`, and so forth? Use `true`, `false` and other constants, numerical literals like `42` and `0.123` (*not wrapped in quotes*) and casting such as `(string) $value`. — Dan Lugg, May 01 '13 at 16:44

score 11 · Answer 1 · answered May 01 '13 at 16:19

11

Someone could send the following as user_login:

user_login="nobody' OR 1 OR email='nobody"

This will result in a query

… WHERE email='nobody' OR 1 OR email='nobody' AND password='…'

which is interpreted as

… WHERE email='nobody' OR 1 OR (email='nobody' AND password='…')

so since the middle part is true (1 means true to MySQL), everything is true and access is granted. This is a classical SQL injection attack.

answered May 01 '13 at 16:19

MvG

57,380
22
148
276

1

To expand on this answer, using PDO with prepared statements is a great way to prevent this. I posted an example here: http://stackoverflow.com/questions/7246389/php-that-is-return-an-json-array-is-showing-up-as-null-in-javascript/7259534#7259534 – CountMurphy May 01 '13 at 16:25

score 4 · Answer 2 · answered May 01 '13 at 16:29

Let's see how you violate the rule of proper escaping for context:

$user_login = stripslashes(strip_tags(htmlentities($_POST['user_login'])));

Assuming the user name DOES contain HTML tags. The <> will first be converted to entities: <>.

Then strip_tags() tries to filter tags - but there are none! Useless function call.

Then you want to remove slashes. Are there any? This function call is only useful if magic quotes are still enabled in your PHP, and you should check if they are, not blindly remove them.

htmlentities() does only convert double quotes to entities, not single quotes. And your SQL uses single quotes to terminate the strings. So the whole bunch of functions used still lets SQL injection through.

Do not invent security functions yourself. Use mysql_real_escape_string() and double quotes in your SQL, and you are save. Or use prepared statements by using mysqli or PDO.

Laurence Moroney · Answer 3 · 2013-05-01T17:55:36.320

0

You need to sanitize $user_login and $pass_login more than just checking if they are empty. Someone could pass SQL code in them, and do a sql injection attack.

See here: http://php.net/manual/en/security.database.sql-injection.php

edited May 01 '13 at 17:55

answered May 01 '13 at 16:16

Laurence Moroney

1,263
8
20

Login keeps getting hacked

3 Answers3