Suspekt…

Patch breaks Suhosin Security Feature in Debian Unstable/Testing

Two days ago I installed a mail client on my reinstalled desktop system that was not doing anything for 2 month and checked mails of the hardened-php account that were not checked for 2 months. Usually noone uses this email account to contact me, but the Suhosin bug reports sometimes end up there. While killing thousands of SPAM messages I also found a message from the Debian PHP maintainers, dating back to the 10th February 2010, telling me about a crash problem inside the Suhosin patch. The email also contained their solution to the problem: a patch for the suhosin patch. You can view this patch here. However you should not commit this patch to your PHP because it does not solve the problem correctly.

I previously blogged about one of the new features in Suhosin Patch for PHP 5.3.x. It is now possible to adjust several internal features by setting certain environment variables on startup. This includes the memory manager canary protection, the sanitization of free memory blocks, the protection of linked lists and hashtables. When a Suhosin patched PHP starts the environment variables are evaluated and the suhosin config is written into a variable called suhosin_config.

It should be obvious that this kind of feature comes with a little problem. Certain bytes in memory now control if Suhosin’s internal memory protections are activated or not. This means that a memory corruption vulnerability in PHP could be used by an attacker to overwrite the config variable and disable the security. Because of this Suhosin Patch tries to align the suhosin_config variable to a page boundary and then set it to read only.

/* hack that needs to be fixed */
#ifndef PAGE_SIZE
#define PAGE_SIZE 4096
#endif
 
#ifdef ZEND_WIN32
__declspec(align(PAGE_SIZE))
#endif
char suhosin_config[PAGE_SIZE]
#if defined(__GNUC__)
__attribute__ ((aligned(PAGE_SIZE)))
#endif
;
 
static void suhosin_write_protect_configuration()
{
#if defined(__GNUC__)
   mprotect(suhosin_config, PAGE_SIZE, PROT_READ);
#endif
}

The implementation has some problems. First of all it only works in case of a GNU C compiler. The second and more serious problem is that it assumes that the PAGE_SIZE is smaller than or equal to 4096. Otherwise mprotect() will not correctly work. On systems where the PAGE_SIZE is bigger than 4096 the mprotect() will either fail or set too many bytes to read only. In case of a write access after the suhosin_config variable this can lead to a crash.

The Debian people saw this crash on some architectures and reacted with a patch. However they did misunderstand the security idea behind it and therefore their patch looks like this.

char *suhosin_config = NULL;
 
static void suhosin_write_protect_configuration()
{
#if defined(__GNUC__)
   mprotect(suhosin_config, sysconf(_SC_PAGESIZE), PROT_READ);
#endif
}
...
if (!suhosin_config) {
   suhosin_config = mmap(NULL, sysconf(_SC_PAGESIZE), PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
   if (suhosin_config == MAP_FAILED) {
      perror("suhosin");
      _exit(1);
   }
}

The Debian maintainers tried to fix the problem by replacing the aligned suhosin_config variable with a pointer. They then allocate a single memory mapped page and set it to read only. While this fixes the possible crash it shows that the Debian PHP maintainers did not fully understand the idea behind that code. The patch ensures that the suhosin configuration is set to read only, but now a memory corruption exploit can just overwrite the suhosin_config pointer and let it point to a memory area that contains a new configuration.

A correct fix would be to check if the dynamic page size is indeed bigger than 4096 and in this case just warn the user that he should recompile PHP with a bigger PAGE_SIZE definition and do not set the variable to read only in this case. But this might arise the next problem that the PAGE_SIZE might exceed the maximum alignment that the compiler supports.

UPDATE: I rewrote several parts of this blog entry to make it less critic and sound less aggressive. I spent the day discussing possible fixes and other problems with the current solution. The current solution is also not safe in all cases (all OS/architectures/compilers) because of intermediate pointers introduced by the compiler that are invisible at the C level. The solution to this is that the runtime configurability of Suhosin will become optional and can be selected at compile time. If the runtime configurability is selected the sysconf() method will be used to determine the correct page size. The pointer however will be protected by pointer obfuscation/encryption and maybe checksums.