php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #55749 TOCTOU issue in getenv() on Windows builds
Submitted: 2011-09-21 04:53 UTC Modified: 2011-09-30 17:11 UTC
From: mattjd at gmail dot com Assigned: pajoye
Status: Closed Package: Unknown/Other Function
PHP Version: trunk-SVN-2011-09-21 (SVN) OS: Windows
Private report: No CVE-ID:
 [2011-09-21 04:53 UTC] mattjd at gmail dot com
Description:
------------
(sent to security@php.net)

There is a TOCTOU-ish issue in the php function getenv() on Windows
builds. See here:
http://svn.php.net/viewvc/php/php-src/trunk/ext/standard/basic_functions.c?
revision=316689&view=markup#l4005

Note that the size of the environment variable is retrieved first on line 4014:
size = GetEnvironmentVariableA(str, &dummybuf, 0);

It is then used to malloc a buffer big enough to hold the variable (at
that time) on line 4025:
ptr = emalloc(size);

This is then used on the next line to retrieve the environment variable into:
size = GetEnvironmentVariableA(str, ptr, size);

What this does not take into account is that the second call to
GetEnvironmentVariableA may fail due to ie. the environment variable
not being found (ERROR_ENVVAR_NOT_FOUND), having been deleted in
another thread.

This leads to the use of the still-uninitialized buffer in
RETURN_STRING which leads to a garbage string being returned, one that
addresses some location on the heap. This string leaks heap
information as well as allowing heap modification over the area which
it spans (which may be bigger than the malloc'd buffer size, due to
the lack of a null terminator).

Running these two scripts, at the same time, and in a multi threaded
situation (ie. using an Apache handler DLL or FastCGI) demonstrates
the issue.

Test script:
---------------
<?php
for(;;) {
       putenv("PHP_TEST=" . str_repeat("A", rand(0, 1024)));
       putenv("PHP_TEST");
}
?>

<?php
for(;;) {
       do {
               $r = getenv("PHP_TEST");
       } while ($r === FALSE || $r === "" || $r[0] === "A");

       for($i = 0; $i < strlen($r); ++$i)
               printf("%02x ", ord($r[$i]));
       print "<br>\n";
       flush();

       //Uncomment this to corrupt the heap:
       //for($i = 0; $i < strlen($r); ++$i)
       //      $r[$i] = "\xCC";
}
?>

Expected result:
----------------
No output from either script.

Actual result:
--------------
Second script outputs parts of the heap.

Patches

getenv_notfound (last revision 2011-09-21 06:24 UTC) by pajoye@php.net)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-09-21 06:23 UTC] pajoye@php.net
On Wed, Sep 21, 2011 at 6:56 AM, matt d <mattjd at gmail dot com> wrote:
> Sure thing: https://bugs.php.net/bug.php?id=55749
>
> I think you'd need to check GetLastError() after the second call to
> GetEnvironmentVariableA, as that's how you can check for
> ERROR_ENVVAR_NOT_FOUND.

I could but it is not necessary. GetLastError only tells me why it fails, which 
is not that important here. But on failure (not found or other errors), the 
return value is zero. See http://msdn.microsoft.com/en-
us/library/windows/desktop/ms683188(v=vs.85).aspx
 [2011-09-21 06:24 UTC] pajoye@php.net
The following patch has been added/updated:

Patch Name: getenv_notfound
Revision:   1316586277
URL:        https://bugs.php.net/patch-display.php?bug=55749&patch=getenv_notfound&revision=1316586277
 [2011-09-26 08:51 UTC] pajoye@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: pajoye
 [2011-09-26 08:51 UTC] pajoye@php.net
This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

Thanks for the catch up!

See  http://svn.php.net/viewvc?view=revision&revision=317306

and http://svn.php.net/viewvc?view=revision&revision=317305 (committed in 5.4 by 
mistake with another fix).
 [2011-09-27 03:03 UTC] mattjd at gmail dot com
Sorry for the late response, but what about this case:

From MSDN: "If lpBuffer is not large enough to hold the data, the return value is 
the buffer size, in characters, required to hold the string and its terminating 
null character and the contents of lpBuffer are undefined."

The environment variable might not only be removed/shortened/changed, but could be 
expanded as well.
 [2011-09-27 07:36 UTC] pajoye@php.net
That's nothing we can do about that, unless we do some kind of loops and pray that 
nothing changed in between :). Or do you have something in mind?

Actually I'm thinking about using fixed length buffer for all PATHs or other 
smaller than max path length values, and be done with that.
 [2011-09-29 03:08 UTC] mattjd at gmail dot com
Having a loop that could take some time to complete in a core function like getenv 
doesn't sit well with me.

Other languages' implementations of the same feature appear to either only read 
the environment at startup, or rely on the user for thread safety.

Regardless of the choice made, we need to ensure that a garbage string is never 
returned.
 [2011-09-29 09:26 UTC] pajoye@php.net
it is never returned as of now.

I will simply fail if the returned length is not the same than the one we expect. 
That's cleaner and will avoid any other issues due to concurrent (but thread safe) 
updates of a variable.
 [2011-09-30 16:34 UTC] mattjd at gmail dot com
As it stands now (the current code in trunk), a garbage string will be returned 
if the environment variable's value expands in size between calls. The second 
call to GetEnvironmentVariableA leaves the buffer untouched, and doesn't return 
0; instead it returns the required buffer size.

The logic would need to look something like this:

char dummybuf;
int size, size2;

SetLastError(0);
/*If the given bugger is not large enough to hold the data, the return value is 
the buffer size,  in characters, required to hold the string and its terminating 
null character. We use this return value to alloc the final buffer. */
size = GetEnvironmentVariableA(str, &dummybuf, 0);
if (size == 0) {
	if (GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
		/* The environment variable doesn't exist. */
		RETURN_FALSE;
	}

	/* env exists, but it is empty */
	RETURN_EMPTY_STRING();
}

ptr = emalloc(size);
SetLastError(0);
size2 = GetEnvironmentVariableA(str, ptr, size);
if (size == 0) {
	efree(ptr);

	if (GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
		/* The environment variable doesn't exist. */
		RETURN_FALSE;
	}

	/* env exists, but it is empty */
	RETURN_EMPTY_STRING();
} else if (size2 < size) {
	/* got the variable value */
	RETURN_STRING(ptr, 0);
} else {
	/* variable expanded between calls */
	/* ??? */
}

..where the /* ??? */ is replaced by however you propose to handle failure, I 
guess :)

Also, I've found that the non-Windows code path is actually also vulnerable to 
the same problem; running the same two scripts on Linux using Apache with the 
worker MPM and a threadsafe build of PHP produces the same results as Windows; 
the heap is leaked (and modifiable). (I guess why this is why the use of the 
worker MPM isn't recommended, huh)

Perhaps there is another issue in that the {get,put,unset}env() family of 
functions need to be used in a threadsafe manner. For example, use of getenv() 
would involve an invocation, and then its result being copied to a local buffer, 
atomically.

An alternative, if there are already the appropriate threadsafe data structures 
available, would be to make a local copy of the environment at process startup, 
and use that in the implementations of the PHP {get,put}env functions (and wherever else needed).
 [2011-09-30 17:11 UTC] pajoye@php.net
See my last comment, that covers what you are describing, if the length changed, 
we return FALSE and be done with it (as it can change over and over while we 
realloc the right buffer).

About unix and thread safe SAPI, it is well known that getenv is not thread safe 
on unix.
 
PHP Copyright © 2001-2014 The PHP Group
All rights reserved.
Last updated: Thu Apr 24 21:01:55 2014 UTC