Skip to content

Fix #74604: Out of bounds in php_pcre_replace_impl - #7597

Closed
cmb69 wants to merge 1 commit into
php:PHP-7.4from
cmb69:cmb/74604
Closed

cmb69 wants to merge 1 commit into
php:PHP-7.4from
cmb69:cmb/74604

Conversation

@cmb69

@cmb69 cmb69 commented Oct 20, 2021

Copy link
Copy Markdown
Member

Trying to allocate a zend_string with a length only slighty smaller
than SIZE_MAX causes an integer overflow; we need to ensure this does
not happen, and throw an Error instead.

@cmb69

cmb69 commented Nov 2, 2021

Copy link
Copy Markdown
Member Author

If there are no objections, I'll merge this by the end of the week.

Comment thread ext/pcre/php_pcre.c Resolved
@cmb69

cmb69 commented Nov 25, 2021

Copy link
Copy Markdown
Member Author

@dstogov, what do you think about this PR?

@dstogov

dstogov commented Nov 25, 2021

Copy link
Copy Markdown
Member

I think this may be fixed in a simpler way:

- alloc_len = zend_safe_address_guarded(2, new_len, 0);
+ alloc_len = zend_safe_address_guarded(2, new_len, _ZSTR_HEADER_SIZE) - _ZSTR_HEADER_SIZE;

- new_len = zend_safe_address_guarded(1, ZSTR_LEN(eval_result), new_len);
+ new_len = zend_safe_address_guarded(1, ZSTR_LEN(eval_result) + _ZSTR_HEADER_SIZE, new_len) - _ZSTR_HEADER_SIZE;

I didn't test this.

@cmb69

cmb69 commented Nov 25, 2021

Copy link
Copy Markdown
Member Author

Thank you! I'll have a closer look.

@cmb69
cmb69 marked this pull request as draft November 25, 2021 13:21
Trying to allocate a `zend_string` with a length only slighty smaller
than `SIZE_MAX` causes an integer overflow; we make sure that this
doesn't happen by catering to the maximal overhead of a `zend_string`.
@cmb69
cmb69 marked this pull request as ready for review November 29, 2021 16:12
@cmb69

cmb69 commented Nov 29, 2021

Copy link
Copy Markdown
Member Author

@dstogov, _ZSTR_HEADER_SIZE doesn't seem to be quite right, due to the trailing NUL byte and the alignment. I've used ZEND_MM_ALIGNED_SIZE(_ZSTR_HEADER_SIZE + 1) and added a macro for that.

@dstogov dstogov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this is fine now.

@cmb69 cmb69 closed this in 712fc54 Nov 29, 2021
@cmb69
cmb69 deleted the cmb/74604 branch November 29, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants