9 replies to this topic

[florian]

something I saw in authenticate_visitor and RequestNewPassword and maybe it's used like this in other snippets too...

  // extract the variable array into plain variables
  extract($_POST);

  // code added to insure against form injection exploits
  $username = preg_replace("/[^\w\.@-]/", "", $username);
  $password = preg_replace("/[^\w\.@-]/", "", $password);
  $captcha = preg_replace("/[^\w\.@-]/", "", $captcha);

The problem is, that by tampering with the posted data you could easily manipulate any variable that has been defined before, like $use_captcha, etc. So the captcha is useless as any hacker would pay attention to submit use_captcha=0...

the solution is to advise a prefix for the extracted variables:

  // extract the variable array into plain variables
  extract($_POST, EXTR_PREFIX_ALL, "auth");

  // code added to insure against form injection exploits
  $username = preg_replace("/[^\w\.@-]/", "", $auth_username);
  $password = preg_replace("/[^\w\.@-]/", "", $auth_password);
  $captcha = preg_replace("/[^\w\.@-]/", "", $auth_captcha);

and any variable not beginning with $auth_ cannot be tampered with anymore...

What do you think - am I becoming paranoid? :-)

[Ralph]

Yes, you are being paranoid... I've read many reviews of how people think that extract() is a dangerous function, but I have also read reviews on how it is not all that bad... As I have warned before in these forums, it is up to the individual developer to assure the security of any and all code that he writes and/or uses...

You will never see me extracting every POST or GET variable individually, using $var = $_POST['var'], in any code that I write... Many of the forms I write have at least 20 - 30 data inputs which makes such a practice an act of futility... I generally use extract() or the built in API function, getFormVars(), and then I sanitize every variable that might contain errant code... Another method that I use, especially if the form is open to public submissions, is to use a loop that extracts and sanitizes every POST variable... I've gotten into the habit of sanitizing everything, just to be on the safe side... And as far as input validations go, for some work I only use client-side validations with Tigra Form Validator while for other work I do both client-side and server-side validations... It all depends on the overall need...

And in answer to the second part, getFormVars() allows the use of both prefix and suffix for field name group recognition... I also use $prefix with straight extract() at times, as well...

[florian]

Maybe your PHP-version is customized to use EXTR_SKIP as the extract-collision-rule (which would be even easier as my above workaround). Then there is no problem at all. But the PHP-standard is EXTR_OVERWRITE.

I didn't want you to "extract" alls Variables manually. That's why I presented you with my solution.

BUT: if I submit a form and add an additional variable "etomite"=0 your extract() will overwrite your cms-instance, just because it carries the same name and oops - you get a "function not found" error from php. Not nice.

And the big difference between using getFormVars with "prefix" and using extract with "prefix" is, that the latter adds the prefix to the names of the variables, as to avoid overwriting any variable defined before.

While with $_POST containing "auth_etomite"=0 and using extractVariables() with prefix "auth", the instance of etomite is still overwritten...

Still being paranoid... :-)

[Ralph]

Umm... Yeah, maybe... Remember, Etomite snippets run in a local scope, kinda like a function, so variables extracted in them have no effect on previously defined variables... And you would never name form inputs the same as global or system vairables anyway... You don't need to worry about over-writing $etomite->dbConfig['dbase'], for example... Think of an Etomite snippet like this...

function etomite_snippet_name($param1="one thing", $param2="another")
{
  ... $param1 and $param2 sent in the snippet call or via runSnippet() and become local variables...
  extract($_POST);
  ... $_POST['var1'] becomes the local variable $var1 ...
  ... $_POST['var2'] becomes the local variable $var2 ...
}

As you can see, all variables are local variables except superglobals and the $etomite class object... Even calling a snippet from within a snippet needs to have the variables explicitly sent in order to be available in the second snippet... Variables can be sent back in a return object just as easily as $output too... So if I send back an array using return $array; and I called the snippet using $data = $etomite->runSnippet(), $data would end up holding the contents of $array...

Ok, probably went in depth further than required, but the example above should help cure the paranoia...

[florian]

Ralph, on Feb 8 2008, 07:43 PM, said:

And you would never name form inputs the same as global or system vairables anyway... You don't need to worry about over-writing $etomite->dbConfig['dbase'], for example...

of course I wouldn't do that. But if you got a hackety hacking skript that would depend on avoiding the captcha, you could achieve that by guessing the variable which is responsible for switching the captcha on and off. (just use some popular browser plugin, add some variablenames to the POST-Array and you're done...)

Ralph, on Feb 8 2008, 07:43 PM, said:

Ok, probably went in depth further than required, but the example above should help cure the paranoia...

well dont worry about the paranoia anymore (too late anyway). But with some more sensitive data (more sensitive than a captcha???) this could easily end up badly. And this is why I vote for using the EXTR_SKIP (cf. here) with extract. And I think anyone should. And anyone who doesn't want to should consider twice, why not.

Just my two pennies - and thus I shall be quiet.

[Dean]

Thanks for your posts; it's certainly something to keep in mind when we are rewriting
Ta!

[Ralph]

In its present form, Etomite makes very little, if any, use of extract() other than in snippets... And most snippets that use extract() were probably authored by me... I am fully versed in the capabilities, functionality, and security concerns associated with the function, as stated earlier... I think the issue is in scope of use, which I explained above... Without seeing a real world example of how extract can cause unsecure code in the context of how it has been used I'd consider this a dead issue...

[PaulD]

Hi all,

I have been trying to use the preg_replace as follows for a public form

		// extract the variable array into plain variables
			extract($_POST, EXTR_PREFIX_ALL, "auth");

		// code added to insure against form injection exploits
			$title = preg_replace("/[^\w\.@-]/", "", $auth_title);
			$description = preg_replace("/[^\w\.@-]/", "", $auth_description);
			$parent = preg_replace("/[^\w\.@-]/", "", $auth_parent);

But the preg replace seems to be removing all my white spaces as well. So if a user inputs Hello World they get returned HelloWorld.

Now I must admit that I can't follow the code in preg_replace. Have tried removing the \w but that meant everything got removed (thinking it might be w for whitespace). PHP.net seems to imply that \s is for spaces.

Can anyone help - thank you. Paul

PS (I am sorry but this is not strictly an etomite specific question and I know this is not for learning php). Any help much appreciated though. I am stuck.

[Ralph]

@PaulD

You can just use either htmlspecialchars() or htmlentities() instead of preg_replace() to sanitize form inputs... I continuously Google to see what methods are best and I have been using htmlentities() as of late... Some might find it to be overkill, but I'd rather be safe than sorry... It boils down to whether you want to do input by input sanitizing or use one generic filter for all inputs... You can spend days researching the subject and come away almost as confused as you started... It seems that there is no "standard" - at least not in PHP...

[PaulD]

Thanks Ralph.

My above code now looks like this for anyone interested...

	// extract the variable array into plain variables
		extract($_POST, EXTR_PREFIX_ALL, "todo");

	// code added to insure against form injection exploits
		$title = htmlentities($todo_title);
		$description = htmlentities($todo_description);
		$parent = htmlentities($todo_parent); // This one probably not needed but you never know

I left the prefix in because it is only a 'bit' extra and it might help and mostly because I like using it! PHP is cool and so accessible. Debugging though is like stirring through spaghetti! (At least with my code).

Thanks again Ralph,

Paul.