Implement RF bug #72777 - Add oniguruma stack limit#2109
Implement RF bug #72777 - Add oniguruma stack limit#2109yohgaki wants to merge 6 commits intophp:masterfrom
Conversation
|
looks ok to me |
|
@smalyshev Which branch should I apply? Even if it's not perfect mitigation for ReDoS, it does work for some type of ReDoS attacks and no additional globals. If nobody objects, I would like to apply it from PHP-5.6 branch. |
|
If RMs do not object, I think 5.6 is good. |
| return SUCCESS; | ||
| } | ||
| /* }}} */ | ||
|
|
There was a problem hiding this comment.
If the PHP_INI_ENTRY() is commented out by preprocessor macros, then shouldn't the PHP_INI_MH() function be too?
There was a problem hiding this comment.
Thank you. I should!
|
@cmb69 onig_set_match_stack_limit_size() is not thread safe, that was the last point as it was discussed on the security lists. Either the ini setting has to be PHP_INI_SYSTEM, or the underling lib needs to be patched. That was the last status, AFAIR. Thanks. |
|
Thanks for the update, Anatol! :-) |
|
Having waited more than a month (2) for feedback and activity on this PR, it would seem abandoned, so I'm closing it. |
|
I think we should revive this, having regex functionality with no limits (while underlying library supports limits) is not great. If @yohgaki doesn't update it I plan to try and bring it into 7.1 with necessary fixes. |
|
|
||
| stack_limit = atol(ZSTR_VAL(new_value)); | ||
| if (stack_limit > 0 && stack_limit <= UINT_MAX) { | ||
| onig_set_match_stack_limit_size(stack_limit); |
There was a problem hiding this comment.
We should probably use onig_set_match_stack_limit_size_of_match_param() and onig_match_with_param()
|
@smalyshev As a side note, if you're looking into making mbstring more defensive, you might want to consider backporting f5d2a30 and 2e59426. |
|
@nikic yes I think it makes sense to backport these, we've had overflow issues that happen because of invalid encoding. |
|
@smalyshev Thank you for taking care of this. |
|
This has been implemented in #3997, so closing this one :) |
7 participants
https://bugs.php.net/bug.php?id=72777