php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #68815 Explicit null dereference / incorrect code
Submitted: 2015-01-12 16:34 UTC Modified: 2015-01-16 08:08 UTC
From: bugreports at internot dot info Assigned:
Status: Not a bug Package: Filter related
PHP Version: master-Git-2015-01-12 (Git) OS: Linux Ubuntu 14.04
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: bugreports at internot dot info
New email:
PHP Version: OS:

 

 [2015-01-12 16:34 UTC] bugreports at internot dot info
Description:
------------
Hi,


In /ext/standard/filters.c:

277        if (php_strip_tags_filter_ctor(inst, tags_ss.s->val, tags_ss.s->len, persistent) != SUCCESS) {
it should be "tags_ss->s", not "tags_ss.s"

it will cause a null pointer dereference.


Thanks,


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-01-12 16:52 UTC] tony2001@php.net
Here is how tags_ss is defined: 
smart_str tags_ss = {0};

I don't think the compiler will agree to dereference a struct.
Try to change and recompile it yourself.
 [2015-01-12 16:52 UTC] tony2001@php.net
-Status: Open +Status: Feedback
 [2015-01-12 17:14 UTC] bugreports at internot dot info
-Status: Feedback +Status: Open
 [2015-01-12 17:14 UTC] bugreports at internot dot info
I don't understand what you mean.

But it should be tags_ss->s, since..
this code:
265                                smart_str_appendc(&tags_ss, '<');

does this:

 smart_str_appendc_ex((dest), (c), 0)

which does this:


85static zend_always_inline void smart_str_appendc_ex(smart_str *dest, char ch, zend_bool persistent) {
86        size_t new_len = smart_str_alloc(dest, 1, persistent);
87        dest->s->val[new_len - 1] = ch;
88        dest->s->len = new_len;
89}

(notice the dest->s->val, dest->s->len.)

Thanks,
 [2015-01-12 17:54 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2015-01-12 17:54 UTC] stas@php.net
Could you provide a reproduction where the problem happens? Also, if you thing there should be a patch, could you attach the patch after checking locally it compiles and does not break tests (at least for the area it changes)?
 [2015-01-12 18:07 UTC] bugreports at internot dot info
-Status: Feedback +Status: Open
 [2015-01-12 18:07 UTC] bugreports at internot dot info
Mmm. Ok, I get you.

There's another problem, though:

this code:

263                        ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(filterparams), tmp) {
264                                convert_to_string_ex(tmp);
265                                smart_str_appendc(&tags_ss, '<');
266                                smart_str_append(&tags_ss, Z_STR_P(tmp));
267                                smart_str_appendc(&tags_ss, '>');
268                        } ZEND_HASH_FOREACH_END();
269                        smart_str_0(&tags_ss);

ZEND_HASH_FOREACH_VAL is a macro for ZEND_HASH_FOREACH

If this:
(_idx = 0; _idx < (_ht)->nNumUsed; _idx++) is false(from the ZEND_HASH_FOREACH function), then tags_ss.s will be a null pointer.
 [2015-01-13 09:42 UTC] tony2001@php.net
>tags_ss.s will be a null pointer.

And then php_strip_tags_filter_ctor() checks if it's NULL or not before dereferencing.
Do you have a reproduce case in which that check doesn't perform well enough?
 [2015-01-13 09:43 UTC] tony2001@php.net
-Status: Open +Status: Feedback
 [2015-01-15 05:12 UTC] bugreports at internot dot info
-Status: Feedback +Status: Open
 [2015-01-15 05:12 UTC] bugreports at internot dot info
in lieu of https://bugs.php.net/bug.php?id=68817&edit=2, this should probably be checked again.

Not the tags_ss->, but the other thing.
 [2015-01-16 08:08 UTC] stas@php.net
-Status: Open +Status: Not a bug
 [2015-01-16 08:08 UTC] stas@php.net
tags_ss.s indeed may be a null pointer. But smart_str_alloc called from smart_str_appendc_ex takes care of allocating the storage before putting anything there. The problem in 68817 was that s->len was accessed before anything was put into string and thus storage was allocated. However, here we see smart_str_appendc which does allocate storage and initialize the string.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Wed Apr 24 04:01:30 2024 UTC