#1
  1. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171

    The proper and secure way of keeping the form populated with GET or POSTed values.


    Hi;
    Is this good enough?

    PHP Code:
    if($_GET)
        {
            foreach(
    $_GET as $val =>$row)
                {
                    if(
    strlen($row)>0)
                        {
                            
    $_SESSION[$val]=html_escape($row);
                        }
                }
        } 
    PHP Code:
    function html_escape($raw_input)
        {
            return 
    htmlspecialchars($raw_inputENT_QUOTES ENT_HTML401'UTF-8');     
        } 
    PHP Code:
    echo '<input type="text" name="demo" value="' . ( isset( $_SESSION['demo'] ) ? $_SESSION['demo'] : '' ) . '" />'
    Last edited by zxcvbnm; June 24th, 2013 at 01:03 AM.
  2. #2
  3. No Profile Picture
    Contributing User
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Dec 2004
    Posts
    2,989
    Rep Power
    375
    to avoid xss attacks (i.e. prevent user from using JS then yeah)
  4. #3
  5. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Hi,

    don't use "blanket escaping".

    Different contexts require different escaping mechanism, so if you apply a particular function to all input, this will only work in some cases and fail in all others. For example, your HTML escaping doesn't make the input safe for JavaScript code or SQL queries. You'll end up with garbled input or even security holes -- unless you completely revert the previous escaping and then apply a different function, which makes the whole approach pretty pointless.

    The PHP developers already tried blanket escaping with the infamous "magic quotes", and it's been a disaster. It doesn't work at all, because 99% of the time, you're busy getting rid of it.

    It's much better do do the escaping ad hoc for each concrete context. In your case, simply apply the HTML escaping right before you insert the session value into the string.
    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".
  6. #4
  7. No Profile Picture
    Dazed&Confused
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2002
    Location
    Tempe, AZ
    Posts
    506
    Rep Power
    128
    Though maybe just preference, I'd never import all $_GET or $_POST variables directly into $_SESSION. It might not really hurt things if this is a simple script, but as a general practice I prefer to handle each expected variable explicitly and, if using $_SESSION to remember state, assign the variables to a contextual "namespace".

    For example:
    PHP Code:
    <?php
    // Start session
    session_start();

    // Establish namespace if not already done
    if ( !isset($_SESSION["someform"]) ){
        
    $_SESSION["someform"] = array();
    }

    // Ease-of-use reference
    $session =& $_SESSION["someform"];

    // Set $var, order of priority: new passed value, stored session value, default
    $var =  isset($_REQUEST["var"]) ? $_REQUEST["var"] :
            (isset(
    $session["var"]) ? $session["var"] : "");

    /* Validation & Business logic here, just using $var */

    // Once we've gone through everything that might alter $var's value,
    // update it into the session
    $session["var"] = $var;

    // Output to user
    print "$var";
    I'll be the first to admit that practice requires quite a bit of code but it's worked for me in the past.
  8. #5
  9. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,068
    Rep Power
    9398
    PHP Code:
    if($_GET)
        {
            foreach(
    $_GET as $val =>$row)
                {
                    if(
    strlen($row)>0)
                        {
                            
    $_SESSION[$val]=html_escape($row);
                        }
                }
        } 
    That is incredibly dangerous. Do you store anything in the session besides GET/POST data? Then I can overwrite it with something as simple as
    Code:
    /path/to/page.php?user=admin&username=admin&admin=1&isadmin=1&...
    The session should be 100% within your explicit control. Do not put arbitrary information in there without using some sort of sandbox, and ideally validating the information first.
    On the bright side a "sandbox" can be as simple as putting the data in a subarray,
    PHP Code:
    $SESSION["someform"][$val] = html_escape($row); 

    [edit] And you shouldn't be html_escape()ing stuff until you're literally putting it into the HTML. What if the data is good and you want to store it in the database? You have to un-HTML-escape it and possibly SQL-escape it. Leave the data as the pure data it is and escape it only at the moment you need to.
    Last edited by requinix; June 24th, 2013 at 03:17 PM.
  10. #6
  11. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171
    Originally Posted by requinix
    That is incredibly dangerous. Do you store anything in the session besides GET/POST data?
    Hi what about this?
    PHP Code:
    if($_GET)
        {
            
    $form_data = array('checkin'=>$_GET['checkin'], 'search_type'=>$_GET['search_type'], 
                               
    'search_id'=>$_GET['search_id'], 'hotel_id'=>$_GET['hotel_id'],'city'=>$_GET['city'], 
                               
    'pid'=>$_GET['pid'],'altFormat'=>$_GET['altFormat'], 'nights'=>$_GET['nights'],
                               
    'adults'=>$_GET['adults'], 'children'=>$_GET['children'],'room_type'=>$_GET['room_type']);
            foreach(
    $form_data as $val =>$row)
                {
                    if(
    strlen($row)>0)
                        {
                            
    $_SESSION['form_data'][$val]=html_escape($row);
                        }
                }
        } 
    I store login SESSIONS in something like $_SESSION['log']['logged'] or $_SESSION['log']['user_id'].
    Originally Posted by requinix
    And you shouldn't be html_escape()ing stuff until you're literally putting it into the HTML. What if the data is good and you want to store it in the database? You have to un-HTML-escape it and possibly SQL-escape it. Leave the data as the pure data it is and escape it only at the moment you need to.
    I don't use these sessions to write to the database. For database i just use $_POST and use bind with PDO. I am not sure why you said this. Am I missing something?
    Originally Posted by Jacques1
    Don't use "blanket escaping".
    Hi Jaques. I got that code from your signature. I just use it for displaying user input data (via POST or GET) on screen.
  12. #7
  13. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    The thing is this: When you're dealing with security, it's a good idea to think ahead and choose robust solutions.

    Your escaping does work -- for this particular use case at this particular point of time. But code usually doesn't stay constant throughout its whole lifetime. You extend it, you change it, you add features, you remove features. If your security depends on very special circumstances (which aren't even visible from the code), then that's the first thing that will break down during those changes.

    This is why we're saying that the "pre-escaping" is a bad idea: It's too fragile, it will fall apart at the first opportunity. So think ahead and choose a more robust solution. And that's actually very easy, simply move the escaping into the "view" (like we already said).

    Isn't security all about thinking ahead? I mean, you may never have actually experienced an SQL injection attempt on your server. Yet still you protect yourself against it, because you know that it can happen in the future.
    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. Did you steal it?
    Devshed Supreme Being (6500+ posts)

    Join Date
    Mar 2007
    Location
    Washington, USA
    Posts
    14,068
    Rep Power
    9398
    Originally Posted by zxcvbnm
    Hi what about this?
    Sure: you're picking and choosing what data goes in and you're putting it in a dedicated place in the session. One thing I would recommend is to not put the data in the same place - what if I want to use the things in two different tabs? One would overwrite the other.

    You can simplify your code with
    PHP Code:
    $form_data = array('checkin''search_type', ...);
    foreach(
    $form_data as $val)
        {
            if(isset(
    $_GET[$val]) && strlen($_GET[$val]) > 0)
                {
                    
    $_SESSION['form_data'][$val] = html_escape($_GET[$val]);
                }
        } 
  16. #9
  17. No Profile Picture
    Dazed&Confused
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2002
    Location
    Tempe, AZ
    Posts
    506
    Rep Power
    128
    Originally Posted by zxcvbnm
    Hi what about this?
    Slight tweak to simplify things and reduce Notices.

    PHP Code:
    if($_GET
        { 
            
    $form_data = array('checkin''search_type''search_id',
                               
    'hotel_id''city''pid''altFormat''nights'
                               
    'adults''children','room_type'); 
            foreach(
    $form_data as $key
                { 
                    if ( isset(
    $_GET[$key]) && strlen($_GET[$key]) > )
                        { 
                            
    $_SESSION['form_data'][$key]=html_escape($_GET[$key]); 
                        } 
                } 
        } 
    You could also switch $_GET to $_REQUEST, allowing your logic to accept the variables via a POST as well.

    Edit: Dang you, requinix.
  18. #10
  19. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    And now please remove the html_escape(). We've just tried to explain why this is a really bad idea.
    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".
  20. #11
  21. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,664
    Rep Power
    171
    Originally Posted by Jacques1
    And now please remove the html_escape(). We've just tried to explain why this is a really bad idea.
    Hi

    Please correct me if I'm wrong. I am gonna have to need to html_escape any user input data "before printing it".

    Please note: Not before writting it to the database. I write it to the database as $_POST or $_GET and bind with PDO. But I html_escape POST or GET before echoing.

    Am I wrong? I doubt it, but I could be.

    Thank you.
  22. #12
  23. No Profile Picture
    Dazed&Confused
    Devshed Novice (500 - 999 posts)

    Join Date
    Jun 2002
    Location
    Tempe, AZ
    Posts
    506
    Rep Power
    128
    Originally Posted by zxcvbnm
    Hi

    Please correct me if I'm wrong. I am gonna have to need to html_escape any user input data "before printing it".

    Please note: Not before writting it to the database. I write it to the database as $_POST or $_GET and bind with PDO. But I html_escape POST or GET before echoing.

    Am I wrong? I doubt it, but I could be.

    Thank you.
    You're not wrong but that's not a particularly clean approach, either. It means that you'll be dealing with the same input by multiple variables throughout your code, reducing readability. As a more general practice it's poor since not all variables your scripts will receive will be output in HTML.

    In this particular instance you also won't have the benefit of the isset() and strlen() calls when it comes time to bind $_POST to the statement, requiring you to either repeat those calls then, or have potential Notice-level errors.

    Readability and consistency are valuable things, and the latter helps the former.
  24. #13
  25. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,959
    Rep Power
    1014
    Originally Posted by zxcvbnm
    I am gonna have to need to html_escape any user input data "before printing it".
    Right.



    Originally Posted by zxcvbnm
    But I html_escape POST or GET before echoing.
    If that were the case, you'd be perfectly fine. But you escape the data before you store it in the session. In other words, your session is already filled with "&lt" and "&quot" long before you actually output the data.

    This is bad and potentially unsecure practice. First of all, there's absolutely no reason to apply HTML escaping before you need it. All that does is cripple the data and make it unusable for anything but HTML. Why would you do that?

    Secondly, when you insert the session data, you completely rely on it already being escaped. You pass it right through in the hopes that it has already been escaped. But what if it hasn't? What if your previous escaping failed due to some bug? Then you'll end up with a huge XSS vulnerability.

    So I repeat again: Escape data right before you use it.

    This is the most sensible and secure approach. You don't mangle your data, and you can be sure that you don't output any unescaped data.

    There's not much that could go wrong with echo html_escape($foo);. But there's a lot that can go wrong with outputting some session value that has (hopefully) been escaped somewhere in the past.

    Not sure why you insist on using this "pre-escaping" approach. It's simply not a good idea.
    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".

IMN logo majestic logo threadwatch logo seochat tools logo