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 S_regmatch (regexec.c:7476) #15602
Comments
From @dcollinsnDetected using AFL and libdislocator, but reproducible with valgrind and an uninstrumented perl. The following regex causes a length-1 buffer to be allocated, but the second element of that buffer to be read, at two different points in S_regmatch. The buffer in question is reginfo->info_aux->poscache: perl -e '/0*()*(||0(?0))^*0^+|(?0)(?0)/' The only visible output of this script is the message: But under debugging tools: %% VALGRIND %% ==40309== Invalid read of size 1 %% GDB SESSION %% Starting program: /home/dcollins/toolchain/perl/perl allcrash/f3i000025 Breakpoint 3, S_regmatch (reginfo=0x7fffffffe070, startpos=0x80255f "", Breakpoint 2, S_regmatch (reginfo=0x7fffffffe070, startpos=0x80255f "", Breakpoint 1, S_regmatch (reginfo=0x7fffffffe070, startpos=0x80255f "", Breakpoint 4, S_regmatch (reginfo=0x7fffffffe070, startpos=0x80255f "", -- |
From @cpansproutOn Tue Aug 23 19:48:57 2016, dcollinsn@gmail.com wrote:
Infinite recursion in regex at -e line 1. -- Father Chrysostomos |
The RT System itself - Status changed from 'new' to 'open' |
From @geeknikv5.25.5 (v5.25.4-130-g7aa7bbc) + AFL + ASAN + libdislocator perl -e '$0=/0(?0)|^*0(?0)|^*(^*())0|/' ==26041==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000e0f1 is located 0 bytes to the right of 1-byte region SUMMARY: AddressSanitizer: heap-buffer-overflow /root/perl/regexec.c:7476 |
From @hvdsOn Tue Aug 23 19:48:57 2016, dcollinsn@gmail.com wrote:
The docs added by davem in 1cb95af say: * The top 4 bits of scan->flags byte say how many different .. and the code certainly appears to think that, using the top 4 bits to decide the size of poscache to malloc, and the bottom 4 bits to decide the index to look at. The invalid read occurs here because scan->flags == 0x3b, so something is violating that expectation; I don't yet know whether the expectation was wrong in the first place or something is abusing it. Hugo |
From @hvdsThe prime cause here is that the handling of S_study_chunk() for CURLYX assumes it will be called only once for each node, but that is not the case. It attempts to use that assumption to record in the corresponding WHILEM for each CURLYX that it is the m'th of n such (whilem_c of whilem_seen); the n is later used to determine the size of poscache (the super-linear detection cache) while the m is used to determine the offset within poscache to look at. When the CURLYX are visited more than once, whilem_c is incremented and stored in the WHILEM node each time, and will end up exceeding whilem_seen; the result is that we later address past the end of poscache. You can see it in -Dr output; it should look like this: For the security concern: the bit-offset in poscache is calculated as: The attached patch is a proof of concept: I've no idea how to test this, and would like to be able to; I'm also not sure if the solution is the ideal one - study_chunk() does not seem like the ideal place to do this work, and scan->flags does not seem like the ideal place to store the information - but it is at least simple. (Adding just the new assert() suggests that no existing tests exercise this.) Hugo |
From @hvds |
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Wed Oct 05 03:34:18 2016, hv wrote:
The usual way to test for this type of bug is to add a test that we expect to So for this issue, something like the attached. Without your fix, run with: cd t the test fails with: re/pat.t .. 800/800 # Failed test 800 - don't bump whilem_c too much at ./test.pl line 1058
As to whether this is a security issue, we're overflowing the end of the buffer, and if we expect regexps to be secure even when the regexps are untrusted strings*, this may at least be a denial of service, since it might crash the interpreter. Depending on the system we might be overwriting the beginning of the next allocated block, maybe allowing a more useful attack. Tony * we don't require regexps to be untainted for example. |
From @tonycozOn Sun Oct 09 17:35:33 2016, tonyc wrote:
Really attached this time. Tony |
From @tonycoz0001-perl-129281-test-for-buffer-overflow-issue.patchFrom 46a40ab3bc159c4cb7a72fed5bded45a43400d5c Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Mon, 10 Oct 2016 10:46:46 +1100
Subject: (perl #129281) test for buffer overflow issue
---
t/re/pat.t | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/t/re/pat.t b/t/re/pat.t
index d0449e2..774fa91 100644
--- a/t/re/pat.t
+++ b/t/re/pat.t
@@ -23,7 +23,7 @@ BEGIN {
skip_all('no re module') unless defined &DynaLoader::boot_DynaLoader;
skip_all_without_unicode_tables();
-plan tests => 799; # Update this when adding/deleting tests.
+plan tests => 800; # Update this when adding/deleting tests.
run_tests() unless caller;
@@ -1796,6 +1796,12 @@ EOP
like("sS", qr/\N{}Ss|/i, "\N{} with empty branch alternation works");
}
+ {
+ # [perl #129281] buffer write overflow, detected by ASAN, valgrind
+ local $::TODO = "whilem_c bumped too much";
+ fresh_perl_is('/0(?0)|^*0(?0)|^*(^*())0|/', '', {}, "don't bump whilem_c too much");
+ }
+
} # End of sub run_tests
1;
--
2.1.4
|
From @iabynOn Sun, Oct 09, 2016 at 05:35:33PM -0700, Tony Cook via RT wrote:
I think there's already enough mischief you can you in perl by running The worst it can do is set a bit in the bytes (or 2 bytes??) following the But these two things occur only if the WHILEM node with the dodgy index All in all, I think it's a bit of a stretch and we should go ahead and I can't comment on the correctness of Hugo's fix, as the regex compiler -- |
From @hvdsOn Sat, 04 Feb 2017 09:28:17 -0800, davem wrote:
Ok, unless someone says they want to do more review or raise other concerns, I'll aim to re-test and push on Monday.
How bold. :p Hugo |
@hvds - Status changed from 'open' to 'pending release' |
From @hvdsOn Mon, 06 Feb 2017 07:17:31 -0800, hv wrote:
Er, #129061 that is. |
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#129281 (status was 'resolved')
Searchable as RT129281$
The text was updated successfully, but these errors were encountered: