Is this login script safe?

Question

Okay, essentially what my question is asking is if their is any major security threats using the login script below and if so what can I use to prevent any SQL injections or users logging into my service without providing correct details? I understand that I'm using an outdated version of PHP however my whole site is built on this previous version and I understand this poses risks using a outdated build.

Login Form

<form method="post" action="login.php?login=login">

<input type='text' placeholder="username" class='form-control' name='username' required autofocus/>
<input type='password' placeholder="password" class='form-control' name='password' required/>
<input class='btn btn-default btn-block'  type='submit' value='Login' class='submit' />

</form>

Form Post

<? if($login==login)
      {
      $username = clean($_POST[username]);
      $password = md5($_POST[password]);
      $date = date("Y-m-d");
      $time = date("H:i:s");
      $sql = mysql_query("select * from users where username = '$username' AND password = '$password'");
      $check = mysql_num_rows($sql);
      if($check!=1)
      {
      echo 'Incorrect username or password.';
      echo('<meta http-equiv="refresh" content="3;url=/login" />');
      $success = "Failed";
      if($content[loginlog]==1)
      $sqllog = mysql_query("insert into usr_logs(user, ip, time, date, success) values('$username', '$ip', '$time', '$date', '$success')");
      }
      else
      {
      $user = mysql_fetch_array($sql);
      $_SESSION[usr_name] = $user[username];
      $_SESSION[usr_level] = $user[level];
      $_SESSION[usr_ip] = $ip;
      $success = "Success";
      echo('<meta http-equiv="refresh" content="1;url=/home" />');
      if($content[loginlog]==1)
        $sqllog = mysql_query("insert into usr_logs(user, ip, time, date, success) values('$username', '$ip', '$time', '$date', '$success')");
      }
      }
      if($login==logout)
      {
      session_unset();
      session_destroy();
      echo 'logged out';
      echo('<meta http-equiv="refresh" content="3;url=/login" />');
      }?>

Thankyou for helping me improve the security of my code and preventing any SQL injections.

I'm voting to close this question as off-topic because questions asking about the quality of working code usually belong on [codereview](http://codereview.stackexchange.com/help/on-topic) — Quentin, Apr 23 '16 at 20:56
There are many terrible things about this code, and the `clean` function (which actually does the stuff you are primarily asking about!) is missing entirely. — Quentin, Apr 23 '16 at 20:57
I have never had to make my code safe to prevent injections so don't know the first thing about making my code safe that is why I have posted it here to start a discussion about this. — , Apr 23 '16 at 21:00
@MarkBaker Thats comment is not clear I assume you are saying **dont use it** use `password_hash()` and `password_verify()` instead — RiggsFolly, Apr 23 '16 at 21:05
Another issue! You are using the `mysql_` database extension **Bad Bad Boy** Please dont use [the `mysql_` database extension](http://stackoverflow.com/questions/12859942/why-shouldnt-i-use-mysql-functions-in-php), it is deprecated (gone for ever in PHP7) Specially if you are just learning PHP, spend your energies learning the `PDO` database extensions. [Start here](http://php.net/manual/en/book.pdo.php) — RiggsFolly, Apr 23 '16 at 21:07
I think what we are all trying to say is _Of all the bad, insecure things you could possible do, this code has done most of them_ — RiggsFolly, Apr 23 '16 at 21:08
Also a bit of sensible code indentation would aid in readability and also more importantly in **debugging** — RiggsFolly, Apr 23 '16 at 21:11
the question i suppose i should be asking now that I know its bad is would someone know from browsing my website thats its that bad lolol aha what would have to be typed into this login script to break it? if I just put a prevention before this script to disallow any other letters other than the alphabet to be allowed through is that enough? — , Apr 23 '16 at 21:13
@Lewis Relying on someone *not* finding your bugs is referred to as [security through obscurity](https://en.wikipedia.org/wiki/Security_through_obscurity). So no, that's not the question you should be asking if you want any level of real security. Keep that site on your LAN for now unless you don't care. — jDo, Apr 23 '16 at 21:17
This site would likely as not fall at the first even beginners attempt as hacking — RiggsFolly, Apr 23 '16 at 21:17
You should also not use bare strings ever in PHP. Unless those are really named constants, quote your array keys. — Paul, Apr 23 '16 at 21:23

score 2 · Accepted Answer · edited Mar 17 '17 at 13:14

First of all, what is clean() function?

Secondly, don't use MD5 encryption method, use at least sha256. This is because MD5 is bad for hashing password due to its security weaknesses and due to it being too fast. If you want more security use ~~salt combined with sha256~~ the password_hash() function. And here's why.

Thirdly, you are passing the user input to the database just like they are, which means your script is vulnerable to SQL Injection. You should use Prepared Statements. An official documentation here explains how to use them.

Fourthly, you should replace these lines:

echo('<meta http-equiv="refresh" content="3;url=/login" />');
echo('<meta http-equiv="refresh" content="1;url=/home" />');

with these one:

header('Location: /login');
header('Location: /home');

The reason is that use of meta refresh is discouraged by the World Wide Web Consortium. More information can be found here.

Last but not least, you are using the old deprecated mysql extension. You should use mysqli or PDO extension to access the database.

@RiggsFolly Yes, you are right. I prefer to use mysqli, tho. I'll edit my answer. — Menma, Apr 23 '16 at 21:24

score 0 · Answer 2 · answered Apr 23 '16 at 21:31

The biggest problem with your code is the use of MD5 which can be broken with rainbow tables in seconds. In terms of SQL injection, it looks like you have some big problems around

''mysql_query("insert into usr_logs(user, ip, time, date, success) values('$username', '$ip', '$time', '$date', '$success')");

Say $username='drop table users...' or something. Not good.

In theory php versions from 5ish onwards should deal with this for you but personally I'd rather trust a bank robber to look after my wallet.

Is this login script safe?

2 Answers2