Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 10 of 10
  1. #1
    Regular Coder
    Join Date
    Aug 2002
    Posts
    151
    Thanks
    0
    Thanked 0 Times in 0 Posts

    foreach through POST data - is this secure?

    Very often, I'm working with forms that have alot of fields. In order to access them/make them into PHP variables, I could do this:

    PHP Code:
    $name $_POST['name']; 
    This is fine for a few but soon becomes messy and bloated when you get a lot of form variables. So I've been doing this:

    PHP Code:
    # get form input
    foreach($_POST as $key=>$value){
        $
    $key TRIM($value);

    However, I suspect there may be security issues with this (if someone manages to POST another wierd variable in, it will be processed also).

    What you do you think? is there a better way to do it?

  • #2
    Regular Coder ralph l mayo's Avatar
    Join Date
    Nov 2005
    Posts
    951
    Thanks
    1
    Thanked 31 Times in 29 Posts
    it's not secure, here's a better way:
    PHP Code:
    $_POST array_map('trim'$_POST); 
    The security issue arises if you have something like
    PHP Code:
    if ($admin)
    {
        
    // protected functionality

    The method you posted allows the user to set $admin to true by submitting a post with admin=1

  • #3
    Regular Coder
    Join Date
    Jun 2005
    Posts
    804
    Thanks
    0
    Thanked 0 Times in 0 Posts
    Well, even if you create an unintended variable, if you never use it for anything, it's not a security risk. This doesn't mean you shouldn't clean and validate everything you're pulling with something more than a TRIM().

    I wouldn't say the "$name = $_POST['foo']" method is "messy and bloated". If anything, it's more efficient than a foreach/TRIM() construct, and it's also a very clear, easy to refer to list of the variables you'll be working with, which can be invaluable to someone else who has to work on your code (or to yourself when you have to revisit the script after a few months). Less typing is not necessarily better.

  • #4
    raf
    raf is offline
    Master Coder
    Join Date
    Jul 2002
    Posts
    6,589
    Thanks
    0
    Thanked 0 Times in 0 Posts
    Quote Originally Posted by Kid Charming
    Less typing is not necessarily better.
    Quote of the day ...

    You always need to be extremely carefull with input from the client, so don't go automatically turn all the variables from the post-collecction into internal variables. If you wanna do that, thes just set register_globals = on ...

    Nothing wrong with it if you are an extremely good coder that has all his other bases covered, but maybe some wannabe like me later needs to work on your super-code and creates security-risks...
    Posting guidelines I use to see if I will spend time to answer your question : http://www.catb.org/~esr/faqs/smart-questions.html

  • #5
    Regular Coder Element's Avatar
    Join Date
    Jul 2004
    Location
    Lynnwood, Washington, US
    Posts
    855
    Thanks
    2
    Thanked 2 Times in 2 Posts
    Quote Originally Posted by ralph l mayo
    it's not secure, here's a better way:
    PHP Code:
    $_POST array_map('trim'$_POST); 
    The security issue arises if you have something like
    PHP Code:
    if ($admin)
    {
        
    // protected functionality

    The method you posted allows the user to set $admin to true by submitting a post with admin=1
    Now who in their right mind would determine the admin based on form inputs? You should always go from a ID or username, and some sort of validation and do the work in PHP to see if they are an admin. the foreach() method is just fine, depending on what you do with it. For example:

    PHP Code:
    <?php

    mysql_connect
    ('localhost''root''') or die(mysql_error());
    mysql_select_db('chinese_pizza') or die(mysql_error());

    foreach(
    $_POST as $key => $value) {
      ${
    'post_' $key} = mysql_real_escape_string($value);
    }

    $query sprintf("INSERT INTO `orders` VALUES(null, '%s', '%s')"$post_name$post_order);
    $result mysql_result($query);

    if(
    $result) {
      echo 
    'Success ' $post_order ' has been added to our delivery list!';
    }

    ?>

  • #6
    raf
    raf is offline
    Master Coder
    Join Date
    Jul 2002
    Posts
    6,589
    Thanks
    0
    Thanked 0 Times in 0 Posts
    Quote Originally Posted by Element
    Now who in their right mind would determine the admin based on form inputs? You should always go from a ID or username, and some sort of validation and do the work in PHP to see if they are an admin.
    i think you're missing his point.

    Imagine you determined that he is an admin (by doing a select or some sessionvariable or IPchecking or whatever), and you store this like
    $admin=True;
    and then go on in your script to process the form and do some actions based on his user-profile.
    If i changed your form and added a formfield with name 'admin' and value '1' then i the script would treat me as if i am a validated admin.
    Posting guidelines I use to see if I will spend time to answer your question : http://www.catb.org/~esr/faqs/smart-questions.html

  • #7
    Master Coder felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, Australia
    Posts
    6,627
    Thanks
    0
    Thanked 648 Times in 638 Posts
    You could use the following code where all of the expected post variables are listed in the first line so no others get copied.

    PHP Code:
    $allowed "parmone,parmtwo";
    while (list (
    $key$val) = each ($_POST))
    {
      if (
    strstr($allowed,$key) && $val)
        $
    $key $val;

    Stephen
    Learn Modern JavaScript - http://javascriptexample.net/
    Helping others to solve their computer problem at http://www.felgall.com/

    Don't forget to start your JavaScript code with "use strict"; which makes it easier to find errors in your code.

  • #8
    Regular Coder Element's Avatar
    Join Date
    Jul 2004
    Location
    Lynnwood, Washington, US
    Posts
    855
    Thanks
    2
    Thanked 2 Times in 2 Posts
    Quote Originally Posted by felgall
    You could use the following code where all of the expected post variables are listed in the first line so no others get copied.

    PHP Code:
    $allowed "parmone,parmtwo";
    while (list (
    $key$val) = each ($_POST))
    {
      if (
    strstr($allowed,$key) && $val)
        $
    $key $val;

    Yeah, I used this method with my old member's script. I had the set form names in the config, so it is not only easy to alter for whever the form names may be used but also allows a client to furtherr customize it with ease.

    And as far as not getting it, I think I do because you also should define $admin after you process post data to be used where ever in the file. At least this is how I code, but I know it would be easy for someone knew to do, which is why you should know how to handlee them. Thats why I also add $'post_'name before my variables to seperate them from the set pre-set variables an script variables.

  • #9
    Senior Coder
    Join Date
    Sep 2005
    Posts
    1,791
    Thanks
    5
    Thanked 36 Times in 35 Posts
    using in_array would seem a more logical way of doing that to me, do you use strstr for a particular reason?

  • #10
    Regular Coder Element's Avatar
    Join Date
    Jul 2004
    Location
    Lynnwood, Washington, US
    Posts
    855
    Thanks
    2
    Thanked 2 Times in 2 Posts
    Yeah thats what I would use, cause its much simpler to contruct an array for the names, like how I did it, in a configuration file.

    Something like:

    Config file
    PHP Code:
    <?php

    // Mumbo Jumbo here...

    $config['formnames'] = array('username''password''menu_tree',  'submit''friends_email''senders_email');

    ?>
    example
    PHP Code:
    <?php

    require_once('/home/root/public_html/includes/config.ini');

    foreach(
    $_POST as $key => $value) {
      if(
    in_array($key,  $config['formnames']) {
        ${
    'p_' $key} = trim(strip_functions(strip_tags($value)));
      }
    }

    ?>
    Edit: nevermind strip_functoins() its a custom function.
    Last edited by Element; 02-10-2006 at 01:30 AM.


  •  

    Posting Permissions

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