-4
\$\begingroup\$

Could anyone tell me if this code is well written and if it has bugs / vulnerabilities?

class.user.php:

<?php
    class User {
        public $authorized = false;
        public $uid;
        public $username;


        public function __construct() {
            $this->db = new PDO($dsn, $db_user, $db_pass);
            $this->db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);

            if (isset($_SESSION['uid'])) {
                $this->authorizsed = true;
                $this->uid = $_SESSION['uid'];
                $this->username = $_SESSION['username'];
            } else if (isset($_POST['reset'])) {
                $user = $_POST['reset'];
                $this->reset($user);
            } else if (isset($_POST['username']) && isset($_POST['password'])) {
                $user = $_POST['username'];
                $pass = $_POST['password'];
                $this->login($user, $pass);
            }
        }


        private function login($user, $pass) {
            $st = $this->db->prepare('SELECT `uid`, `username`, `password`
                    FROM users
                    WHERE username = :u');
            $st->execute(array(':u' => $user));
            $row = $st->fetch();

            if ($row && $row->password == sha1($pass)) {
                $this->authorized = true;

                $this->uid = $row->uid;
                $_SESSION['uid'] = $this->uid;

                $this->username = $row->username;
                $_SESSION['username'] = $this->username;

                return true;
            } else {
                return false;
            }
        }


        private function reset($user) {
            $st = $this->db->prepare('SELECT `uid`, `username`, `email`
                    FROM users
                    WHERE username = :u');
            $st->execute(array(':u' => $user));
            $row = $st->fetch();

            if ($row) {
                $token = $this->generateRequest();

                $st = $this->db->prepare('UPDATE users SET `reset` = :reset, password = 0 WHERE uid = :uid LIMIT 1');
                $status = $st->execute(array(':uid' => $row->uid, ':reset' => $token));

                $body = "We received a request for your account details.<br/><br/>Username: {$row->username}<br/>To reset your password, click on this link: <a href='http://www.example.org/?reset={$token}'>http://www.example.org/?reset={$token}/a>";

                $to = $row->email;
                $subject = 'Password request';
                $from = 'no-reply@example.org';

                // To send HTML mail, the Content-type header must be set
                $headers  = 'MIME-Version: 1.0' . "\r\n";
                $headers .= 'Content-type: text/html; charset=iso-8859-1' . "\r\n";

                // Create email headers
                $headers .= 'From: '.$from."\r\n".
                            'Reply-To: '.$from."\r\n";

                mail($to, $subject, $body, $headers)
            }
        }

        private function generateRequest() {
            $token = md5(openssl_random_pseudo_bytes(32));
            return $token;
        }

    }
?>

index.php:

<?php
    session_start();

    require('class.user.php');
    $user = new User();

?>

<!doctype html>

<html lang="en">
    <head>
        <meta charset="utf-8">
        <title>Login</title>

        <link rel="stylesheet" href="styles.css">
    </head>

    <body>
        <?php
            if ($user->authorized):
        ?>
                Welcome, <?=$user->username;?>!
        <?php
            elseif (isset($_GET['forgot'])):
        ?>
            <div class="module form-module">
                <div class="form">
                    <h2>Reset password</h2>
                    <?php if (isset($_POST['reset'])): ?>
                        <div class="success">Email sent</div>
                    <?php endif; ?>
                    <form method="POST">
                        <input type="text" name="reset" placeholder="Username"/>
                        <button>Reset</button>
                    </form>
                </div>
                <div class="cta"><a href="./">Login</a></div>
            </div>
        <?php
            else:
        ?>
            <div class="module form-module">
                <div class="form">
                    <h2>Login to your account</h2>
                    <?php if (isset($_POST['username']) && isset($_POST['password'])): ?>
                        <div class="error">Invalid login</div>
                    <?php endif; ?>
                    <form method="POST">
                        <input type="text" name="username" placeholder="Username"/>
                        <input type="password" name="password" placeholder="Password"/>
                        <button>Login</button>
                    </form>
                </div>
                <div class="cta"><a href="?forgot">Forgot your password?</a></div>
            </div>
        <?php
            endif;
        ?>
    </body>
</html>

Is it possible that somehow, someone bypass the reset system ?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Dan Jul 3 '16 at 15:02
  • \$\begingroup\$ Note: $dsn, $db_user, $db_pass isn't in the local scope of __construct(), so you won't have a connection to your database. \$\endgroup\$ – hd. Jul 5 '16 at 14:08
  • 1
    \$\begingroup\$ This code was plagiarized from a programming challenge: github.com/HackThis/real7-login/blob/master/index.php \$\endgroup\$ – 200_success Apr 9 '17 at 2:46
6
\$\begingroup\$

I generally like your code. There are some structural issues that may cause problems if your code base grows larger and some minor security issues, but apart from that, it is quite readable, well formatted, you follow some security best-practices like using prepared statements, etc.


Security

You didn't actually show us the code that accepts the reset token, but the request for a reset looks mostly ok.

There are some minor to medium issues though:

  • sha1 isn't good enough, use bcrypt.
  • always use ===. It doesn't matter in this case, but it's good to get used to it as using == can lead to some pretty nasty bugs in some cases.
  • if you don't have some general CSRF protection for all POST requests that you didn't post, you are vulnerable to login CSRF.
  • you are probably vulnerable to XSS via the username, which could be exploited in case you are vulnerable to login CSRF.
  • you should store the reset token in a hashed form (your call to md5 doesn't count, as you are sending that value to the user as well as storing it in the DB).
  • it is typical to ask for an email address when resetting a password, not just a username. Usernames are often displayed, so someone may request a new password for users, which may slightly annoy them. Don't trust the input though, compare it to the email address stored in the database.

Structure

Your User class decides for itself what it should do via the constructor, which isn't ideal, as it makes code hard to test and reuse.

You are also mixing controller, model, and database access code. You don't have to follow the MVC model 100%, but some separation would help readability.


Misc

  • you can get rid of variables that are only used once and whose name doesn't add any value. One example would be $user and $pass when calling login($user, $pass).
  • try to be consistent with your names, it will increase readability (eg username vs user vs u).
  • $this->authorizsed is misspelt, meaning that the field will always be false. You should think about using an IDE with auto-complete to avoid these sort of bugs in the future.
  • generateRequest doesn't actually generate a request, but a random token. generateToken would be more fitting.
  • I would prefer to use a templating engine instead of mixing PHP and HTML. It results in nicer code, and depending on the engine also gives you some amount of automated XSS protection.
\$\endgroup\$
7
  • \$\begingroup\$ Thanks a lot for everything ! I'm now fixing all these things you listed in your golden message About the CSRF and XSS, how would you do to exploit them ? \$\endgroup\$ – Lulzsec Jul 1 '16 at 23:11
  • \$\begingroup\$ @tim put a lot of work in this response. If he answered your request, why not select his answer? Or if it's not what you're looking for but you found it helpful (obviously you did from your reply), why not an up vote? Just trying to understand. \$\endgroup\$ – BeetleJuice Jul 2 '16 at 4:14
  • 1
    \$\begingroup\$ @Lulzsec For the CSRF, I could for example log a victim into an account that I control, in the hope that the victim divulges information under that account (an example would be the sending of a confidential message). If further requests of your app are vulnerable, I could perform any action the victim can perform. \$\endgroup\$ – tim Jul 2 '16 at 6:38
  • 1
    \$\begingroup\$ @Lulzsec For the XSS, I would create a new user with the XSS payload in the name, and log the victim into that account via the CSRF. Now I could for example attack the admin area or further apps on the same domain (because I can now bypass any CSRF protection that they may have), perform phishing attacks (please re-enter your password, ...), etc. \$\endgroup\$ – tim Jul 2 '16 at 6:39
  • \$\begingroup\$ @BeetleJuice, I'm pretty much new to this website, sorry for the disagreement, I've marked his answer as the good answer and I've up voted the post \$\endgroup\$ – Lulzsec Jul 2 '16 at 12:50
9
\$\begingroup\$

✔ Binding parameters

You're using a database API and successfully binding your parameters. This will prevent SQL injection.

✘ No character white-lists

Since you're outputting user input (ie: usernames) on-screen, you best to validate the input against a whitelist/blacklist, and passing the output through htmlentities. This will prevent abuse, such as images as usernames, or JavaScript (which can lead to persistent XSS)

Suggested action: Passing user input through character validations. For example preg_match().

✘ Password hashing

Your current approach to password hashing is poor. PHP has a built-in library to deal with passwords.

Suggested action: Check out password_hash and reading PHP: Password Hashing FAQ

✘ No time-limit on password resets

Currently, on password reset, you're just writing a token to the database and (it doesn't seem) giving this token a timeout. So, I could generate a password reset for your admin account, then brute force the reset token.

Also, you have no rate request limit on requesting a password reset and logging in. Someone could brute force every account to password reset status (or login), then brute force each token - though that will take some time, there's currently (in the application layer) anything preventing it.

Suggested action: Giving the tokens a timeout of ~15minutes or lower. Adding a CAPTCHA on request password reset and login (or after x failed login attempts).

✘ No session binding

As it stands, you're not binding the session value (commonly PHPSESSID) to anything user specific to help tackle session hijacking which could occur elsewhere in the application.

Suggested action: Bind the PHPSESSID to the users details (ie: their IP address and user agent. Though this can be spoofed, it will help tackle the attack)


Other notes

  • $dsn, $db_user, and $db_pass isn't in the local scope of __construct(), so you won't have a connection to your database.
  • Your application will become very messy if you don't adopt a design patter, such as MVC - especially with multiple outputs based on conditions (such as checking if the user is logged in or not)
  • Ensure you don't have display_errors on (in production), as this could leak some information that can help attackers.
  • You'll want to start commenting your code as when your project becomes bigger, you'll wish you had.
  • You'll want to start adopting the DRY principle. For example, the query that fetches a user from a database in the reset() method will be used more than once. Move that logic to its own method.
\$\endgroup\$

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