global-buffer-overflow in Perl_foldEQ_utf8_flags (utf8.c:5278) #15796
Comments
From @geeknikTriggered with Perl v5.25.8-132-gc10193e. If it matters: od -tx1 test136 ./perl test136==18801==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000dfcd41 is located 0 bytes to the right of global variable '<string |
From @geeknik |
From @geeknikTriggered with Perl v5.25.8-132-gc10193e. If it matters: ./perl -e '/(?=00)0?$/il' ================================================================= 0x000000dfcd41 is located 0 bytes to the right of global variable '<string |
From @arcBrian Carpenter <perl5-security-report@perl.org> wrote:
It doesn't seem to.
Reduction: ./miniperl -e '/(?=\0a)b?$/iu' #130521 looks like the same bug, in that it reduces in the same way My hunch is that the culprit is re_intuit_start(), but I'm afraid I'd -- |
|
The RT System itself - Status changed from 'new' to 'open' |
From @hvdsOn Fri, 06 Jan 2017 04:22:00 -0800, arc@aaroncrane.co.uk wrote:
I can't reproduce the problem with the original code, nor with your reduced form, nor with #130521. Could you give a perl -V? When I trace through test136, the call from find_by_class() hits the EXACTFU case but then neither is_utf8_pat nor utf8_target, so it takes the do_exactf_non_utf8 branch rather than the do_exactf_utf8 from Brian's stack trace. Cheers, Hugo |
From @arcHugo van der Sanden via RT <perl5-security-report@perl.org> wrote:
See below. It also happens under -Mre=debug too; the output looks like this: $ ./perl -Ilib -Mre=debug -e '/(?=\0a)b?$/iu'
|
From @hvdsOn Sat, 07 Jan 2017 07:31:02 -0800, arc wrote:
Thanks, I'd managed to convince myself the stacktrace was showing the foldEQ_utf8_flags() call from the other branch, and failed to spot the 3 levels of indirection reaching the same call in the branch we're actually in. As you found, I think there are two separate bugs here, both at heart (I think) based on an assumption that the length of check substrs cannot exceed minlen - but, as here, we can get them from a lookahead. Karl: please check the second patch - I think when e < s we do not need to (and must not) check further; setting e = s causes us to try at least once, even if as in this case the string is empty. I suspect your comments in the removed chunks are intended to imply the erroneous assumption that the length of check substrs cannot exceed minlen, but I'm not sure why we additionally predicate that on reginfo->intuit.
I think we just need a byte that folds to itself, so I've made the testcases using digits. I was slightly worried that on a test run re/fold_grind timed out; I'm assuming that's because clang+sanitize=address is slow - disabling the watchdog allowed it to complete successfully in "386.33user 0.69system 6:27.79elapsed 99%CPU". Still waiting for the rest of the test run, and will then verify that the first of the new tests actually fails without the fixes. In any case, I don't see any significant security concern here - we'll read out of bounds, but I can't see that we'll write, nor that we'll give the wrong answer to the underlying pattern match - so I think this ticket can also be moved to the public queue. I'd welcome comments on that, or on the 3 proposed patches below. Hugo commit 9de31938f2d75d7cef537e4981da943d1d116b54 [perl #130522] do not allow endpos to exceed strend Inline Patchdiff --git a/regexec.c b/regexec.c
index 056a993..6a9da32 100644
--- a/regexec.c
+++ b/regexec.c
@@ -149,6 +149,7 @@ static const char* const non_utf8_target_but_utf8_required
#define HOP3lim(pos,off,lim) (reginfo->is_utf8_target \
? reghop3((U8*)(pos), off, (U8*)(lim)) \
: (U8*)((pos + off) > lim ? lim : (pos + off)))
+#define HOP3clim(pos,off,lim) ((char*)HOP3lim(pos,off,lim))
#define HOP4(pos,off,llim, rlim) (reginfo->is_utf8_target \
? reghop4((U8*)(pos), off, (U8*)(llim), (U8*)(rlim)) \
@@ -1291,10 +1292,10 @@ Perl_re_intuit_start(pTHX_
*/
if (prog->anchored_substr || prog->anchored_utf8 || ml_anch)
- endpos= HOP3c(rx_origin, (prog->minlen ? cl_l : 0), strend);
+ endpos = HOP3clim(rx_origin, (prog->minlen ? cl_l : 0), strend);
else if (prog->float_substr || prog->float_utf8) {
rx_max_float = HOP3c(check_at, -start_shift, strbeg);
- endpos= HOP3c(rx_max_float, cl_l, strend);
+ endpos = HOP3clim(rx_max_float, cl_l, strend);
}
else
endpos= strend;
[perl #130522] don't try to find_byclass outside the string Inline Patchdiff --git a/regexec.c b/regexec.c
index 6a9da32..7d2a3ac 100644
--- a/regexec.c
+++ b/regexec.c
@@ -1974,10 +1974,8 @@ S_find_byclass(pTHX_ regexp * prog, const regnode *c, char *s,
* trying that it will fail; so don't start a match past the
* required minimum number from the far end */
e = HOP3c(strend, -((SSize_t)ln), s);
-
- if (reginfo->intuit && e < s) {
- e = s; /* Due to minlen logic of intuit() */
- }
+ if (e < s)
+ break;
c1 = *pat_string;
c2 = fold_array[c1];
@@ -2021,10 +2019,6 @@ S_find_byclass(pTHX_ regexp * prog, const regnode *c, char *s,
*/
e = HOP3c(strend, -((SSize_t)lnc), s);
- if (reginfo->intuit && e < s) {
- e = s; /* Due to minlen logic of intuit() */
- }
-
/* XXX Note that we could recalculate e to stop the loop earlier,
* as the worst case expansion above will rarely be met, and as we
* go along we would usually find that e moves further to the left.
[perl #130522] test cases for len(STCLASS) > minlen Inline Patchdiff --git a/t/re/re_tests b/t/re/re_tests
index e8a7fa9..5491b49 100644
--- a/t/re/re_tests
+++ b/t/re/re_tests
@@ -1976,6 +1976,9 @@ AB\s+\x{100} AB \x{100}X y - -
(.*?(a(a)|i(i))n) riiaan y $2-$3-$4-$1 aa-a--riiaan # Jump trie capture buffer issue [perl #129897]
(^(?:(\d)x)?\d$) 1 y [$1-$2] [1-] # make sure that we reset capture buffers properly (from regtry)
(X{2,}[-X]{1,4}){3,}X{2,} XXX-XXX-XXX-- n - - # [perl #130307]
+/(?=01)0?1?$/iu n - - # [perl #130522] should not read outside string
+/(?=01)0?1?$/iu 0 n - - # [perl #130522]
+/(?=01)0?1?$/iu 01 y - - # [perl #130522]
# Keep these lines at the end of the file
# vim: softtabstop=0 noexpandtab |
From @hvdsOn Sun, 08 Jan 2017 08:26:20 -0800, hv wrote:
Test run with address=sanitized passed ok; however the new tests don't trigger the problem on blead - the closest I've found that does (looking for one that actually would match at least one string) is: However it only causes a problem if run against undef: an empty string is already safe. So I'll look to wrap that in a pat.t test. Hugo |
From @arcHugo van der Sanden via RT <perl5-security-report@perl.org> wrote:
Agreed. (Though I suspect we try to read a long way out of bounds for
As you note in your followup, making that change doesn't make asan I'm otherwise happy with your patch, though. -- |
From @arcHugo van der Sanden via RT <perl5-security-report@perl.org> wrote:
Just for the record: asan remains silent for me on that test case -- |
From @hvdsOn Mon, 09 Jan 2017 07:01:07 -0800, arc wrote:
Yes, copy/paste. I've just prepped the pat.t test, using qr{(?=\0z)\0?z?$}i. If Karl can confirm he's happy with the second patch, I'll transqueue and push. Hugo |
From @khwilliamsonOn Mon, 09 Jan 2017 08:52:25 -0800, hv wrote:
It seems correct to break immediately if we go back further than the beginning of the string. This code, marked as mine, was essentially copied from elsewhere in the file. I was puzzled by the test for reginfo->intuit, and so did some excavating. That was added in But even here, he was essentially just changing an existing variable name. I didn't look further back than that. |
From @hvdsNow pushed: commit 6785390 commit dda0191 commit 871e132 Hugo |
|
@hvds - Status changed from 'open' to 'pending release' |
From @hvdsMerging this to [perl #130522], as discussed there this is the same problem. Hugo |
|
The RT System itself - Status changed from 'new' to 'open' |
|
@hvds - 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#130522 (status was 'resolved')
Searchable as RT130522$
The text was updated successfully, but these errors were encountered: