SitePoint Sponsor

User Tag List

Results 1 to 24 of 24
  1. #1
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    How secure is this code??

    Its getting a id from a previous page link. the link looks like this www.mysite.com/article.php?id=1

    I'm trying to protect it from sql injection, Xss and other types of hacking.

    PHP Code:
    <?php

    $id 
    mysql_real_escape_string($_GET['id']);
    $id str_replace('http://','/',$id); 
    $result mysql_query("SELECT id, headline, article, picture_url, date FROM article WHERE id=$id");
        if (!
    is_numeric($id)) {
              exit (
    "Cant Find Page"); 
    }
        if (!
    mysql_num_rows($result)) {     
            exit (
    "Article Does Not Exist");

        if (!
    $result) {
               exit (
    '<p>Could Not Find Article</p>');
    }
        else {    
    $row mysql_fetch_assoc($result);
    $article $row['article'];
    $title $row['headline'];
    $date $row['date'];
    $picture $row['picture_url'];
    }
    ?>

  2. #2
    play of mind Ernie1's Avatar
    Join Date
    Sep 2005
    Posts
    1,252
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    You should use ctype_digit and check before the query.
    my mobile portal
    ghiris.ro

  3. #3
    SitePoint Enthusiast snajt's Avatar
    Join Date
    May 2008
    Location
    Kalmar, Sweden
    Posts
    45
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Ernie1 View Post
    You should use ctype_digit and check before the query.
    You're right, but be aware that ctype_digit takes a string as argument.

    So
    PHP Code:
    var_dump(ctype_digit(10)) == FALSE 
    because 10 is a integer.

    You can also use this regexp to filter non-digits:

    PHP Code:
    preg_replace("/[^\d]/"""$str

  4. #4
    SitePoint Enthusiast
    Join Date
    Jul 2006
    Posts
    90
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I will doit this way:

    PHP Code:
    <?php

    $id 
    = (int) $_GET['id'];
    $rs mysql_query("SELECT id, headline, article, picture_url, date FROM article WHERE id=" $id " LIMIT 1");
    if(
    mysql_num_rows($rs) == 1){
        
    $row         mysql_fetch_assoc($rs);
        
    $article    $row['article']; 
        
    $title         $row['headline']; 
        
    $date         $row['date']; 
        
    $picture     $row['picture_url']; 
    }else{
        echo
    '<p>Could Not Find Article</p>'
    }

    ?>
    In those cases I know that the variabel/input is (should be) a int i just use the this method as I show you on my code.

  5. #5
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    oxodesign is correct, but the only thing to remember that the value of id in the case above becomes 0 (zero).

    So you could test for $id > 0 before taking the trouble to query your database for a zero value (unless that is a valid value for you).
    PHP Code:
    $_GET['id'] = "abc123";
    $id = (int) $_GET['id'];
    var_dump$id );  // gives int 0

    $_GET['id'] = "123abc";
    $id = (int) $_GET['id'];
    var_dump$id );  // gives int 123 

  6. #6
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Could you explain what the (int) does??

    so if I have

    PHP Code:
    $id = (int) mysql_real_escape_string($_GET['id']); 
    would that work the same? and since I have that, does that mean I could get rid of the this part of code
    PHP Code:
    if (!is_numeric($id)) {
              exit (
    "Cant Find Page"); 


  7. #7
    Coding and Breathing CoderMaya's Avatar
    Join Date
    Feb 2008
    Location
    Atlit, Israel
    Posts
    470
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Just for the record, it's considered best to end your SQL queries with a semi-colon. Keep that in mind whenever you're writing one.
    Learn about the new Retro Framework
    Code PHP the way it was meant to be coded!

  8. #8
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    did you see one missing anywhere?

  9. #9
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Big problem with your origional code is that you modify the string after escaping it. Escaping is always the last step, as once you escape you cannot alter or you risk breaking the escaping.

    is_numeric() allows decimals, +- signs, hex, leading zeros, and more. see the documentation.

    ctype_digit() is a good choice, as well as casting the value to integer.

  10. #10
    Coding and Breathing CoderMaya's Avatar
    Join Date
    Feb 2008
    Location
    Atlit, Israel
    Posts
    470
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Chameleon View Post
    did you see one missing anywhere?
    At the end of both your SELECTs
    Learn about the new Retro Framework
    Code PHP the way it was meant to be coded!

  11. #11
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    To recap what is being said:
    PHP Code:
    $id = (int) $_GET['id'] ; // its gets cast into a number, so its 0 or greater

    if( $id ){
    $sql ="SELECT id, headline, article, picture_url, date 
          FROM article 
          WHERE id= 
    $id 
          LIMIT 1 ;"
    ;

    mysql_query$sql );  // do your query
    }else{
    // handle failure here


    Use ctype_digit if you prefer
    Last edited by Cups; Sep 4, 2008 at 11:51. Reason: formatted the sql too

  12. #12
    Grumpy Minimalist
    Join Date
    Jul 2006
    Location
    Ontario, Canada
    Posts
    424
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I think that a source of your confusion may be that this particular example uses an integer as input, whereas you've probably been told to use mysql_real_escape_string() at all times.

    The short answer: use the code provided by Cups.

    The long answer: read on to prevent future problems you may have with validation.

    Firstly, you need to understand the difference between SQL injections and XSS:
    SQL injections are a problem when inputting/outputting to/from a database.
    XSS is a problem when outputting to a web page.

    In both cases, you need to accomplish two things to be secure, in this order:
    1. Validation - ensure that the data is what you expect. If it isn't, either force it to be something that you expect, or error out.
    2. Escaping - use escaping to separate data (the substance) from meta-data (stuff that describes the substance).

    A quick note about the wording of number 1 above: this sentence is NOT the same as "ensure that the data isn't what you don't expect". It's always best to "whitelist", where you test for known good data and deny everything else, instead of "blacklist", where you test for known bad data and allow everything else.

    -----

    Let's apply this thinking to your particular example:

    You are taking in an ID from the querystring, and plan to use it in an SQL query. Because this ID is from the user, it's untrusted, therefore you must protect against SQL injection.

    The first step is to validate using what you expect. Well, since your IDs are numeric, you would expect the input to be numeric. So, you need some kind of PHP method to make sure that the value is numeric. Some of these methods have already been mentioned by others: type casting (the "(int)" syntax), ctype_digit, and intval are examples of functions that will help you here. All of these will help you ensure that your user input is an integer (or help you to convert it to one).

    If you've done your validation step properly, your data should now be secure. The second step is only to prevent accidents, not attacks (although many do rely on them to protect against attacks).

    The second step is escaping. In your case, you've built up an SQL query and would like to insert your integer into it. A PHP integer, by definition, only contains the values -, or 0-9. None of these characters can be interpreted by SQL as meta-data (things like SELECT, INSERT, FROM are meta-data in SQL, things like 42, 'Stuff', `table` are data). Therefore, you don't need to escape it. It's that simple.

    So, the code that Cups summarized does all of this perfectly and is precisely what you want.

    -----

    But what if you're performing SQL with string values instead of integer values? In this case it gets a little more tricky, but the general process (validation followed by escaping) is the same. In this case, validation is dependent upon your application. You will validate a username in a much different way from the way that you validate a product key, for example. Since there's such a variation in the way that strings need to be validated, there's an entire language (of sorts) that is used for this - regular expressions. Depending upon your needs, you may validate strings by using loops in your code, or you might be using preg_match. Either way, the PHP manual is your bible in these situations when you need to find a function just right for your validation needs.

    When it comes to escaping string data, this is where you'll be using mysql_real_escape_string since you're putting it into a MySQL query, and you need to prevent your data from being interpreted as meta-data.

    -----

    Another quick example: what about if you want to output a string to an HTML page?

    In this case, once again, you'll be performing validation based on the type of string you're using. This will probably mean using regex, like the previous example.

    As far as escaping goes, you'll be using htmlentities with the third parameter (charset) specified.

    -----

    In summary:

    Validation is dependent upon the type of data that you are receiving, and is used to prevent errors and provide security.

    Escaping is dependent upon the destination of the data and is used to prevent the data from being interpreted as meta-data, thus preventing errors.

    The purpose of this post was to provide you with a framework to think about these types of security issues (namely, validate then escape), and provide some examples to demonstrate that this framework applies to most issues you'll be facing. Hopefully it's helped you to understand why this code:

    Code PHP:
    $id = (int) mysql_real_escape_string($_GET['id']);

    ...is incorrect.

  13. #13
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    is this any better? should I also filter anything.

    I'm guessing since I'm not allowing the user to display anything on the page from there input that I wont have to worry about XSS?
    This is just a get the infomation from the database from the link on the main page. So I should worry about sql injection?

    Thank you all for all your help!

    PHP Code:
    <?php
    $id 
    = (int) $_GET['id'];
    $message 'Can\'t Find Article';
    $result mysql_query("SELECT id, title, full_article, full_picture_url, date FROM articles WHERE id=$id");
    mysql_real_escape_string($id);
        if (
    intval($id) && mysql_num_rows($result) && ($id 0)) {
    $row mysql_fetch_assoc($result);
    $article $row['full_article'];
    $title $row['title'];
    $date $row['date'];
    $picture $row['full_picture_url'];
    } else {
              exit (
    "$message"); }

    ?>

  14. #14
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by CoderMaya View Post
    Just for the record, it's considered best to end your SQL queries with a semi-colon. Keep that in mind whenever you're writing one.
    From PHP.net manual
    "A SQL query

    The query string should not end with a semicolon."

  15. #15
    SitePoint Enthusiast WMX's Avatar
    Join Date
    Sep 2008
    Posts
    94
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Chameleon View Post
    is this any better? should I also filter anything.

    I'm guessing since I'm not allowing the user to display anything on the page from there input that I wont have to worry about XSS?
    This is just a get the infomation from the database from the link on the main page. So I should worry about sql injection?

    Thank you all for all your help!
    Cleaning up and organizing your code a bit...

    PHP Code:

    <?php

    $id 
    = (int) $_GET['id'];
    $errormessage "Can't Find Article";
    $articledata false;

    // try to get an article
    if ($id 0)
    {
        
    $result mysql_query("
            SELECT id, title, full_article, full_picture_url, date 
            FROM articles 
            WHERE id=
    $id
        "
    );
        if (
    $result)
        {
            
    $articledata mysql_fetch_assoc($result);

            
    // really no need to do this, you should just 
            // display it from the $articledata array.
            
    $article $articledata['full_article'];
            
    $title $articledata['title'];
            
    $date $articledata['date'];
            
    $picture $articledata['full_picture_url'];
        }
    }

    if (
    $articledata)
    {
        
    // display it
        
    ?>
        <h1><?php echo $articledata['title']; ?></h1>
        <div>Posted on: <?php echo $articledata['date']; ?></div>
        <div>
        <img src="<?php echo $articledata['full_picture_url']; ?>
            style="float:right" alt="" />
        <?php echo $articledata['full_article']; ?>
        </div>
        <?php
    }
    else
    {
        echo 
    $errormessage;
    }

    ?>
    ~

  16. #16
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    What about escaping the data from the database before displaying it?

  17. #17
    SitePoint Enthusiast WMX's Avatar
    Join Date
    Sep 2008
    Posts
    94
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Cups View Post
    What about escaping the data from the database before displaying it?
    It would really depend on what data you're storing and if it's already escaped. For example if an article can contain html (that you've previously sanitized), you wouldn't want to escape that.
    ~

  18. #18
    Coding and Breathing CoderMaya's Avatar
    Join Date
    Feb 2008
    Location
    Atlit, Israel
    Posts
    470
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by Chameleon View Post
    From PHP.net manual
    "A SQL query

    The query string should not end with a semicolon."
    Weird. I always close them with a semi-colon and it has proven to increase security.
    Learn about the new Retro Framework
    Code PHP the way it was meant to be coded!

  19. #19
    SitePoint Enthusiast WMX's Avatar
    Join Date
    Sep 2008
    Posts
    94
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I don't see how it could "increase security". Most SQL injection involves terminating the current query and adding a second malicious query, then commenting the remaining SQL. But since php's mysql_query function can only execute *one* query, all that is irrelevant, as is a trailing semicolon.
    ~

  20. #20
    SitePoint Wizard silver trophybronze trophy Cups's Avatar
    Join Date
    Oct 2006
    Location
    France, deep rural.
    Posts
    6,869
    Mentioned
    17 Post(s)
    Tagged
    1 Thread(s)
    Quote Originally Posted by WMX View Post
    It would really depend on what data you're storing and if it's already escaped. For example if an article can contain html (that you've previously sanitized), you wouldn't want to escape that.
    Given the OPs question I thought that I raised a valid point, seeing as Tarh went to the trouble of spelling out when to validate and when to escape, then the example fails to show an example of it.

    Do you really think our OP will have filtered the input? In all fairness its taken some time for him/her to understand what type-casting is, and its use in this type of operation.

  21. #21
    SitePoint Enthusiast WMX's Avatar
    Join Date
    Sep 2008
    Posts
    94
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    You make a good point Cups. One thing I'd like to know is where are the articles coming from. Is the OP writing them himself or are the user submitted?

    If there not supposed to be any HTML in the content, then the easiest thing to do is use htmlspecialchars on anything before outputting it.
    ~

  22. #22
    SitePoint Zealot Chameleon's Avatar
    Join Date
    Oct 2004
    Location
    Arizona
    Posts
    140
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    I would be writting the articles and they do contain some html code in them when they are stored into the database. currently I input them using phpmyadmin until I come up with a secure login script to insert the articles.

    Is that newier code better and more secure?

  23. #23
    SitePoint Enthusiast WMX's Avatar
    Join Date
    Sep 2008
    Posts
    94
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    The code in post #15 should be fine for you. Just keep in mind that it needs to be totally rethought if you ever want to allow others to post content.
    ~

  24. #24
    Theoretical Physics Student bronze trophy Jake Arkinstall's Avatar
    Join Date
    May 2006
    Location
    Lancaster University, UK
    Posts
    7,062
    Mentioned
    2 Post(s)
    Tagged
    0 Thread(s)
    The reason a semi colon isn't needed is that mysql_query() can only execute 1 single query (not including subqueries).

    Semi-colons are used to separate commands - but when there's only one it wouldn't make a difference.

    But I can't see any reason why you shouldn't use it - I just don't because there's technically no reason - especially when using objects like PDO.
    Jake Arkinstall
    "Sometimes you don't need to reinvent the wheel;
    Sometimes its enough to make that wheel more rounded"-Molona


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •