2
\$\begingroup\$

I have a site where a friendly hacker says they have accessed my admin area. I am not able to see how this is possible so any help would be greatly appreciated. I am using PDO prepared statements to connect to my MySQL database.

Login Page (this is where the user logs in):

<?php 
require_once('inc/constants.inc.php');

if(!isset($_SESSION)){
    session_start();
}


if(isset($_SESSION['LoggedIn']) && isset($_SESSION['Username']) && $_SESSION['LoggedIn'] == 1){
    //echo "<h1>YES, YOU ARE LOGGED IN</h1>";
    if(isset($_SESSION['returnURL'])){
        if($_SESSION['returnURL'] == "/admin/interactions.php"){
            header("Location: index.php");
        }else{
            header("Location: " . $_SESSION['returnURL']);
        }

    }else{
        header("Location: index.php");
    }

}


if(!empty($_POST['useremail']) && !empty($_POST['password'])){
    require_once("inc/class.users.inc.php");
    $users = new users();
    if($_POST['whichForm'] == "login"){
        $theResult = $users->accountLogin();
    }else if($_POST['whichForm'] == "facebook"){
        $theResult = $users->accountLoginFB();
    }

    if($theResult == true){
        if(isset($_SESSION['returnURL'])){
            header("Location: " . $_SESSION['returnURL']);
        }else{
            header("Location: index.php");
        }
    }else{ //If it couldn't register
        $theError = true;
    }
}else if(isset($_POST['whichForm'])){
    $theError = true;
}
else{
    $theError = false;
}

$pageTitle = "Login";
$pageKeywords = "Login page";
$loggedIn = false;

require_once($_SERVER['DOCUMENT_ROOT'] . '/common/constants.php');
include_once($_SERVER['DOCUMENT_ROOT'] . "/common/header.php"); 
include_once($_SERVER['DOCUMENT_ROOT'] . "/common/mainNav.php");?>



<section class="container">
    <div id="blankBuffer" class="carousel-buffer"></div>
    <section class="container" id="mainContent">
        <div class="loginBox clearfix">
            <div class="span5 offset3 clearfix">
                <div class="well">
                    <legend>Sign in to Store Portal</legend>
                    <?php if($theError): ?>
                        <div class="alert alert-error">
                            <a class="close" data-dismiss="alert" href="#">x</a>Incorrect Username or Password!
                        </div>
                    <?php endif;//end if($theError) ?>
                    <form method="POST" action="login.php" name="loginform" id="loginform" class="cf" accept-charset="UTF-8">
                        <input type="hidden" name="whichForm" id="whichForm" value="login"/>
                        <input class="span4" placeholder="User Email" type="text" name="useremail" id="useremail"><br>
                        <input class="span4" placeholder="Password" type="password" name="password" id="password" ><br>
                        <button class="btn" type="submit">Login</button>
                    </form>
                </div>
            </div>

            <div class="span5 offset3">
                <div class="well info">
                    <p>Don't have an account? <a href="signup.php">Click here to sign up.</a></p>
                </div>
            </div>
        </div><br />
    </section>
</section>

Account login function (this is the function that performs the check if username and password match a record in the db and saves information into the Session Variable):

