php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Bug #80966 Fishy code in ext/standard/browscap.c
Submitted: 2021-04-18 07:32 UTC Modified: 2021-04-20 10:31 UTC
From: lylgood at foxmail dot com Assigned:
Status: Open Package: *Extensibility Functions
PHP Version: master-Git-2021-04-18 (Git) OS: All
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: lylgood at foxmail dot com
New email:
PHP Version: OS:

 

 [2021-04-18 07:32 UTC] 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 !

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2021-04-19 11:13 UTC] cmb@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: cmb
 [2021-04-19 11:13 UTC] 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-20 03:00 UTC] lylgood at foxmail dot com
-Status: Feedback +Status: Assigned
 [2021-04-20 03:00 UTC] 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-20 10:31 UTC] cmb@php.net
-Summary: A potential use after free bug in ext/standard/browscap.c +Summary: Fishy code in ext/standard/browscap.c -Status: Assigned +Status: Open -Assigned To: cmb +Assigned To:
 [2021-04-20 10:31 UTC] cmb@php.net 
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.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved. 		Last updated: Sat Dec 21 14:01:32 2024 UTC