-
Notifications
You must be signed in to change notification settings - Fork 560
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
heap-buffer-overflow in S_regmatch (regexec.c:5439) #15634
Comments
From @geeknikperl -e '(x)!~/0|()0|()\1/&0' triggers a heap-buffer-overflow in ==22476==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000e20f is located 1 bytes to the left of 10-byte region SUMMARY: AddressSanitizer: heap-buffer-overflow /root/perl/regexec.c:5439 Perl 5.20.2 doesn't crash, but this happens while running under Valgrind: ==25966== Invalid read of size 1 |
From @hvdsThis pattern is causing it to read before the start of the string when we hit the backreference, as can be seen from the offsets and failure output under debug: % perl -Mre=Debug,EXECUTE -e '"x"=~/()y|()\1/' 2>&1 | grep - (Note that we get an additional bad read in pv_escape when debug is turned on.) I /think/ it's not possible for the negative offsets to leak out to something that'd cause us to write; if that's correct, I don't believe this is a security issue. I'd welcome others' opinions. Patch attached should fix it; test checks for the negative offset in debug output (where available), not sure if there's a better way to do that. Hugo |
From @hvds |
The RT System itself - Status changed from 'new' to 'open' |
From @geeknikFound in Perl v5.25.6 (v5.25.5-72-g2814f4b) with AFL + ASAN. Minimizing ==6272==ERROR: AddressSanitizer: heap-use-after-free on address 0x60200000dc50 is located 0 bytes inside of 10-byte region previously allocated by thread T0 here: SUMMARY: AddressSanitizer: heap-use-after-free /root/perl/regexec.c:5440 |
From @tonycozOn Sun Oct 09 21:28:51 2016, brian.carpenter@gmail.com wrote:
Be careful running this case, it consumed a large amount of memory when I attempted to reproduce it under valgrind. Tony |
The RT System itself - Status changed from 'new' to 'open' |
From @hvdsThere are two parts to this, plus a lot of non-contributory garbage. The first part, which is triggering valgrind, is repeatedly doubling a variable in place while matching over it: my $len = 5; # eventual length $len * (2 ** $len) At some point the doubling causes a realloc, and the string we're matching over moves under us. It might be possible for us to spot this on return from an EVAL node, but I'm not sure what we'd want to do at that point - I doubt it's as simple as updating some pointers to point to the new buffer, though it might be reasonable to just panic() instead. It'd be kinda nice if we had a more generic solution that lets you flag a PV to say "hey, I'm looking at this buffer; if you should want to free it best mortalize it instead". The second part busts the stack: the string is actually many copies of '(xxx?' (where xxx is your gid, ${')'}; in my case it tries to become 2 ** 33 copies of a 33-byte string), and attempting to parse it adds 4 stack frames for every copy; if short enough it does eventually die with "Unmatched ( in regex ...", otherwise it either terminates "Out of memory!" or busts the stack and segfaults. My inclination is that both of these are primarily programmer problems rather than security issues, though I'd class both as nice-to-fix. (Umm, not sure though: if there's a possibility turning the first part into s/// could cause it to write into the freed buffer, that might be a bigger concern. It still needs the program to be stupid enough to do that though.) Hugo |
From @geeknikI have a smaller test case for this issue: perl -e '$q0=q/00|0()0|0()\1/;0=~$q0' ==19699==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000dfcf is located 1 bytes to the left of 10-byte region SUMMARY: AddressSanitizer: heap-buffer-overflow /root/perl/regexec.c:5440 ==12629== Invalid read of size 1 |
From @hvdsOn Fri Oct 14 18:21:08 2016, brian.carpenter@gmail.com wrote:
This is rather another case of #129377, and is fixed by my proposed patch there. Hugo |
From @tonycozOn Wed, 05 Oct 2016 05:03:45 -0700, hv wrote:
That looks sensible to me. Your patch seems to be from git show rather than git format-patch, which makes it harder to apply. Tony |
From @hvdsOn Tue, 17 Jan 2017 15:24:46 -0800, tonyc wrote:
Thanks Tony, now pushed as 2dfc11e; will move to open queue.
Sorry, it didn't occur to me someone might actually apply it. :) Hugo |
@hvds - Status changed from 'open' to 'pending release' |
From @tonycozOn Fri, 14 Oct 2016 18:21:08 -0700, brian.carpenter@gmail.com wrote:
Your original test case produced a stack overflow (deep recursion) for me before or after Hugo's 129377 patch in 2dfc11e. This test case is fixed by Hugo's patch. On Mon, 17 Oct 2016 04:02:43 -0700, hv wrote:
So merging this into 129377. Tony |
From @hvdsOn Tue, 24 Jan 2017 19:45:35 -0800, tonyc wrote:
I'm not sure merging was the right thing to do - the "another case of #129377" applies only to Brian's followup claim to have a smaller test case, not to the original report. That's fine though, I can create a clearer new ticket based on the first half of my original analysis - I think we already have ample references to the second (stackbusting) issue there. Hugo |
From @khwilliamsonThank you for filing this report. You have helped make Perl better. With the release today of Perl 5.26.0, this and 210 other issues have been Perl 5.26.0 may be downloaded via: If you find that the problem persists, feel free to reopen this ticket. |
@khwilliamson - Status changed from 'pending release' to 'resolved' |
Migrated from rt.perl.org#129377 (status was 'resolved')
Searchable as RT129377$
The text was updated successfully, but these errors were encountered: