2
\$\begingroup\$

Drafted this up today out of the need for a very simple php based login form to protect an html page.

The app is simply included the top of index.html like this:

<?php require_once('./login.php'); ?>

Usernames are stored in a flat csv file 3 levels up (just above docroot). For this particular application I am not concerned about storing the passwords as plaintext since documents outside the docroot should be unreadable to anyone without server access.

The logins.csv file looks like this:

username1,password1 username2,password2

etc.

The authentication file itself uses an adaptation of this template and is as follows:

if (!session_id()) { ini_set('session.use_cookies', 'On'); ini_set('session.use_trans_sid', 'Off'); session_set_cookie_params(0, '/'); session_start(); } $title = '{title of this website}'; $favicon = '{url link to favicon image}'; $warning = false; if ($_SERVER['REQUEST_METHOD'] == 'POST') { $_SESSION['logged'] = validate($_POST); if ($_SESSION['logged']) { header("Location: " . $_SERVER['REQUEST_URI']); } else { $warning = "Invalid Login. Please Try Again."; } } $loginForm = ' <!DOCTYPE html> <html lang="en"> <head> <meta charset="utf-8" /> <title>' . $title . '</title> <link rel="shortcut icon" href="' . $favicon . '" /> <style> @import url(https://fonts.googleapis.com/css?family=Roboto:300); .login-page { width: 360px; padding: 8% 0 0; margin: auto; } .form { position: relative; z-index: 1; background: #FFFFFF; max-width: 360px; margin: 0 auto 100px; padding: 45px; text-align: center; box-shadow: 0 0 20px 0 rgba(0, 0, 0, 0.2), 0 5px 5px 0 rgba(0, 0, 0, 0.24); } .form input { font-family: "Roboto", sans-serif; outline: 0; background: #f2f2f2; width: 100%; border: 0; margin: 0 0 15px; padding: 15px; box-sizing: border-box; font-size: 14px; } .form button { font-family: "Roboto", sans-serif; text-transform: uppercase; outline: 0; background: #4CAF50; width: 100%; border: 0; padding: 15px; color: #FFFFFF; font-size: 14px; -webkit-transition: all 0.3 ease; transition: all 0.3 ease; cursor: pointer; } .form button:hover,.form button:active,.form button:focus { background: #43A047; } .warning { margin-bottom: 10px; color: red; } body { background: #76b852; /* fallback for old browsers */ background: -webkit-linear-gradient(right, #76b852, #8DC26F); background: -moz-linear-gradient(right, #76b852, #8DC26F); background: -o-linear-gradient(right, #76b852, #8DC26F); background: linear-gradient(to left, #76b852, #8DC26F); font-family: "Roboto", sans-serif; -webkit-font-smoothing: antialiased; -moz-osx-font-smoothing: grayscale; } </style> </head> <body> <div class="login-page"> <div class="form"> ' . ($warning ? '<div class="warning">' . $warning . '</div>' : '') . ' <form class="login-form" method="post" enctype="multipart/form-data"> <input autocomplete="off" type="text" value="" name="username" placeholder="username" /> <input autocomplete="off" type="password" value="" name="password" placeholder="password" /> <button type="submit">login</button> </form> </div> </div> </body> </html>'; if (empty($_SESSION['logged']) || !userNameExists($_SESSION['logged'])) { $_SESSION['logged'] = false; die($loginForm); } function validate($post) { $usernames = getUsernames(); $response = false; if (!array_diff(['username','password'], array_keys($post))) { if (isset($usernames[$post['username']]) && $usernames[$post['username']] == $post['password']) { $response = $post['username']; } } return $response; } function userNameExists($username) { $usernames = getUsernames(); if (isset($usernames[$username])) { return true; } else { return false; } } function getUsernames() { $csv = array_map("str_getcsv", file('../../../logins.csv',FILE_SKIP_EMPTY_LINES)); foreach ($csv as $i => $row) { $row = array_map("trim", $row); $usernames[$row[0]] = $row[1]; } return $usernames; }

Primarily I'm looking for design suggestions and possible security problems here. My goal is to have something with as few pieces as possible that gets the job done, looks decent, is fairly bullet proof and can serve as a drop in replacement for site that require an .htpasswd protected html file.

\$\endgroup\$

    2 Answers 2

    Reset to default
    2
    \$\begingroup\$

    I am not concerned about storing the passwords as plaintext since documents outside the docroot should be unreadable to anyone without server access.

    You should still be concerned about it. Passwords aren't hashed because you assume that the hashes are readable, but as second line of defense, in case the passwords do leak (for example via a backup, LFI, etc).

    session_set_cookie_params(0, '/');

    For a session cookie, you really want to set httponly to true, and possibly secure if you are using HTTPS.

    Note also that == isn't timing safe and that it is always better to use === instead (POST values are always treaded as string, so you won't have a problem like 0 == "password", but it's just good practice).

    Misc

    • validate is a rather generic name. At the very least it should be validateCredentials. It should also accept the specific variables it needs, instead of the whole POST array. So it may be validateCredentials($username, $password).
    • Do you really need the array_diff check? It seems to just add additional complexity.
    • Think about putting your CSS code in it's own file. That way it can be cached, and your code becomes more readable.
    • I'm not sure why you even need the userNameExists check, or why you are setting the session logged value to false if it is empty.
    • It doesn't matter here, but it's best practice to die after a header redirect.
    \$\endgroup\$
    3
    • \$\begingroup\$Thank you for the feedback - I'll answer the questions here. (1) array_diff is there to ensure that $_POST contains the required parameters. Either way I'll need to make a check to ensure that both are present (someone could be sending post data maliciously from somewhere else) - this is the easiest one liner I can think of.\$\endgroup\$
      – You Old Fool
      CommentedJul 6, 2016 at 22:03
    • \$\begingroup\$(2) userNameExists() is there so I session will be instantly destroyed if credentials are removed from the csv. It just checks to makes sure they still exist at time of page load regardless of the fact that the session might exist.\$\endgroup\$
      – You Old Fool
      CommentedJul 6, 2016 at 22:04
    • \$\begingroup\$Everything else you said is here very helpful and much appreciated! Thanks.\$\endgroup\$
      – You Old Fool
      CommentedJul 6, 2016 at 22:05
    1
    \$\begingroup\$

    Primarily I'm looking for design suggestions and possible security problems here.

    Here's the first one:

    I am not concerned about storing the passwords as plaintext

    You should ALWAYS be concerned and NEVER store passwords as plaintext.

    Use password()_hash to hash a password during registration. Use password_verify() to compare the password from login with the hashed password from registration. They are built-in php functions especially made for this purpose.

    Lists with plaintext passwords are a wet dream for malicious hackers. Not only is every account on your website compromised, every other account on any other website that uses the same password is compromised too. Enough people out there using the same password for everything.

    \$\endgroup\$
    3
    • 1
      \$\begingroup\$password_hash() and password_verify() i think\$\endgroup\$
      – DannyTheDev
      CommentedJul 7, 2016 at 10:43
    • 1
      \$\begingroup\$That's right. I edited the answer :)\$\endgroup\$
      – Max
      CommentedJul 7, 2016 at 11:47
    • \$\begingroup\$I had no idea about those (relatively new) built in functions. Great advice, thanks.\$\endgroup\$
      – You Old Fool
      CommentedJul 7, 2016 at 13:41

    Start asking to get answers

    Find the answer to your question by asking.

    Ask question

    Explore related questions

    See similar questions with these tags.