0

I would kindly ask you to review the following code and tell me if it's enough to prevent most of SQL injection and XSS attacks.

  • SQL injection: treated via PDO prepared statements;
  • XSS: All user's inputs are parsed in every documents through htmlentities + ENT_QUOTES + 'UTF-8';

Here is an example of my code:

<?php
    $db_hostname='localhost';
    $db_username='user';
    $db_password='pass';
    $db_dbname='db_name';
    $db_conn_str="mysql:host=" . $db_hostname . ";dbname=" . $db_dbname;

    try {
        $db = new PDO($db_conn_str, $db_username, $db_password);
        $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

        $stmt = $db->prepare("SELECT * FROM users_db WHERE uname = ?");

        $stmt->bindValue(1, $_SESSION['Kuser']);
        $stmt->execute();

        $result = $stmt->fetchAll();

        if (count($result) > 0) {
            $fnameR = htmlentities($result[0]['fname'], ENT_QUOTES, 'UTF-8');
            $lnameR = htmlentities($result[0]['lname'], ENT_QUOTES, 'UTF-8');
            $addressR = htmlentities($result[0]['address'], ENT_QUOTES, 'UTF-8');
            $emailR = htmlentities($result[0]['email'], ENT_QUOTES, 'UTF-8');
            $db=null;
        }
        else {
            $db=null;
            header('Location: access.php');
        }
    }
    catch (PDOException $e) {
        $db=null;
        header('Location: error.php');
    }
?>

This code snippet retrieves if an user exist in the database and if so, fetches some data from the table that get parsed in the html document.

Please also be aware of the following:

  • Absolutely no input in the application from user A is visible to user B;
  • The only user inputs visible in any html documents of the application is their own information stored in the database like in the above code block where an user can modify their personal information through a standard html form;
  • The only file in the application that parses input from other users is accessed by me, and the information is treated via htmlentities as well. It is a page that generates a html table with the list of usernames like so:
while($row = mysqli_fetch_assoc($resultSHOW)) {
    echo "<tr><td bgcolor='#FFF'>" . htmlentities($row['uname'], ENT_QUOTES, 'UTF-8'). "</td></tr>";
}
  • Any interaction between the user and the MySQL database are operated with MySQL credentials that has only SELECT UPDATE and INSERT privileges. All other SQL commands are disabled for them.
  • All of PHP and MySQL errors messages are customized. The user is in fact redirected to a generic error page when an error occurs.

So the question remains, is all of the above enough to prevent most of SQL injection and XSS attacks? I understand that it's really hard to prevent every security threats, I am simply looking in preventing most of the case here.

Also, concerning session hijacking, my application works in a way that when an user log in, a custom session variable is created and saved. Then, every page this user visit checks if this session variable exists and allow the user to access the account section of the application like so:

<?php 
if (!isset($_SESSION['user'])) {    
    header('Location: login.php');
}
?>

Where $_SESSION['user'] is the user's username.

It is the only tool my application uses to check if an user has actually log in! If this variable is not set, the user is redirected to the login.php page, otherwise, they can navigate through the account section of the application.

Reading many documents available online, I understand that this can be really vulnerable to session hijacking. What would be the best approach regarding that? I want to keep it as simple as possible, I am not interested in using cookies as they are vulnerable to cookie thefts. But if you think it is better so, I will kindly listen to your arguments.

  • You said you aren't using cookies, but PHP automatically uses a cookie to identify the user between requests when using sessions. That's where your SESSION variable comes from. – multithr3at3d May 13 at 12:41
  • Ok! Thanks for the info! I wasn't aware of this! – DjShoucrout May 13 at 15:11
2

I'm sorry to inform you that while you're using parameterized queries (which is great) your authentication checks are severely flawed. Sending the redirect header does not halt the execution, so any code that occurs after the redirect will still execute. You can validate this yourself by using a web agent that can ignore redirects such as curl.

| improve this answer | |
  • Hi and thanks for the reply! Since my original post I have been throwing some die() all over the place after the redirects. Do you think this is enough? Thanks :) – DjShoucrout Jun 15 at 2:12
  • Die will usually spam your logs, but anything that halts execution is fine. – wireghoul Jun 15 at 8:16
  • You can use exit() instead of die(). – ThoriumBR Jul 14 at 1:56
  • die() is the same as exit() and you should use one of them after redirect(). exit or die will not spam your server logs - unfortunately @wireghoul is wrong here. ( stackoverflow.com/questions/1795025/… ) – Awaaaaarghhh Jul 14 at 3:09
  • Additionally you could get some redirection bugs if someone enters URL like host/index.php/index.php instead of just host/index.php. – Awaaaaarghhh Jul 14 at 3: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.