Skip to content

regcomp: infinite loop (perl-5.30.0) #17594

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

Closed
lightsey opened this issue Feb 28, 2020 · 6 comments
Closed

regcomp: infinite loop (perl-5.30.0) #17594

lightsey opened this issue Feb 28, 2020 · 6 comments
Milestone

Comments

@lightsey
Copy link
Contributor

Description

On May 24, 2019 @Etsukata reported an infinite loop bug to the Perl security contact address. This report was originally tracked as rt.perl.org#134132. After the github migration, it was tracked as Perl/perl5-security#112

The original example regex was:

perl -e 'qr/\p{jg=​:A?\K+​:}/'

The infinite loop this regex caused was not considered to be a vulnerability by the Perl security team and an initial fix for the bug was committed to blead as ac3afc4

This initial fix resulted in significant discussion about it's suitability for a backport to the maintenance branch. Some of this discussion continued in the private issue tracker after the issue was initially resolved.

I don't understand the details of these discussions sufficiently to summarize them, but I'm opening this issue to track any remaining work related to the original Perl/perl5-security#112 report.

@khwilliamson khwilliamson added this to the 5.32.0 milestone Mar 20, 2020
@khwilliamson khwilliamson modified the milestones: 5.32.0, 5.30.3 Apr 2, 2020
@hvds
Copy link
Contributor

hvds commented Apr 21, 2020

A related issue has been raised as #17730. I've pushed https://github.com/Perl/perl5/tree/smoke-me/hv/gh17594 for smoking, which stops marking any zero-width constructs as SIMPLE.

I've also split it into two commits, in which the first stops marking them SIMPLE (which should stop them getting to regrepeat(), which can't handle them correctly), and the second actually disallows them in regrepeat(). If we come to consider this for 5.32 or backporting, taking only the first commit may be the safer option.

@hvds
Copy link
Contributor

hvds commented Oct 5, 2020

@xsawyerx @khwilliamson @lightsey I've rebased this branch onto blead; I'd like to get it merged so we get an idea whether it causes any issues comfortably before next release, any opinions for or against that?

There are two commits; the first (which I think is uncontroversial) stops marking certain zero-width constructs as SIMPLE, and documents more clearly that SIMPLE nodes are expected to be one character wide. The second (which may be more controversial) removes the cases for those zero-width constructs in regrepeat(), so we'll croak if we see them there rather than doing a wrong calculation.

@khwilliamson
Copy link
Contributor

It LGTM. I think it should be merged

@xsawyerx
Copy link
Member

xsawyerx commented Oct 8, 2020

👍 good with me.

@khwilliamson
Copy link
Contributor

@lightsey replied to me privately. So its fine to merge this now.

hvds added a commit that referenced this issue Oct 8, 2020
GH #17594: avoid marking them SIMPLE during compile so they will not
wrongly be permitted in STAR/PLUS/CURLY nodes: a SIMPLE node is supposed
to have width 1 (with the exception of LNBREAK as a special case).
hvds added a commit that referenced this issue Oct 8, 2020
GH #17594: the logic here expects the node to have width 1 (except for
LNBREAK), it is not expected to do the right thing on zero-width nodes.
@hvds
Copy link
Contributor

hvds commented Oct 8, 2020

Now merged with commits fc7dfe8 and 4f0d304.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants