#1
  1. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jun 2013
    Posts
    15
    Rep Power
    0

    Is this secure? Passing a URL parameter into a SQL query


    PHP Code:
    if (isset($_GET['id'])) {
        
    $id $_GET['id'];
    }
    else {
        
    $id null;
    }

    require(
    "common.php");

    $query "
        SELECT
            ID,
            Email,
            First_Name,
            Last_Name,
            Type,
            Region
        FROM users
        WHERE
            ID = :id
    "
    ;

    $query_params = array(
        
    ':id' => $id 
    );

    try {
        
    $stmt $db->prepare($query); 
        
    $result $stmt->execute($query_params);     
    }
    catch(
    PDOException $ex){
        die(
    "Failed to run query: " $ex->getMessage()); 
    }

    $rows $stmt->fetchAll(); 
  2. #2
  3. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Hi,

    passing values to the parameters of a prepared statement is secure. However, PDO has an evil gotcha: By default, it doesn't use prepared statements. It only makes it look like it and instead uses escaping behind the scenes (which has the same vulnerabilities like manual escaping).

    So your query is only secure if you've told PDO to use actual prepared statements:

    PHP Code:
    $db_options = array( 
        
    PDO::ATTR_EMULATE_PREPARES => false                     // important! use actual prepared statements (default: emulate prepared statements) 
        
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION           // throw exceptions on errors (default: stay silent) 
        
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC      // fetch associative arrays (default: mixed arrays) 
    ); 
    $database = new PDO('mysql:host=localhost;dbname=YOURDB;charset=utf8''YOURUSER''YOURPW'$db_options);    // important! specify the character encoding in the DSN string, don't use SET NAMES 
    Another huge security issue of your code is that you display the error message. This allows anybody to inspect the query and possibly gather further information about your server. Don't let that happen! Internal error messages are not meant to be seen by users. They're meant for you, the developer.

    Why do you catch the exception, anyway? Catching an exception just to display the error messages makes no sense, because that's already the default behaviour of exceptions.

    Just let the exception do its work. This allows you to redirect the error message to anywhere you want. During development, you probably want to see the error messages on the screen. So you turn display_errors on in the php.ini. On your live server, you do not want the messages to appear on the screen, so instead you activate log_errors.

    Don't catch exceptions unless you actually want to do something with it (like retrying the action or using an alternative route).
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  4. #3
  5. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jun 2013
    Posts
    15
    Rep Power
    0
    Hi jacques1,

    Thank you for the information on error messages, I have updated my code.

    Here is the relevent content of my common.php, where I believe I have enabled prepared statements:

    (Note that this code is based on one of the tutorials on this forum)

    PHP Code:
        $options = array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8'); 

        try 
        { 
            
    $db = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8"$username$password$options);
         } 
        catch(
    PDOException $ex
        { 
            die(
    "Failed to connect to the database"); 
        } 
         
        
    $db->setAttribute(PDO::ATTR_ERRMODEPDO::ERRMODE_EXCEPTION); 
         
        
    $db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODEPDO::FETCH_ASSOC); 
    Aside from the error messages, would my code be considered secure? Not at risk of SQl injections?
  6. #4
  7. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jun 2013
    Posts
    15
    Rep Power
    0
    Or this....


    PHP Code:
        $options = array(PDO::ATTR_EMULATE_PREPARES => false
         
    PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION 
         
    PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC); 

        try 
        { 
            
    $db = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8"$username$password$options);
         } 
        catch(
    PDOException $ex
        { 
            die(
    "Failed to connect to the database"); 
        } 
    (I'll consider removing the catch)
  8. #5
  9. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Originally Posted by DpchMd
    Here is the relevent content of my common.php, where I believe I have enabled prepared statements:
    No, you haven't. You didn't turn off PDO::ATTR_EMULATE_PREPARES.

    In fact, this code can easily be vulnerable to SQL injections, because it uses SET NAMES. This query changes the character encoding without notifying PHP of it, which can break all escaping functions (as explained in the link above). So in some cases, you may end up with no security at all -- even if you seemingly use prepared statements.

    Yep, the PHP language developers have screwed up again. You have to know that and be very careful with the code you copy and paste from somewhere.

    I think it would be best to completely replace the connection code.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  10. #6
  11. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jun 2013
    Posts
    15
    Rep Power
    0
    Originally Posted by Jacques1
    No, you haven't. You didn't turn off PDO::ATTR_EMULATE_PREPARES.

    In fact, this code can easily be vulnerable to SQL injections, because it uses SET NAMES. This query changes the character encoding without notifying PHP of it, which can break all escaping functions (as explained in the link above). So in some cases, you may end up with no security at all -- even if you seemingly use prepared statements.

    Yep, the PHP language developers have screwed up again. You have to know that and be very careful with the code you copy and paste from somewhere.

    I think it would be best to completely replace the connection code.
    Thanks again - have a missed anything in my latest post? (above yours)
  12. #7
  13. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Apart from the dangerous and useless try-catch: No.
    The 6 worst sins of security ē How to (properly) access a MySQL database with PHP

    Why canít I use certain words like "drop" as part of my Security Question answers?
    There are certain words used by hackers to try to gain access to systems and manipulate data; therefore, the following words are restricted: "select," "delete," "update," "insert," "drop" and "null".
  14. #8
  15. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Jun 2013
    Posts
    15
    Rep Power
    0
    Originally Posted by Jacques1
    Apart from the dangerous and useless try-catch: No.
    Thanks very much :-)

IMN logo majestic logo threadwatch logo seochat tools logo