Perl_re_op_compile(SV **const, int, OP *, const regexp_engine *, REGEXP *, _Bool *, U32, U32): Assertion `*(pRExC_state->end) == '\0'' failed (regcomp.c:7012) #15606
Comments
From @geeknikperl -e 'v300&O|0' triggers a heap-buffer-overflow in Perl_my_atof2 ==23567==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000e1ba is located 0 bytes to the right of 10-byte region SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 __interceptor_strlen |
From @tonycozOn Fri Aug 19 12:44:43 2016, brian.carpenter@gmail.com wrote:
Also produces uninitialized access warnings from valgrind: ==13615== Conditional jump or move depends on uninitialised value(s) The UTF-8 string path of bit-and in do_vop() doesn't NUL terminate The bit-or op then attempts to or the result of that and a number, so I couldn't work up a test for it that would fail without sanitize or valgrind. I can see a small chance of this being a security issue, eg. if an SV was re-used for the result of bit-and and that value was used in a context Tony |
From @tonycoz0001-perl-128998-NUL-terminate-the-result-of-bit-on-UTF-8.patchFrom bca1770e25759434e3e158f55ddcbf2cbe39c0a3 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 23 Aug 2016 10:46:32 +1000
Subject: [PATCH] (perl #128998) NUL terminate the result of bit & on UTF-8
strings
---
doop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/doop.c b/doop.c
index ad9172a..f9e308b 100644
--- a/doop.c
+++ b/doop.c
@@ -1093,6 +1093,7 @@ Perl_do_vop(pTHX_ I32 optype, SV *sv, SV *left, SV *right)
if (sv == left || sv == right)
(void)sv_usepvn(sv, dcorig, needlen);
SvCUR_set(sv, dc - dcorig);
+ *SvEND(sv) = '\0';
break;
case OP_BIT_XOR:
while (lulen && rulen) {
--
2.1.4
|
|
The RT System itself - Status changed from 'new' to 'open' |
From zefram@fysh.orgTony Cook via RT wrote:
With this being a problem here, there are going to be similar problems
On some, but not all, builds I can detect the problem thus: $ perl -lwe 'print length unpack("p", pack("p", v300 & "O"))' Devel::Peek shows lack of nul termination on all builds that I tried,
This problem does occur: $ perl -lwe ' open(FOO, ">", (v335 & "O"))' Unlikely, yes, but the semantics are notionally guaranteed. I think it -zefram |
From @dcollinsnHello! While fuzzing the a quadmath build of the perl interpreter, I encountered the following testcase: perl -e '0|v1000&E' This appears to be creating a string with a codepoint over 255. The output: $ ../bin/perl -e '0|v1000&E' Under Valgrind, with --track-origins=yes: Operator or semicolon missing before &E at -e line 1. And under GDB, using AFL's libdislocator to turn the invalid access into a SEGV: (gdb) run Program received signal SIGSEGV, Segmentation fault. So fold_constants is trying to reduce this, something is disappointed because the v-string uses codepoints over 255. Because this is a quadmath build, we take this branch: 1209 #ifdef USE_QUADMATH Now, that string /should/ just read "@". Breaking on Perl_my_atof2 without libdislocator loaded yields the following: Breakpoint 1, Perl_my_atof (s=0x122a200 "@") at numeric.c:1210 The "AAAAA" is an uninitialized poison pattern. The root cause *seems* to be that this string is not being null-terminated when it is created, presumably for the same reason as the deprecation warning. This is happening in Perl_do_vop. The case OP_BIT_AND around line 1077 does not append a null. There are three cases here: AND, OR, and XOR. OR falls through to mop_up_utf, which appends a NUL. XOR jumps to it. AND breaks from the case structure, skipping mop_up_utf. The patch which I am currently testing corrects this issue, eliminating the valgrind noise and the SEGV under libdislocator. I'm only waiting for a clean test run. A test is difficult to add since the only outwardly visible symptoms require that the memory allocated to the constant-folded SV be non-zeroed, or that valgrind be used. -- |
From @dcollinsnUpon further review, the AND case is deliberately different than the OR and XOR cases. I can tell because when I applied my patch that has the AND case just jump to mop_up_utf, the test suite failed a lot in t/op/bop.t. So instead I'll be a bit less heavy-handed and just pull in the line that NUL-terminates the string. This passes all tests (woo!). My question - there was a recent discussion about whether perl strings could be assumed to be null-terminated. The outcome, as I understand it, was that perl strings would usually be null-terminated, but XS code could produce SVs with unterminated strings. So while this patch fixes one issue by null-terminating this particular string, a question: Is it safe, in the general case, for Perl_my_atof2() to use strlen? If not, there are surely more similar bugs hiding out. (That discussion was in RT #128170) -- |
From @dcollinsn0001-RT-129058-Null-terminate-strings-after-utf8-bitwise-.patchFrom d92e756c7ac47f94845cd28b96f17a8afc89c436 Mon Sep 17 00:00:00 2001
From: Dan Collins <dcollinsn@gmail.com>
Date: Tue, 23 Aug 2016 13:18:47 -0400
Subject: [PATCH] [RT #129058] Null-terminate strings after utf8 bitwise and
The bug report pertains to the testcase:
perl -e '0|v1000&E'
The bitwise AND operation was not properly null-terminating the output
string. This is evidently fine for most cases, as the LEN and CUR
are properly set, but in the quadmath build where I discovered this
error, the subsequent bitwise OR with a number caused perl to call
Perl_my_atof2, which in turn calls strlen, on the constant string
'v1000'&'E'. This causes strlen to read uninitialized memory.
This patch adds a null-termination to the AND case in Perl_do_vop.
---
doop.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/doop.c b/doop.c
index ad9172a..f9e308b 100644
--- a/doop.c
+++ b/doop.c
@@ -1093,6 +1093,7 @@ Perl_do_vop(pTHX_ I32 optype, SV *sv, SV *left, SV *right)
if (sv == left || sv == right)
(void)sv_usepvn(sv, dcorig, needlen);
SvCUR_set(sv, dc - dcorig);
+ *SvEND(sv) = '\0';
break;
case OP_BIT_XOR:
while (lulen && rulen) {
--
2.8.1
|
From zefram@fysh.orgDan Collins via RT wrote:
There is an unresolved conflict between these two. It's kind-of accepted Obviously the two non-Postel behaviours (assuming nul termination on I think it is more workable to insist on nul termination. This seems To go the other way is also possible, but with more hassle. Although it -zefram |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozThis issue is now public in #129058, though the security implications On Mon Aug 22 19:00:31 2016, zefram@fysh.org wrote:
That "semi-approval" has always seemed questionable to me.
I tried writing a simple xsub to check for NUL termination, but I suspect Devel::Peek::Dump() shows the missing NUL, but it's implemented as a
Ok. Does it deserve a release, or just a published fix? (#129058 includes the Tony |
From @iabynOn Tue, Aug 23, 2016 at 06:15:07PM -0700, Tony Cook via RT wrote:
I think this is obscure enough to just push the fix and have a peldelta entry. But thinking further, there are two issues here: * Perl_do_vop() returned an unterminated string; fixed (but not pushed * Perl_atof and atof2 don't accept a length arg. This is arguably a design atof is only used in a few places in core. -- |
From @geeknikTriggered in v5.25.5 (v5.25.4-130-g7aa7bbc) w/ AFL + ASAN. od -tx1 assert78 ./perl assert78 |
From @geeknik |
From @dcollinsnOn Fri Sep 16 14:40:54 2016, brian.carpenter@gmail.com wrote:
==26698== Conditional jump or move depends on uninitialised value(s) (gdb) r Breakpoint 1, Perl_re_op_compile (patternp=<optimized out>, pat_count=<optimized out>, expr=<optimized out>, eng=0x6a80a0 <PL_core_reg_engine>, old_re=0x0, is_bare_re=<optimized out>, -- |
From [Unknown Contact. See original ticket]On Fri Sep 16 14:40:54 2016, brian.carpenter@gmail.com wrote:
==26698== Conditional jump or move depends on uninitialised value(s) (gdb) r Breakpoint 1, Perl_re_op_compile (patternp=<optimized out>, pat_count=<optimized out>, expr=<optimized out>, eng=0x6a80a0 <PL_core_reg_engine>, old_re=0x0, is_bare_re=<optimized out>, -- |
From @demerphqI looked into this a bit yesterday. The short version is that we end up creating an SVpv of length 1 which While this is documented as legal (we have a length field for a reason) the I have a fix to the regex engine to handle this right but so far I have IOW I can fix the assert but so far not the valgrind complaints about Yved On 16 Sep 2016 6:43 p.m., "Dan Collins via RT" <perlbug-comment@perl.org>
|
From @cpansproutOn Sat Sep 17 07:14:00 2016, demerphq wrote:
Wow. I had no idea this would happen: $ ./perl -Ilib -MDevel::Peek -e 'Dump "D" & "\0\x{500}"' (PV usually ends with "\0 and not just " by itself.) -- Father Chrysostomos |
|
The RT System itself - Status changed from 'new' to 'open' |
From @demerphqOn 17 Sep 2016 10:39 a.m., "Father Chrysostomos via RT" <
Pv's from perl normally do yes. However when I check the code it looks So we have two bugs here. The first is basically what you showed the Then there is the strange way that valgrind changes the behaviour of this Yves |
From @cpansproutOn Sat Sep 17 07:46:32 2016, demerphq wrote:
If we fix the first, then testing the second is harder. Maybe we need an XS::APItest::string_without_null(...) function that can be used in yet another .t file that runs re_tests. -- Father Chrysostomos |
From @demerphqOn 17 September 2016 at 19:05, Father Chrysostomos via RT
And maybe elsewhere too Yves -- |
From @demerphqOn 17 September 2016 at 16:46, demerphq <demerphq@gmail.com> wrote:
And as I suspected it does. A bit of digging reveals that if you use Yves |
From @cpansproutOn Sat Sep 17 10:05:50 2016, sprout wrote:
I have just added such a function, along with t/re/regexp_nonull.t, which uses it. -- Father Chrysostomos |
From @cpansproutOn Sat Sep 17 11:08:33 2016, sprout wrote:
(There are follow-up comments in the thread starting at <CANgJU+XSVs0c4EuHs0NmRk24rOybYj8WwjezZD6Aqrd+rdEuFQ@mail.gmail.com>.) -- Father Chrysostomos |
From @cpansproutOn Sat Sep 17 10:45:41 2016, demerphq wrote:
The second byte of &’s target was not being written to an any point after allocation, so it was uninitialized. This variant writes a long string to that target before doing a utf-8 bitwise and: ./perl -Ilib -XMfeature=:all -e 'for (["aaa","aaa"],[substr ("a\x{100}",0,1), "a"]) { use Devel::Peek; Dump $$_[0] &. $$_[1] }' It gives the same results under valgrind. So I have used that approach in the test added in commit b43665f, which fixes the bug. -- Father Chrysostomos |
From @demerphqOn 19 September 2016 at 05:21, Father Chrysostomos via RT
Cool. Nice work. Also I have merged your regexp_nonull.t to blead As far as I can tell we can close this ticket now. cheers, -- |
From @cpansproutOn Mon Sep 19 13:34:28 2016, demerphq wrote:
Thank you.
Done. -- Father Chrysostomos |
|
@cpansprout - Status changed from 'open' to 'pending release' |
From @xsawyerxOn Mon Sep 05 09:24:02 2016, davem wrote:
Should we assign a CVE number to this? |
From @geeknikAny word on a CVE assignment for this flaw? |
From @xsawyerxOn Mon, 28 Nov 2016 01:48:07 -0800, brian.carpenter@gmail.com wrote:
Considering the syntax *does* allow this (without specialized "pack"/"unpack" input), we should consider this a low-risk security issue. Does anyone object or has a different opinion? |
From @xsawyerxOn Mon, 28 Nov 2016 02:01:59 -0800, xsawyerx@cpan.org wrote:
After review again (and well, again...), I concluded this does not require a CVE number since the likelihood is exceptionally low. If no one objects, I now believe we should just resolve it, push a fix, and reflect it in perldelta. (Sorry for the flip flop.) |
From @tonycozOn Thu, 22 Dec 2016 06:21:28 -0800, xsawyerx@cpan.org wrote:
This turns out to be duplicated by both 129058 *and* 129287, the latter of which was fixed in b43665f and documented in perl5255delta.pod. I'll make this ticket public and merge both this and 129058 into 129287. Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
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#129287 (status was 'resolved')
Searchable as RT129287$
The text was updated successfully, but these errors were encountered: