18

I am not a PHP developer but I'm assessing the security of a PHP5 application.

The author relied on extract($_POST) and extract($_GET) in some places, outside of functions.

My suggestion is to call extract($_POST, EXTR_PREFIX_ALL, 'form') and change the code accordingly, but his stance is that any variable is being redefined inside subsequent includes anyway.

I can easily change the superglobals by providing, for instance, _ENV=something inside the post values, but superglobals are arrays and I'm turning them into strings, I'm not sure it can have evil effects.

I could have a look at the several isset() uses and go backwards from there.. but I imagine there are attacks of this kind that don't require knowledge or divination of the source.

Is there some interesting variable to be set/changed, maybe in the innards of PHP?

Thanks

Marco Mariani
  • 13,556
  • 6
  • 39
  • 55
  • 3
    The only thing that comes to mind is overwriting a superglobal this way (i.e. defining an array named `_SERVER` inside `$_POST` and extracting that into the global space). Is that possible? Will it overwrite `$_SERVER`? Probably not (I hope). I'll have to try out some time. – Pekka Oct 01 '10 at 09:12
  • 1
    Yes, it worked with _ENV at least. Providing EXTR_SKIP would fix that. – Marco Mariani Oct 01 '10 at 09:21
  • 1
    *(suggestion)* Using [Suhosin](http://www.hardened-php.net/suhosin/a_feature_list.html) prevents overwriting of Superglobals and helps mitigate possible attack vectors. – Gordon Oct 01 '10 at 11:35

3 Answers3

16

For assessing "might" try this:

File:htdocs/mix/extraction.php

<?php
extract($_GET);
var_dump($_SERVER);//after extract
?>

and call it like that:

http://localhost/mix/extraction.php?_SERVER=test

After the extract on my Xampp the output looks something like that:

string(4) "test"

If any one knows anything about your variable naming and you use extract on $_POST or $_GET globals, then you have a serious problem. With a bit of time and work it would be possible to find out some namings by try and error.

Without knowing your source an intruder could try to hijack any global variabl like $_SESSION (but here it will only take any effect if you do the session_start(); before the extract($_GET), $_COOKIE or $_SERVER and even set specific values for them like that:

//localhost/mix/extraction.php?_SERVER[HTTP_USER_AGENT]=Iphone

If you use extract like that:

extract($var,EXTR_SKIP);

extract($var,EXTR_PREFIX_SAME,'prefix');

extract($var,EXTR_PREFIX_ALL,'prefix');

then you will be perfectly safe.

ITroubs
  • 11,094
  • 4
  • 27
  • 25
  • At first I thought your answer was just restating what we already said. Then I tried setting specific array items like that, and Oh My God :) – Marco Mariani Oct 01 '10 at 10:17
9

A common name for the database connection is $db, but that would just blow up the system, you can overwrite the $_SESSION variable.

session_start();

$_SESSION['test'] ='test';
var_dump($_SESSION);
$vars = array("_SESSION" => 'awww');
extract($vars);
var_dump($_SESSION);

output

array(1) {
  ["test"]=>
  string(4) "test"
}
string(4) "awww"

Overwrite variables $idUser or other fun stuff, want to mess up the iterables? Pass array('i' => 5) to extract, there are all sorts of fun you can have depending on scope.

Edit:

I just thought of another, if the form is handeling file uploads, why not try and overwrite variables named $file, $fileName, $fileExtention and see if you can get it to read files outside your permission level.

Kristoffer Sall-Storgaard
  • 10,576
  • 5
  • 36
  • 46
  • Like the other superglobals, _SESSION is an array. I can change it to a string, but any use of _SESSION from that point would just cause an error. Now I'm looking at specific variables, thanks. – Marco Mariani Oct 01 '10 at 09:56
2

I'm not aware of any universal exploitability.

Anyway, it definitely is awfully bad practice.

What the script's author is saying is that the script's security relies on him not forgetting anything in the subsequent includes, which is horrible.

For strong general arguments against global extract()ing, see What is so wrong with extract()?

Community
  • 1
  • 1
Pekka
  • 442,112
  • 142
  • 972
  • 1,088