2

Scenario: My PHP script requires 10 POST strings to work. The value of all of them needs to be escaped with htmlspecialchars(). So the first lines of the script look like this:

$var1 = htmlspecialchars($_POST['var1']);
$var2 = htmlspecialchars($_POST['var2']);
// And more. You get the point.

This is some code that could simplify it:

foreach($_POST as $key => $value){
    $$key = htmlspecialchars($_POST[$value]);
}

I'm unsure about the $$ with user input. I guess somebody could send many POST requests I don't need and block the server with that. Is this realistic?

The foreach code would be at the very top of my script. So it won't be able to overwrite any other variables.

Improve this question
6
  • 2
    Turn var1 into var[1], e.g. <input name="var[1]" /> - then you can read them via an array loop rather than an "endless" loop. Commented Jan 3, 2012 at 21:50
  • You don't need the foreach. Just use the extract function instead. Commented Jan 3, 2012 at 21:51
  • @MattH Ehm, those were obviously just example names. I don't call any variables var1 or var2 but rather descriptive. Commented Jan 3, 2012 at 21:52
  • @AurelioDeRosa: That's still a bad idea (as that pretty much emulates register_globals). What if I send $_POST['authorized'] or something. Commented Jan 3, 2012 at 21:53
  • 2
    Running everything through htmlspecialchars() assumes you're targetting an HTML environment for output. What if you decide you need SQL? PDF? Javascript? Now you have to UNDO all that work. Commented Jan 3, 2012 at 21:56

3 Answers 3

Reset to default
5

Rather than just blindly handling everything in $_POST (although just passing them through htmlspecialchars() is pretty harmless), you can use a whitelist of keys that are acceptable:

// An array of $_POST keys that are acceptable
$whitelist = array('var1','var2','var3');

foreach($_POST as $key => $value) {
   // Only handle $_POST keys you expect to receive...
   if (in_array($key, $whitelist)) {
      $$key = htmlspecialchars($_POST[$value]);
    }
}

This evades the possibility of a malicious user submitting hundreds of values to POST and consuming extra system resources.

Update

Commenters are correct. It is better to iterate through the whitelist than $_POST:

// Iterate over $whitelist and check for corresponding keys in $_POST
$missing_keys = array();
foreach($whitelist as $key) {
   if (isset($_POST[$key])) {
     $$key = htmlspecialchars($_POST[$key]);
   }
   else $missing_keys[] = $key;
}
echo "Missing keys: " . implode(",", $missing_keys);
Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Or better yet, just foreach over the whitelist and use the value as the key into $_POST. If someone generates an abnormally large $_POST input array then foreaching the whitelist will mean you don't spend an undue amount of time iterating over the bogus fields.
Good point. But wouldn't it be a better idea to loop through $whitelist than through $_POST? // @GordonM was faster than me.
@GordonM Yes it is better that way. Added above.
Don't forget to verify you have all 10 inputs. Count, or flag error if key not in POST. Or do else $$key= a safe default.
@MouseFood Seriously, my question was about the security issue so I minimized the code. I know that I have to validate them.
1

There is no worry that they will send lots of _POST variables and "block up the server"... This is capped by the post_max_size setting your ini file, and besides, the _POST variables are already loaded into memory, so at worst you'll just be (roughly) doubling the memory usage of the script. If you're absolutely sure that you won't overwrite existing variables because of code placement AND if you're sure that you're never going to run any code that depends on an isset() trigger, then you should be pretty safe. That said, white-listing your keys is always a good idea.

Improve this answer

2 Comments

I wouldn't be so sure about not blocking up the server - events.ccc.de/congress/2011/Fahrplan/events/4680.en.html
The hack you point to is very interesting, but it's based on hash collisions when the _POST array is being created (when elements are being added to the array), and generating variable variables doesn't affect this. Regardless of what dotweb does he is vulnerable to this exploit (unless he sets max_input_vars, a new ini setting to prevent this), as it is just a result of the way php handles array creation. Dotweb's variable variable loop doesn't expose him to much risk of blocking up the server... for reference, creating variable variables for a 10000 element array took .004 seconds.
0

An improvement on the accepted answer by Michael Berkowski:

Use in_array() instead, with a whitelist array.

function sanitize($data) {

  // fields to sanitize
  $whitelist = array('title', 'category', 'tags', 'hovertext', 'content');

  // loop over our data array
  foreach ($data as $key => $val) { // loop over our data array

    // if the current key is in our whitelist...
    if (in_array($key, $whitelist)) {

      // sanitize the value
      $cleanVal = htmlspecialchars($val);

      // replace the value in our data array
      $data[$key] = $cleanVal;

    }
  }

  // return our clean data array
  return $data;
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.