0
\$\begingroup\$

I am using the following code to password protect pages on my site. This is how I am including it on a page:

<?php include("/path/to/password_protect.php"); ?>

After that line I have the rest of the page code, which is not displayed if the user is not logged in.

This is the password protect code itself, included on every page I want secured:

password_protect.php

<?php

#  SETTINGS START

$LOGIN_INFORMATION = array(
  'admin' => 'admin'
);
// request login? true - show login and password boxes, false - password box only
define('USE_USERNAME', true);
// User will be redirected to this page after logout
define('LOGOUT_URL', '/hub');
// time out after NN minutes of inactivity. Set to 0 to not timeout
define('TIMEOUT_MINUTES', 0);
// This parameter is only useful when TIMEOUT_MINUTES is not zero
// true - timeout time from last activity, false - timeout time from login
define('TIMEOUT_CHECK_ACTIVITY', true);

#  SETTINGS END

// timeout in seconds
$timeout = (TIMEOUT_MINUTES == 0 ? 0 : time() + TIMEOUT_MINUTES * 60);
// logout?
if(isset($_GET['logout'])) {
  setcookie("verify", '', $timeout, '/'); // clear password;
  header('Location: ' . LOGOUT_URL);
  exit();
}
if(!function_exists('showLoginPasswordProtect')) {
// show login form
function showLoginPasswordProtect($error_msg) {
?>
<html>
<head>
  <title>Please enter password to access this page</title>
</head>
<body>

  <form method="post">
    <h3>Please enter password to access this page</h3>
    <font><?php echo $error_msg; ?></font><br />
<?php if (USE_USERNAME) echo 'Username:<br /><input type="input" name="access_login" /><br />Password:<br />'; ?>
    <input type="password" name="access_password" /><p></p><input type="submit" name="Submit" value="Submit" />
  </form>

</body>
</html>
<?php
  // stop at this point
  die();
}
}
// user provided password
if (isset($_POST['access_password'])) {
  $login = isset($_POST['access_login']) ? $_POST['access_login'] : '';
  $pass = $_POST['access_password'];
  if (!USE_USERNAME && !in_array($pass, $LOGIN_INFORMATION)
  || (USE_USERNAME && ( !array_key_exists($login, $LOGIN_INFORMATION) || $LOGIN_INFORMATION[$login] != $pass ) )
  ) {
    showLoginPasswordProtect("Details Incorrect");
  }
  else {
    // set cookie if password was validated
    setcookie("verify", md5($login.'%'.$pass), $timeout, '/');

    // Some programs (like Form1 Bilder) check $_POST array to see if parameters passed
    // So need to clear password protector variables
    unset($_POST['access_login']);
    unset($_POST['access_password']);
    unset($_POST['Submit']);
  }
}
else {
  // check if password cookie is set
  if (!isset($_COOKIE['verify'])) {
    showLoginPasswordProtect("");
  }
  // check if cookie is good
  $found = false;
  foreach($LOGIN_INFORMATION as $key=>$val) {
    $lp = (USE_USERNAME ? $key : '') .'%'.$val;
    if ($_COOKIE['verify'] == md5($lp)) {
      $found = true;
      // prolong timeout
      if (TIMEOUT_CHECK_ACTIVITY) {
        setcookie("verify", md5($lp), $timeout, '/');
      }
      break;
    }
  }
  if (!$found) {
    showLoginPasswordProtect("");
  }
}
?>

This is not to secure anything highly sensitive (e.g. credit card details), but is to password protect analytics dashboards for client websites. To that end it needs to be moderately secure. My questions would be:

1) Are there any improvements I could make to the code to make it more streamlined?

2) Are there any large security issues that I need to address? As I said above it is not protecting hugely sensitive data but if there is a major problem or an easy improvement that would be great.

Thanks for any feedback!

\$\endgroup\$
  • 1
    \$\begingroup\$ There should be no difference regardless of whether you want to protect your money or your funny cat photo. You want authorized access only or not. Before even skimming through the code, why reinventing the wheel instead of using simple HTTP Basic Authentication? \$\endgroup\$ – fracz Jun 15 '16 at 15:35
  • \$\begingroup\$ @Mike, I took the time to give you thorough feedback when no one else did; You said my answer was really helpful but you neither upvoted not selected it. Why is that? \$\endgroup\$ – BeetleJuice Jul 4 '16 at 4:35
  • \$\begingroup\$ @BeetleJuice because I haven't revisited it due to working. Done now \$\endgroup\$ – Mike Harrison Jul 4 '16 at 8:24
2
\$\begingroup\$

I have 2 concerns with your code

  1. Mixing PHP and HTML works, but it's difficult to follow (which means bug-prone, not scalable). It's much better to separate your business logic from the display as much as possible.

  2. Having a default login of admin:admin is definitely not a good idea. It's probably the first thing anyone without credentials would try.

As @fracz suggested, HTTP Basic Authentication would fit your needs well, and it's really easy to use. I would probably do something like:

<?php
$LOGIN_INFORMATION = [
    'joe' => 'fkwoeiwsfrj3', 'jill' => '9ioDdoODidosD'
];

/**
 * Returns TRUE if authentication is successful. FALSE otherwise.
 * @param array $registeredLogins All acceptable logins
 * @return bool
 */
function isAuthenticated($registeredLogins){

    $authenticated = null; //make true if authentication succeeds

    //Server vars below will contain what user entered in the login prompt
    $login = isset($_SERVER['PHP_AUTH_USER'])? $_SERVER['PHP_AUTH_USER']: null;
    $pass = isset($_SERVER['PHP_AUTH_PW'])? $_SERVER['PHP_AUTH_PW']: null;

    if (!$login || !$pass):
        //required username or password is missing
        $authenticated = false;
    else:
        //username and password were provided. Are they accurate?
        $authenticated = array_key_exists($login, $registeredLogins)
                            && $registeredLogins[$login] === $pass;
    endif;

    return $authenticated;
}

/**
 * Exits and causes the browser to show user a login prompt
 */
function requestCredentials(){
    header('WWW-Authenticate: Basic realm="Login to Analytics Dashboard"');
    http_response_code(401);
    die("Invalid username or password");
}

//check for authentication. Require it if missing.
if(!isAuthenticated($LOGIN_INFORMATION)) requestCredentials();
?>

<html>
<!-- your page goes here. Will only display if user is authenticated -->
</html>

I have simplified things a bit, so you will need to adjust it to your specific situation. With this scheme, once the user enters username and password that is accepted, the browser will keep automatically sending credentials with new requests made to the same domain. Here are the details.

A couple of important things to keep in mind:

  1. Whatever your security logic, if you don't use TLS (HTTPS), the client and the site will be vulnerable. Any attacker can just read your user's verify cookie, or the username and password transmitted. The attacker can then use this info to impersonate the user and gain access. This affects both your code and what I suggested. Use TLS if you can. It's free.

  2. Basic Authentication doesn't have a clean logout system (a way for the server to tell the browser to stop sending credentials and to delete them). This can be worked around easily enough though. The reference I linked above goes over this. Have a logout function that redirects the user (by setting the Location header and then exiting) to a page on the same domain with bad credentials hard-coded; eg: header("Location: http://badusername:badpass@www.example.com/logout.html");exit. Call the logout function before you echo anything to the browser and the browser will forget the old credentials, remember the bad credentials and redirect the user to logout.html. User will be forced to enter proper credentials again if he wants access.

  3. Because browsers will automatically attach credentials to all requests made to the domain, even the pages where you do not include password_protect.php will be queried with these credentials. This means that with this solution, TLS should be used not just on the protected pages, but on all domain's pages.

\$\endgroup\$
  • \$\begingroup\$ Thanks very much for your detailed feedback that is really helpful. This solution looks perfect, I am using Let's Encrypt already so TLS is running site-wide. The only issue is using this method I lose control over the styling of the login form as it is generated by the browser. Is there a way to combine this with an HTML form, so I can style the login experience? Thanks again for your help, I hadn't come across HTTP Basic Authentication before \$\endgroup\$ – Mike Harrison Jun 20 '16 at 10:26
  • \$\begingroup\$ To use an HTML form you'll need to do a little more work, closer to your initial implementation but you can still put some of my code to use. Just try to keep your logic separate from your HTML. 1) get credentials from $_POST and test them, 2) Build up the login form in a string entirely in PHP; save it as $login_form. 3) If credentials are missing or bad, echo $login_form; exit;. 4) We're done with the logic: close PHP code block ?> and build your regular page's HTML. \$\endgroup\$ – BeetleJuice Jun 20 '16 at 11:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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