Fix negative-size-param crash when OBJ_obj2txt() fails#21008
Fix negative-size-param crash when OBJ_obj2txt() fails#21008ndossche wants to merge 1 commit intophp:masterfrom
Conversation
When the function returns -1, the length passed to the string
constructor is negative:
```
==188567==ERROR: AddressSanitizer: negative-size-param: (size=-1)
#0 0x7f36ea0305bd in memcpy ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors_memintrinsics.inc:115
#1 0x559c568a05b3 in zend_string_init /work/php-src/Zend/zend_string.h:191
#2 0x559c568b3cb7 in add_assoc_stringl_ex /work/php-src/Zend/zend_API.c:1986
#3 0x559c559234a2 in add_assoc_stringl /work/php-src/Zend/zend_API.h:579
#4 0x559c55928b3e in php_openssl_pkey_get_details /work/php-src/ext/openssl/openssl_backend_v3.c:671
#5 0x559c559006d4 in zif_openssl_pkey_get_details /work/php-src/ext/openssl/openssl.c:2319
#6 0x559c566b7ed2 in zend_test_execute_internal /work/php-src/ext/zend_test/observer.c:306
#7 0x559c569e024a in ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER /work/php-src/Zend/zend_vm_execute.h:2154
#8 0x559c56b40995 in execute_ex /work/php-src/Zend/zend_vm_execute.h:116519
#9 0x559c56b558b0 in zend_execute /work/php-src/Zend/zend_vm_execute.h:121962
#10 0x559c56cba0ab in zend_execute_script /work/php-src/Zend/zend.c:1980
#11 0x559c566ec8bb in php_execute_script_ex /work/php-src/main/main.c:2645
#12 0x559c566ecccb in php_execute_script /work/php-src/main/main.c:2685
#13 0x559c56cbfc16 in do_cli /work/php-src/sapi/cli/php_cli.c:951
#14 0x559c56cc21e3 in main /work/php-src/sapi/cli/php_cli.c:1362
#15 0x7f36e932d1c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
#16 0x7f36e932d28a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
#17 0x559c55809b34 in _start (/work/php-src/build-dbg-asan/sapi/cli/php+0x609b34) (BuildId: aa149f943514fff0c491e1f199e30fed0e977f7c)
```
|
Actually noticed via another report that if OBJ_obj2txt returns 0 (which it can on failure) that the failure is not propagated either. So I may need to adjust this PR. |
| add_assoc_stringl(&ary, "curve_oid", oir_buf, oir_len); | ||
| ASN1_OBJECT_free(obj); | ||
| if (oir_len < 0) { | ||
| ktype = -2; |
There was a problem hiding this comment.
As this is unlikely to happen, I'm not sure we should add new ktype...
There was a problem hiding this comment.
In an ideal world we'd have an ADT type like in Rust to signal an error and then we don't need such magic numbers.
There was a problem hiding this comment.
what I'm saying is that this fails only on low memory because that obj is from known nid that is for valid curve name so the only case where this fail would be malloc and freinds failure as far as I see. In such case the process will most likely get killed soon so don't think we need to document new error type for this...
I don't think this is possible because obj has to be valid at this stage and the only case where 0 as far as I see is returned is here (a is the obj): I might be missing something though... |
It is possible in LibreSSL: which returns only 0 on error, not -1: |
3 participants
When the function returns -1, the length passed to the string constructor is negative:
This was found by a hybrid static-dynamic analyser that looks for inconsistent handling of error checks in bindings.