Are there any security vulnerabilities in this PHP code?

Question

I just got a site to manage, but am not too sure about the code the previous guy wrote. I'm pasting the login procedure below, could you have a look and tell me if there are any security vulnerabilities? At first glance, it seems like one could get in through SQL injection or manipulating cookies and the ?m= parameter.

---

define ( 'CURRENT_TIME', time ()); / / Current time. 
define ( 'ONLINE_TIME_MIN', (CURRENT_TIME - BOTNET_TIMEOUT)); / / Minimum time for the status of "Online". 
define ( 'DEFAULT_LANGUAGE', 'en'); / / Default language. 
define ( 'THEME_PATH', 'theme'); / / folder for the theme. 

/ / HTTP requests. 
define ( 'QUERY_SCRIPT', basename ($ _SERVER [ 'PHP_SELF'])); 
define ( 'QUERY_SCRIPT_HTML', QUERY_SCRIPT); 
define ( 'QUERY_VAR_MODULE', 'm'); / / variable contains the current module. 
define ( 'QUERY_STRING_BLANK', QUERY_SCRIPT. '? m ='); / / An empty query string. 
define ( 'QUERY_STRING_BLANK_HTML', QUERY_SCRIPT_HTML. '? m ='); / / Empty query string in HTML. 
define ( 'CP_HTTP_ROOT', str_replace ( '\ \', '/', (! empty ($ _SERVER [ 'SCRIPT_NAME'])? dirname ($ _SERVER [ 'SCRIPT_NAME']):'/'))); / / root of CP. 

/ / The session cookie. 
define ( 'COOKIE_USER', 'p'); / / Username in the cookies. 
define ( 'COOKIE_PASS', 'u'); / / user password in the cookies. 
define ( 'COOKIE_LIVETIME', CURRENT_TIME + 2592000) / / Lifetime cookies. 
define ( 'COOKIE_SESSION', 'ref'); / / variable to store the session. 
define ( 'SESSION_LIVETIME', CURRENT_TIME + 1300) / / Lifetime of the session. 

////////////////////////////////////////////////// ///////////////////////////// 
/ / Initialize. 
////////////////////////////////////////////////// ///////////////////////////// 

/ / Connect to the database. 
if (! ConnectToDB ()) die (mysql_error_ex ()); 

/ / Connecting topic. 
require_once (THEME_PATH. '/ index.php'); 

/ / Manage login. 
if (! empty ($ _GET [QUERY_VAR_MODULE])) 
( 
  / / Login form. 
  if (strcmp ($ _GET [QUERY_VAR_MODULE], 'login') === 0) 
  ( 
    UnlockSessionAndDestroyAllCokies (); 

    if (isset ($ _POST [ 'user']) & & isset ($ _POST [ 'pass'])) 
    ( 
      $ user = $ _POST [ 'user']; 
      $ pass = md5 ($ _POST [ 'pass']); 

      / / Check login. 
      if (@ mysql_query ( "SELECT id FROM cp_users WHERE name = '". addslashes ($ user). "' AND pass = '". addslashes ($ pass). "' AND flag_enabled = '1 'LIMIT 1") & & @ mysql_affected_rows () == 1) 
      ( 
        if (isset ($ _POST [ 'remember']) & & $ _POST [ 'remember'] == 1) 
        ( 
          setcookie (COOKIE_USER, md5 ($ user), COOKIE_LIVETIME, CP_HTTP_ROOT); 
          setcookie (COOKIE_PASS, $ pass, COOKIE_LIVETIME, CP_HTTP_ROOT); 
        ) 

        LockSession (); 
        $ _SESSION [ 'Name'] = $ user; 
        $ _SESSION [ 'Pass'] = $ pass; 
        / / UnlockSession (); 

        header ( 'Location:'. QUERY_STRING_BLANK. 'home'); 
      ) 
      else ShowLoginForm (true); 
      die (); 
    ) 

    ShowLoginForm (false); 
    die (); 
  ) 

  / / Output 
  if (strcmp ($ _GET [ 'm'], 'logout') === 0) 
  ( 
    UnlockSessionAndDestroyAllCokies (); 
    header ( 'Location:'. QUERY_STRING_BLANK. 'login'); 
    die (); 
  ) 
) 

////////////////////////////////////////////////// ///////////////////////////// 
/ / Check the login data. 
////////////////////////////////////////////////// ///////////////////////////// 

$ logined = 0, / / flag means, we zalogininy. 

/ / Log in session. 
LockSession (); 
if (! empty ($ _SESSION [ 'name']) & &! empty ($ _SESSION [ 'pass'])) 
( 
  if (($ r = @ mysql_query ( "SELECT * FROM cp_users WHERE name = '". addslashes ($ _SESSION [' name'])."' AND pass = ' ". addslashes ($ _SESSION [' pass']). " 'AND flag_enabled = '1' LIMIT 1 ")))$ logined = @ mysql_affected_rows (); 
) 
/ / Login through cookies. 
if ($ logined! == 1 & &! empty ($ _COOKIE [COOKIE_USER]) & &! empty ($ _COOKIE [COOKIE_PASS])) 
( 
  if (($ r = @ mysql_query ( "SELECT * FROM cp_users WHERE MD5 (name )='". addslashes ($ _COOKIE [COOKIE_USER ])."' AND pass = '". addslashes ($ _COOKIE [COOKIE_PASS]). " 'AND flag_enabled = '1' LIMIT 1 ")))$ logined = @ mysql_affected_rows (); 
) 
/ / Unable to login. 
if ($ logined! == 1) 
( 
  UnlockSessionAndDestroyAllCokies (); 
  header ( 'Location:'. QUERY_STRING_BLANK. 'login'); 
  die (); 
) 

/ / Get the user data. 
$ _USER_DATA = @ Mysql_fetch_assoc ($ r); 
if ($ _USER_DATA === false) die (mysql_error_ex ()); 
$ _SESSION [ 'Name'] = $ _USER_DATA [ 'name']; 
$ _SESSION [ 'Pass'] = $ _USER_DATA [ 'pass']; 

/ / Connecting language. 
if (@ strlen ($ _USER_DATA [ 'language'])! = 2 | |! SafePath ($ _USER_DATA [ 'language']) | |! file_exists ( 'system / lng .'.$_ USER_DATA [' language '].' . php'))$_ USER_DATA [ 'language'] = DEFAULT_LANGUAGE; 
require_once ( 'system / lng .'.$_ USER_DATA [' language'].'. php '); 

UnlockSession (); 

Answer 1

Yes, there are a few vulnerabilities in this code.

This could potentially be a problem:

define ( 'QUERY_SCRIPT', basename ($ _SERVER [ 'PHP_SELF'])); 

PHP_SELF is bad because an attacker can control this variable. For instance try printing PHP_SELF when you access the script with this url: http://localhost/index.php/test/junk/hacked . Avoid this variable as much as possible, if you do use it, make sure you sanitize it. It is very common to see XSS crop up when using this variable.

1st Vulnerability:

setcookie (COOKIE_USER, md5 ($ user), COOKIE_LIVETIME, CP_HTTP_ROOT); 
setcookie (COOKIE_PASS, $ pass, COOKIE_LIVETIME, CP_HTTP_ROOT); 

This is a rather serious vulnerability. If an attacker had SQL injection in your application then they could obtain the md5 hash and the user name and login immediately without having to break the md5() hash. It is as if you are storing passwords in clear text.

This session vulnerability is two fold, it is also an "immortal session", Session id's must always be large randomly generated values that expire. If they don't expire then they are much easier to brute force.

You should NEVER re-invent the wheel, call session_start() at the very start of your application and this will automatically generate a secure session id that expires. Then use a session variable like $_SESSION['user'] to keep track if the browser is actually logged in.

2nd vulnerability:

$ pass = md5 ($ _POST [ 'pass']);

md5() is proven to be insecure because collisions have been intentionally generated. md5() should never be used for passwords. You should use a member of the sha2 family, sha-256 or sha-512 are great choices.

3rd Vulnerability:

CSRF

I don't see any CSRF protection for you authentication logic. I suspect that all requests in your application are vulnerable to CSRF.

Answer 2

The accepted answer is missing a lot and wrong about a few things. Here are the vulnerabilities that I see in the code:

define('QUERY_SCRIPT', basename($_SERVER['PHP_SELF']));

As said elsewhere, this can contain more than just the script path. Use $_SERVER['SCRIPT_NAME'] or __FILE__ instead.

define('CP_HTTP_ROOT', ...

This constant is used to set the cookie path to the script path. This isn't insecure but the user will need to log in separately to every script. Instead use sessions (discussed below) and set one base path for your app.

UnlockSessionAndDestroyAllCokies()

I don't know exactly what this is doing, but it doesn't sound good. Simply start the session early in the script for every request. Check for existing user information in the session to know if they're already logged in.

$pass = md5($_POST['pass']);

Passwords should have a unique salt with each hash, and preferably use a better hashing algorithm. These days (PHP 5.5+) you should be using password_hash and password_verify to take care of the password hashing details for you.

mysql_query("SELECT id FROM cp_users WHERE name = '". addslashes($user)...

This is an old question, but today the mysql_ PHP functions are no longer supported. Use mysqli or PDO. Using an unsupported library leaves you open to unpatched vulnerabilities. addslashes is not a perfect protection against SQL injection. Use prepared statements, or at least the library's string escape functions.

if (isset($_POST['remember']) && $_POST['remember'] == 1) 
( 
    setcookie(COOKIE_USER, md5($user), COOKIE_LIVETIME, CP_HTTP_ROOT);
    setcookie(COOKIE_PASS, $pass, COOKIE_LIVETIME, CP_HTTP_ROOT); 
) 

And here is the biggest problem. The cookies are storing user credentials. You are sending back a hashed username and hashed password as cookie values with the response. These can easily be read and used by a 3rd party. Since this script uses a simple hash, a rainbow table will let someone look up many of the user passwords. But since the response contains the "secured" credentials, an attacker wouldn't need to do anything other than pass them into any other request to log in. This is not the proper way to "remember" a user and isn't necessary.

A user's connection to a request should be handled only in the session. That's their purpose. The requests and responses contain a cookie with a random identifier. The user details remain only on the server and link to this identifier. (Side note: there's a bug that this block is wrapped with parentheses instead of braces.)

$_SESSION['Pass'] = $pass; 

Now the user's password is being stored in two places: the database and session storage. If the sessions are stored outside the database (and PHP stores them on disk by default), you now have two ways someone can try to steal user credentials.

if ($logined !== 1 && !empty($_COOKIE[COOKIE_USER]) ...
  if (($r = @mysql_query("SELECT * FROM cp_users WHERE MD5(name)='". addslashes($_COOKIE [COOKIE_USER ])."' AND pass = '". addslashes($_COOKIE [COOKIE_PASS])..

An attacker can now attempt logging in by simply passing cookie headers in a request with an md5 hash of a user name and password, which was handed to them in a previous response. The login form should be the only place taking user credentials and logging them in. This form (and all others) should use a CSRF token.

UnlockSession();

Again I don't know what this is doing, but at the end of the script the session should just be written to storage.