search

php / php-src

The PHP Interpreter
https://www.php.net
Other
38.11k stars 7.74k forks source link

Disabling a class with typed properties causes a use after free #11960

Open Girgias opened 1 year ago

Girgias commented 1 year ago

Description

Found as part of investigating #11884

Disabling a class with typed properties will free a property in zend_disable_class() and then again in destroy_zend_class()

--TEST--
Disable class with typed properties
--INI--
disable_classes=Exception
--XFAIL--
Use After Free due to types being freed by zend_disable_class()
--FILE--
<?php
$o = new Exception();
?>

Resulted in this output:

Warning: Exception() has been disabled for security reasons in /home/girgias/dev/php-src/Zend/tests/type_declarations/disable_internal_class_typed_property.php on line 6
=================================================================
==376078==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000093a0 at pc 0x0000012b8815 bp 0x7fff75bf7bd0 sp 0x7fff75bf7bc8
READ of size 8 at 0x6060000093a0 thread T0
    #0 0x12b8814 in destroy_zend_class /home/girgias/dev/php-src/Zend/zend_opcode.c:471
    #1 0x137da79 in _zend_hash_del_el_ex /home/girgias/dev/php-src/Zend/zend_hash.c:1435
    #2 0x137e113 in _zend_hash_del_el /home/girgias/dev/php-src/Zend/zend_hash.c:1462
    #3 0x13870dc in zend_hash_graceful_reverse_destroy /home/girgias/dev/php-src/Zend/zend_hash.c:1987
    #4 0x1308dce in zend_shutdown /home/girgias/dev/php-src/Zend/zend.c:1126
    #5 0x10928ad in php_module_shutdown /home/girgias/dev/php-src/main/main.c:2367
    #6 0x1a7398e in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1353
    #7 0x7fd6cd249b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #8 0x7fd6cd249c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #9 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

0x6060000093a0 is located 32 bytes inside of 56-byte region [0x606000009380,0x6060000093b8)
freed by thread T0 here:
    #0 0x7fd6cd4d7fb8 in __interceptor_free.part.0 (/lib64/libasan.so.8+0xd7fb8) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x1347590 in zend_disable_class /home/girgias/dev/php-src/Zend/zend_API.c:3553
    #2 0x1086eff in php_disable_classes /home/girgias/dev/php-src/main/main.c:352
    #3 0x10919a7 in php_module_startup /home/girgias/dev/php-src/main/main.c:2228
    #4 0x1a6e0e6 in php_cli_startup /home/girgias/dev/php-src/sapi/cli/php_cli.c:410
    #5 0x1a7368f in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1307
    #6 0x7fd6cd249b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #7 0x7fd6cd249c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #8 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

previously allocated by thread T0 here:
    #0 0x7fd6cd4d92ef in malloc (/lib64/libasan.so.8+0xd92ef) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x11fa0a5 in __zend_malloc /home/girgias/dev/php-src/Zend/zend_alloc.c:3130
    #2 0x1354e70 in zend_declare_typed_property /home/girgias/dev/php-src/Zend/zend_API.c:4337
    #3 0x1359fb6 in zend_declare_property_ex /home/girgias/dev/php-src/Zend/zend_API.c:4582
    #4 0x16ffbc7 in register_class_Exception /home/girgias/dev/php-src/Zend/zend_exceptions_arginfo.h:212
    #5 0x170eea5 in zend_register_default_exception /home/girgias/dev/php-src/Zend/zend_exceptions.c:764
    #6 0x17c37cc in zend_register_default_classes /home/girgias/dev/php-src/Zend/zend_default_classes.c:35
    #7 0x13a29b3 in zm_startup_core /home/girgias/dev/php-src/Zend/zend_builtin_functions.c:38
    #8 0x1334548 in zend_startup_module_ex /home/girgias/dev/php-src/Zend/zend_API.c:2311
    #9 0x13346a2 in zend_startup_module_zval /home/girgias/dev/php-src/Zend/zend_API.c:2326
    #10 0x13879b1 in zend_hash_apply /home/girgias/dev/php-src/Zend/zend_hash.c:2033
    #11 0x1336849 in zend_startup_modules /home/girgias/dev/php-src/Zend/zend_API.c:2437
    #12 0x10917e3 in php_module_startup /home/girgias/dev/php-src/main/main.c:2210
    #13 0x1a6e0e6 in php_cli_startup /home/girgias/dev/php-src/sapi/cli/php_cli.c:410
    #14 0x1a7368f in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1307
    #15 0x7fd6cd249b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #16 0x7fd6cd249c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #17 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

SUMMARY: AddressSanitizer: heap-use-after-free /home/girgias/dev/php-src/Zend/zend_opcode.c:471 in destroy_zend_class
Shadow bytes around the buggy address:
  0x606000009100: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
  0x606000009180: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x606000009200: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
  0x606000009280: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
  0x606000009300: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
=>0x606000009380: fd fd fd fd[fd]fd fd fa fa fa fa fa fd fd fd fd
  0x606000009400: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
  0x606000009480: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x606000009500: fd fd fd fd fd fd fd fd fa fa fa fa fd fd fd fd
  0x606000009580: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
  0x606000009600: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==376078==ABORTING

I will note, that the engine seems very brittle to disabling classes, as just:

--TEST--
Disable class
--EXTENSIONS--
zend_test
reflection
--INI--
disable_classes=ReflectionMethod
--XFAIL--
Use After Free due to types being freed by zend_disable_class()
--FILE--
<?php

$o = new ReflectionMethod(Exception::class, "__toString");
?>

Will not cause any UAF however adding a var_dump($o); causes a different UAF:

Warning: ReflectionMethod() has been disabled for security reasons in /home/girgias/dev/php-src/Zend/tests/type_declarations/disable_internal_class_typed_property.php on line 3
=================================================================
==379992==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000010820 at pc 0x0000017a4e17 bp 0x7fff099cae50 sp 0x7fff099cae48
READ of size 4 at 0x606000010820 thread T0
    #0 0x17a4e16 in rebuild_object_properties /home/girgias/dev/php-src/Zend/zend_object_handlers.c:78
    #1 0x17a58fc in zend_std_get_properties /home/girgias/dev/php-src/Zend/zend_object_handlers.c:129
    #2 0x17a61c2 in zend_std_get_debug_info /home/girgias/dev/php-src/Zend/zend_object_handlers.c:163
    #3 0x17bfec5 in zend_std_get_properties_for /home/girgias/dev/php-src/Zend/zend_object_handlers.c:1970
    #4 0x17c02d1 in zend_get_properties_for /home/girgias/dev/php-src/Zend/zend_object_handlers.c:1999
    #5 0xf73892 in php_var_dump /home/girgias/dev/php-src/ext/standard/var.c:163
    #6 0xf74a00 in zif_var_dump /home/girgias/dev/php-src/ext/standard/var.c:228
    #7 0x142b1f9 in ZEND_DO_ICALL_SPEC_RETVAL_UNUSED_HANDLER /home/girgias/dev/php-src/Zend/zend_vm_execute.h:1275
    #8 0x16beb41 in execute_ex /home/girgias/dev/php-src/Zend/zend_vm_execute.h:57191
    #9 0x16da2f1 in zend_execute /home/girgias/dev/php-src/Zend/zend_vm_execute.h:61583
    #10 0x13106ae in zend_execute_scripts /home/girgias/dev/php-src/Zend/zend.c:1875
    #11 0x10933d3 in php_execute_script /home/girgias/dev/php-src/main/main.c:2492
    #12 0x1a710a4 in do_cli /home/girgias/dev/php-src/sapi/cli/php_cli.c:966
    #13 0x1a7384d in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1340
    #14 0x7ff3d1849b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #15 0x7ff3d1849c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #16 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

0x606000010820 is located 0 bytes inside of 56-byte region [0x606000010820,0x606000010858)
freed by thread T0 here:
    #0 0x7ff3d1ad7fb8 in __interceptor_free.part.0 (/lib64/libasan.so.8+0xd7fb8) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x1347590 in zend_disable_class /home/girgias/dev/php-src/Zend/zend_API.c:3553
    #2 0x1086eff in php_disable_classes /home/girgias/dev/php-src/main/main.c:352
    #3 0x10919a7 in php_module_startup /home/girgias/dev/php-src/main/main.c:2228
    #4 0x1a6e0e6 in php_cli_startup /home/girgias/dev/php-src/sapi/cli/php_cli.c:410
    #5 0x1a7368f in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1307
    #6 0x7ff3d1849b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #7 0x7ff3d1849c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #8 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

previously allocated by thread T0 here:
    #0 0x7ff3d1ad92ef in malloc (/lib64/libasan.so.8+0xd92ef) (BuildId: e5f0a0d511a659fbc47bf41072869139cb2db47f)
    #1 0x11fa0a5 in __zend_malloc /home/girgias/dev/php-src/Zend/zend_alloc.c:3130
    #2 0x1354e70 in zend_declare_typed_property /home/girgias/dev/php-src/Zend/zend_API.c:4337
    #3 0xb74f2b in register_class_ReflectionMethod /home/girgias/dev/php-src/ext/reflection/php_reflection_arginfo.h:1334
    #4 0xc04adb in zm_startup_reflection /home/girgias/dev/php-src/ext/reflection/php_reflection.c:7233
    #5 0x1334548 in zend_startup_module_ex /home/girgias/dev/php-src/Zend/zend_API.c:2311
    #6 0x13346a2 in zend_startup_module_zval /home/girgias/dev/php-src/Zend/zend_API.c:2326
    #7 0x13879b1 in zend_hash_apply /home/girgias/dev/php-src/Zend/zend_hash.c:2033
    #8 0x1336849 in zend_startup_modules /home/girgias/dev/php-src/Zend/zend_API.c:2437
    #9 0x10917e3 in php_module_startup /home/girgias/dev/php-src/main/main.c:2210
    #10 0x1a6e0e6 in php_cli_startup /home/girgias/dev/php-src/sapi/cli/php_cli.c:410
    #11 0x1a7368f in main /home/girgias/dev/php-src/sapi/cli/php_cli.c:1307
    #12 0x7ff3d1849b49 in __libc_start_call_main (/lib64/libc.so.6+0x27b49) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #13 0x7ff3d1849c0a in __libc_start_main_alias_2 (/lib64/libc.so.6+0x27c0a) (BuildId: 245240a31888ad5c11bbc55b18e02d87388f59a9)
    #14 0x602524 in _start (/home/girgias/dev/php-src/sapi/cli/php+0x602524) (BuildId: 39d90bf93768af8e2054956234a459f636cc8e76)

SUMMARY: AddressSanitizer: heap-use-after-free /home/girgias/dev/php-src/Zend/zend_object_handlers.c:78 in rebuild_object_properties
Shadow bytes around the buggy address:
  0x606000010580: fd fd fd fd fd fd fd fd fa fa fa fa 00 00 00 00
  0x606000010600: 00 00 00 fa fa fa fa fa 00 00 00 00 00 00 00 fa
  0x606000010680: fa fa fa fa 00 00 00 00 00 00 00 fa fa fa fa fa
  0x606000010700: 00 00 00 00 00 00 00 fa fa fa fa fa 00 00 00 00
  0x606000010780: 00 00 00 fa fa fa fa fa 00 00 00 00 00 00 00 fa
=>0x606000010800: fa fa fa fa[fd]fd fd fd fd fd fd fa fa fa fa fa
  0x606000010880: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
  0x606000010900: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
  0x606000010980: fa fa fa fa 00 00 00 00 00 00 00 00 fa fa fa fa
  0x606000010a00: 00 00 00 00 00 00 00 00 fa fa fa fa 00 00 00 00
  0x606000010a80: 00 00 00 00 fa fa fa fa 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object red

PHP Version

PHP 8.1

Operating System

No response

iluuu1994 commented 1 year ago

It looks like this is caused by these lines:

https://github.com/php/php-src/blob/902d39d57cd4451f7b8a23d55452ce8dbb4d7259/Zend/zend_API.c#L3341-L3348

The issue is that classes are only disabled after all classes have been loaded. This includes subclasses, which may have inherited these property infos. Accessing these property infos from the subclass will lead to a use-after-free. Disabling a parent class without also disabling the child class is bogus. I suppose we could recursively disable all subclasses.

Edit: Or we just error saying that the child class must also be disabled. Moreover, I'm not sure if disabling a core class like Exception can't lead to other issues somewhere.

Girgias commented 1 year ago

I'm very much questioning the utility of this "feature" as I would expect loads of issues to arise disabling core classes that we do not at all take into account, and most other objects in extensions are required for them to function properly (imagine disabling the cURL handler)

KapitanOczywisty commented 1 year ago

I've checked ~100 shared www hosts, and disable_classes wasn't used on any of them, at the same time disable_functions was used on more than half. In worst case the php source could be patched to explicitly disable some classes, if someone would need to. In most cases php is virtualized or sandboxed anyway. And contrary to ML I don't think that using removed ini directive causes any warnings not to mention fatal errors.

jhdxr commented 1 year ago

disable_functions offers a way to rewrite the core functions - disable them and reimplement them in userland before any use. However, it used to work in a way similar to disable_classes until a feature request (https://bugs.php.net/bug.php?id=79382, #5473 ) changes it.

Instead of fixing this, we may either deprecate and remove this feature, or change it to the way how disable_functions now to make it more useful.

iluuu1994 commented 1 year ago

@jhdxr As Nikita wrote in the PR, a lot of C code refers to internal classes directly through a pointer, rather than by name and going through the symbol table. This can save a lot of unnecessary lookups when the lookup should always refer to that same class. Furthermore, the reason this feature is currently unreliable is because code internally makes assumptions about the classes and the shape of its instances. If classes can be modified by users it sounds like this issue is going to be exacerbated, rather than mitigated.

Girgias commented 1 year ago

disable_functions offers a way to rewrite the core functions - disable them and reimplement them in userland before any use. However, it used to work in a way similar to disable_classes until a feature request (https://bugs.php.net/bug.php?id=79382, #5473 ) changes it.

Instead of fixing this, we may either deprecate and remove this feature, or change it to the way how disable_functions now to make it more useful.

I am planning on removing this outright in 8.4, I just need to work out one small thing.

jhdxr commented 1 year ago

I am planning on removing this outright in 8.4, I just need to work out one small thing.

I'm OK with remvoing it. Going through the old bug tracker will find it causes more trouble. Security, as the warning message indicates, is not a good reason to keep it as well IMHO, if there is vulnerability in the implementation itself, it should be fixed and user should upgrade PHP. If anyone really wants to disable certain classes, better just disable the whole modules.

However, removing in 8.4 seems too fast. maybe deprecated it in 8.4 and remove it in 8.5?

Girgias commented 1 year ago

I am planning on removing this outright in 8.4, I just need to work out one small thing.

I'm OK with remvoing it. Going through the old bug tracker will find it causes more trouble. Security, as the warning message indicates, is not a good reason to keep it as well IMHO, if there is vulnerability in the implementation itself, it should be fixed and user should upgrade PHP. If anyone really wants to disable certain classes, better just disable the whole modules.

However, removing in 8.4 seems too fast. maybe deprecated it in 8.4 and remove it in 8.5?

No, there is utterly no reason for this feature. And this is getting RFCed anyway. Even just having this "deprecated" would mean pottentially fixing issues. And this feature has never worked, has always been pointless, and I doubt has actually ever been used for anything meaningful.

Considering the amount of stuff this declare breaks, this INI setting provides negative value.

jhdxr commented 1 year ago

Correct me if I'm wrong, but I cannot recall any precedent to remove some documented ini setting and functions without deprecation. Anyway I'm OK with RFC, while I may not vote for an instantly removal.