Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RF bug #72777 - Add oniguruma stack limit #2109

Open
wants to merge 6 commits into
base: master
from

Conversation

7 participants
@yohgaki
Copy link
Contributor

commented Sep 1, 2016

Yasuo Ohgaki added some commits Sep 1, 2016

Yasuo Ohgaki
Yasuo Ohgaki
Yasuo Ohgaki
@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2016

looks ok to me

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2016

@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.

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

If RMs do not object, I think 5.6 is good.

@@ -1473,6 +1473,25 @@ static PHP_INI_MH(OnUpdate_mbstring_http_output_conv_mimetypes)
return SUCCESS;
}
/* }}} */

This comment has been minimized.

Copy link
@KalleZ

KalleZ Sep 9, 2016

Member

If the PHP_INI_ENTRY() is commented out by preprocessor macros, then shouldn't the PHP_INI_MH() function be too?

This comment has been minimized.

Copy link
@yohgaki

yohgaki Sep 21, 2016

Author Contributor

Thank you. I should!

Yasuo Ohgaki added some commits Sep 22, 2016

@cmb69

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

If RMs do not object, I think 5.6 is good.

ACK.

@Tyrael, @weltling what do you think?

@weltling

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

@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.

@cmb69

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

Thanks for the update, Anatol! :-)

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 3, 2017

@yohgaki can you apply the change requested by @weltling, alternatively, if you consider this work abandoned (because the fix belongs upstream), please close this PR.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Mar 1, 2017

Having waited more than a month (2) for feedback and activity on this PR, it would seem abandoned, so I'm closing it.

@krakjoe krakjoe closed this Mar 1, 2017

@smalyshev smalyshev reopened this Mar 18, 2019

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

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);

This comment has been minimized.

Copy link
@smalyshev

smalyshev Mar 18, 2019

Contributor

We should probably use onig_set_match_stack_limit_size_of_match_param() and onig_match_with_param()

@nikic

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@smalyshev As a side note, if you're looking into making mbstring more defensive, you might want to consider backporting f5d2a30 and 2e59426.

@smalyshev

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

@nikic yes I think it makes sense to backport these, we've had overflow issues that happen because of invalid encoding.

@yohgaki

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

@smalyshev Thank you for taking care of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.