public function accountLogin()
    {
        $emailExplode = explode("@", $_POST['useremail'], 2);
        $userName = $emailExplode[0];


        if(count($emailExplode) == 1){
            $theEmail = $userName . "@" . str_replace("www.", "", $_SERVER['SERVER_NAME']);
            //$theEmail = $userName;
        }else{
            $theEmail = $_POST['useremail'];
        }

        //NEED TO SEPERATE THE CREATION FROM LOGIN

        $sql = "SELECT UserID, Username, user_edit FROM users WHERE user_email = :user AND Password = MD5(:pass) LIMIT 1";

        try{
           $stmt = $this->_db->prepare($sql);
           $stmt->bindParam(':user', $theEmail, PDO::PARAM_STR);
           $stmt->bindParam(':pass', $_POST['password'], PDO::PARAM_STR);
           $stmt->execute(); 
           if($stmt->rowCount() == 1){
                $row = $stmt->fetch(PDO::FETCH_ASSOC);
                $_SESSION['Username'] = htmlentities($userName, ENT_QUOTES);
                $_SESSION['UserID'] = $row['UserID'];
                $_SESSION['LoggedIn'] = 1;
                $_SESSION['Edit'] = $row['user_edit'];

                $log = $_SERVER['DOCUMENT_ROOT'] . "/admin/inc/loggedUsers.txt";

                $theEntry = date("Y-m-d H:i:s") . " - " . htmlentities($userName, ENT_QUOTES) . "\n";

                $handle = fopen($log, "a+") or die("can't open file");
                fwrite($handle, $theEntry);
                fclose($handle);

                return TRUE;
           } else{
               return FALSE;
           }

        }catch(PDOException $e){
            return FALSE;
        }
    }

This snippet is included on all protected pages to verify that the user is actually logged in and has editing capabilities:

<?php
//THIS DOCUMENT CHECKS IF A PERSON IS LOGGED IN AND HAS EDITING CAPABILITIES. IF TRUE, THEY ARE ALLOWED TO VIEW THE PAGE, OTHERWISE IF THEY AREN'T LOGGED IN THEY ARE REDIRECTED TO THE LOGIN PAGE. IF THEY DON'T HAVE EDITING CAPABILITIES, THEY ARE SENT TO HOME PAGE WITH AN ERROR MESSAGE.


if(!isset($_SESSION)){
    session_start();
}

if($_SERVER['REQUEST_URI'] != "/admin/interactions.php"){
    $_SESSION['returnURL'] = $_SERVER['REQUEST_URI'];
}

if(isset($_SESSION['LoggedIn']) && isset($_SESSION['Username']) && $_SESSION['LoggedIn'] == 1){
    $loggedIn = true;
    $canEdit = $_SESSION['Edit'] + 0;

    if($canEdit != 1){
        header("Location: http://" . $_SERVER['SERVER_NAME'] . "?error=noAccess");
    }
}else{
    header("Location: http://" . $_SERVER['SERVER_NAME'] . "/admin/login.php");
}?>
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! Please edit your title & question to say something about what your code does and why, to give a bit of context. \$\endgroup\$
    – Phrancis
    Commented Mar 21, 2016 at 20:43

1 Answer 1

4
\$\begingroup\$

Security

Redirect and Broken Authentication

A client does not have to follow a header redirect. This means that code below such a redirect can be executed by anyone.

This means that your whole authentication is likely broken (it depends how exactly you use the verify code, and if you always check loggedIn). But at the very least, the redirect gives a false sense of security.

A simple die after a redirect will fix this.

Hashing

md5 is not acceptable for hashing, and it hasn't been for at least 15 years. Use bcrypt. You can use password_hash function which was made specifically for that. Example:

password_hash("rasmuslerdorf", PASSWORD_DEFAULT);
// $2y$10$.vGA1O9wmRjrwAVXD98HNOgsNpDczlqm3Jq7KnEd1rVAGv3Fykk1a

CSRF

You do not seem to have CSRF protection for your login. For an admin login this may be ok, but it's bad practice.

If it's actually ok depends on a couple of factors. For example, I could imagine that less privileged users - eg mods - use the same login as the admin. Then, a mod may forcefully log an admin in their account via CSRF. The admin may think that they are (still) logged into their actual account, and may disclose sensitive information - eg when sending a private message, which the mod could then read in the outbox.

Misc

  • there is likely a timing issue with your password check, as the whole thing takes place in a select query, which isn't timing safe. Remote timing attacks are likely not possible today, or only possible in limited circumstances, but it's still something to consider. PHPs bcrypt will give you a free timing safe string compare.
  • you shouldn't use ==, but ===. It's likely not an issue in your cases, but just good practice, especially regarding security.
  • you should regenerate the session id when the session changes (eg on login), to prevent session fixation (current default php.ini settings will prevent it as well, but you shouldn't rely on it).
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.