Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:26808
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 64.233.166.176 as permitted sender)
DomainKey-Status: good
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:to:cc:subject:date:message-id:mime-version:content-type:x-priority:x-msmail-priority:x-mailer:x-mimeole:importance:from;
        b=IoL5jzzyyuBATLNBkpz/XQBbmvjfCNqTv2Wg4GqqxO6Lkzn0enofvpuep9PR7WzRDNCqPVVzQzI4q5ULubMSoXc6x2rXJEyGSFe0++pziI9nESGfuus+YRRSvF64n3blNqfPM8rkxlzUady8ogotfwXwQeQtKhTTeyjpTzsMVXE=
To: <internals@lists.php.net>
Cc: "'Kevin Hoffman'" <KevinJohnHoffman@gmail.com>
Date: Tue, 5 Dec 2006 23:22:51 -0600
Message-ID: <011a01c718f6$996ac040$6400a8c0@S0HOMbvkxh1>
MIME-Version: 1.0
Content-Type: multipart/mixed;
	boundary="----=_NextPart_000_011B_01C718C4.4ED05040"
Importance: Normal
Subject: [PATCH] Bug #39751: putenv causes string copy of freed memory region, causing crash
From: kevinjohnhoffman@gmail.com (Kevin Hoffman)

------=_NextPart_000_011B_01C718C4.4ED05040
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Bug summary =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

If putenv deletes an environment variable (putenv("VAR=3D")) after it =
was set previously (putenv("VAR=3Dxxx")) then pe->previous_entry (pe is =
the internal hash table entry for the environment variable) will point =
to a freed memory region.

This bug is triggered on windows because pe->previous_entry is set to =
point directly to the environment string (environ[...]) instead of a =
*copy* of the string. The MSVCRT library will free that string when an =
environment variable is deleted. Thus, pe->previous_entry can point to a =
free memory region when it is used later in the putenv_ht hashtable =
deconstructor.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Impact =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Allows possible denial of service attack by crashing Apache.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D PHP Versions =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

I've used this bug to crash Apache using PHP 5.1.2, 5.1.4, 5.2, and the =
latest version from CVS.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Bug Fix List =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

Fixes bug #39751 and probably bug# 36819.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Patch Details =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

PHP_FUNCTION(putenv) is called the first time. The TZ environment =
variable value is set and an entry is created in the putenv_ht hashtable =
with pe.previous_value=3DNULL.

Now the second call to PHP_FUNCTION(putenv) it first executes:
basic_functions.c line 4416
 zend_hash_del(&BG(putenv_ht), pe.key, pe.key_len+1);

This deletes the old entry from the putenv_ht hash table (ok). Then it =
looks through the C environment to see if there is a previous value =
(basic_functions.c lines 4418 - 4425).

Because this is the second time through there is a previous value for =
the TZ environment variable (in my case, it's US/Eastern).
So pe.previous_value =3D *env on line 4422:
 pe.previous_value =3D *env;

Note that its directly pointing to the string managed by the C runtime. =
This is the root cause of the bug (crash).

Now it sets the environment variable using the new value, using the =
putenv lib call. (basic_functions.c line 4435) (In this case, "TZ=3D"). =
Because we're removing the value of the environment variable the bug =
will be triggered.

putenv eventually calls __crtsetenv

In __crtsetenv the remove variable is set to 1, because the string is =
"TZ=3D"
setenv.c line 94
 remove =3D (*(equal + 1) =3D=3D _T('\0'));

The memory for the old environment variable is freed. Any pointers that =
point to env[ix] are now INVALID pointers. This includes the =
pe.previous_value pointer!
setenv.c line 183
 _free_crt(env[ix]);


Now the pe information is inserted into the putenv_ht hashtable. Note =
that the pe.previous_value field is now pointing to a freed block of =
memory (in debug builds, this is immediately set to 0xDD -- in release =
builds it still points to the old data until that memory region is =
allocated and overwritten).

The next time the TZ environment variable is set or when the PHP =
interpreter is exiting and cleaning up (zm_deactivate_basic), the =
destructor on the old hash table entry will be called (the =
php_putenv_destructor function).

It then checks to see if there is a previous value (if previous_value is =
nonzero). If it is, it calls putenv with the previous value.
basic_functions.c lines 3846 - 3855
 ...
 putenv(pe->previous_value);
 ...

However, the pe.previous_value variable points to a freed block of =
memory. putenv immediately tries to copy the new value of the =
environment variable (pe->previous_value).
putenv.c lines 127 - 129:
 if ( (newoption =3D (_TSCHAR *)_malloc_crt((_tcslen(option)+1) *
              sizeof(_TSCHAR))) =3D=3D NULL )
             return -1;

If the new value (pe->previous_value) has been overwritten and is no =
longer zero-terminated then the call to _tcslen (strlen) can possibly =
keep reading memory until it hits an invalid memory region. At this =
point it causes an invalid memory access fault.

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Test Case =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

It's hard to reproduce this bug without a lot of complex code in between =
the place where the environment variable is set to nothing and the place =
where it's set to a value again. I have written an example script that =
causes some memory access assertions sometimes.

------=_NextPart_000_011B_01C718C4.4ED05040
Content-Type: text/plain;
	name="bug39751.patch.txt"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
	filename="bug39751.patch.txt"

Index: ext/standard/basic_functions.c=0A=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0A=
RCS file: /repository/php-src/ext/standard/basic_functions.c,v=0A=
retrieving revision 1.829=0A=
diff -u -r1.829 basic_functions.c=0A=
--- ext/standard/basic_functions.c	5 Dec 2006 04:52:44 -0000	1.829=0A=
+++ ext/standard/basic_functions.c	6 Dec 2006 05:13:02 -0000=0A=
@@ -3852,6 +3852,9 @@=0A=
 		SetEnvironmentVariable(pe->key, "bugbug");=0A=
 #endif=0A=
 		putenv(pe->previous_value);=0A=
+# if defined(PHP_WIN32)=0A=
+		efree(pe->previous_value);=0A=
+# endif=0A=
 	} else {=0A=
 # if HAVE_UNSETENV=0A=
 		unsetenv(pe->key);=0A=
@@ -4419,7 +4422,12 @@=0A=
 		pe.previous_value =3D NULL;=0A=
 		for (env =3D environ; env !=3D NULL && *env !=3D NULL; env++) {=0A=
 			if (!strncmp(*env, pe.key, pe.key_len) && (*env)[pe.key_len] =3D=3D =
'=3D') {	/* found it */=0A=
+#if defined(PHP_WIN32)=0A=
+				/* must copy previous value because MSVCRT's putenv can free the =
string without notice */=0A=
+				pe.previous_value =3D estrndup(*env, 1024);=0A=
+#else=0A=
 				pe.previous_value =3D *env;=0A=
+#endif=0A=
 				break;=0A=
 			}=0A=
 		}=0A=

------=_NextPart_000_011B_01C718C4.4ED05040--