[PATCH] Bug #39751: putenv causes string copy of freed memory region, causing crash

============ Bug summary ============ If putenv deletes an environment variable (putenv("VAR=")) after it was set previously (putenv("VAR=xxx")) 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. ============ Impact ============ Allows possible denial of service attack by crashing Apache. ============ PHP Versions ============ I've used this bug to crash Apache using PHP 5.1.2, 5.1.4, 5.2, and the latest version from CVS. ============ Bug Fix List ============ Fixes bug #39751 and probably bug# 36819. ============ Patch Details ============ 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=NULL. 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 = *env on line 4422: pe.previous_value = *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="). 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=" setenv.c line 94 remove = (*(equal + 1) == _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 = (_TSCHAR *)_malloc_crt((_tcslen(option)+1) * sizeof(_TSCHAR))) == 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. ============ Test Case ============ 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.

Index: ext/standard/basic_functions.c =================================================================== RCS file: /repository/php-src/ext/standard/basic_functions.c,v retrieving revision 1.829 diff -u -r1.829 basic_functions.c --- ext/standard/basic_functions.c 5 Dec 2006 04:52:44 -0000 1.829 +++ ext/standard/basic_functions.c 6 Dec 2006 05:13:02 -0000 @@ -3852,6 +3852,9 @@ SetEnvironmentVariable(pe->key, "bugbug"); #endif putenv(pe->previous_value); +# if defined(PHP_WIN32) + efree(pe->previous_value); +# endif } else { # if HAVE_UNSETENV unsetenv(pe->key); @@ -4419,7 +4422,12 @@ pe.previous_value = NULL; for (env = environ; env != NULL && *env != NULL; env++) { if (!strncmp(*env, pe.key, pe.key_len) && (*env)[pe.key_len] == '=') { /* found it */ +#if defined(PHP_WIN32) + /* must copy previous value because MSVCRT's putenv can free the string without notice */ + pe.previous_value = estrndup(*env, 1024); +#else pe.previous_value = *env; +#endif break; } }