Bug #80966 [Asn->Opn]: Fishy code in ext/standard/browscap.c
Edit report at https://bugs.php.net/bug.php?id=80966&edit=1
ID: 80966
Updated by: cmb@php.net
Reported by: lylgood at foxmail dot com
-Summary: A potential use after free bug in
ext/standard/browscap.c
+Summary: Fishy code in ext/standard/browscap.c
-Status: Assigned
+Status: Open
Type: Bug
Package: *Extensibility Functions
Operating System: All
PHP Version: master-Git-2021-04-18 (Git)
-Assigned To: cmb
+Assigned To:
Block user comment: N
Private report: N
New Comment:
Before the if (persistent), pattern has a refcount >= 1. This is
increased by zend_string_copy(), so the refcount is > 1. If
interning fails, the refcount of pattern is decreased by 1, so
it is still >= 1, i.e. pattern won't be freed. Thus, I don't
see a potential use-after-free here.
However, that code looks fishy, and likely could be cleaned up.
Previous Comments:
------------------------------------------------------------------------
[2021-04-20 03:00:46] lylgood at foxmail dot com
This bug is reported by a code analyzer. Did you mean the pattern return via
zend_new_interned_string() will hold ZSTR_IS_INTERNED(pattern) is true, and will not run into
zend_string_release(pattern) ?
------------------------------------------------------------------------
[2021-04-19 11:13:20] cmb@php.net
That code looks correct to me, since the pattern is copied, so
needs to be released if interning fails. Could you come up with a
reproduce script showing the use-after-free?
------------------------------------------------------------------------
[2021-04-18 07:32:45] lylgood at foxmail dot com
Description:
------------
File: ext/standard/browscap.c
Bug Function: php_browscap_parser_cb
In function php_browscap_parser_cb, pattern is re-assigned by pattern = zend_new_interned_string()
at line 368.
Then if ZSTR_IS_INTERNED(pattern) is false, pattern will be freed via zend_string_release(pattern)
at line 372.
But after that, pattern is still used at line 378 by zend_hash_update_ptr(bdata->htab, pattern,
entry), which is a use after free bug.
Test script:
---------------
if (persistent) {
368: pattern = zend_new_interned_string(zend_string_copy(pattern));
if (ZSTR_IS_INTERNED(pattern)) {
Z_TYPE_FLAGS_P(arg1) = 0;
} else {
372: zend_string_release(pattern); //pattern could be freed !
}
}
...
378: zend_hash_update_ptr(bdata->htab, pattern, entry);//freed pattern is used !
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=80966&edit=1
Thread (4 messages)