Skip Menu |
Report information
Id: 129190
Status: resolved
Priority: 0/
Queue: perl5

Owner: Nobody
Requestors: dcollinsn [at] gmail.com
Cc:
AdminCc:

Operating System: (no value)
PatchStatus: (no value)
Severity: (no value)
Type: (no value)
Perl Version: (no value)
Fixed In: (no value)

Attachments
0001-perl-129190-intuit_method-can-move-the-line-buffer.patch



Date: Sat, 3 Sep 2016 21:44:57 -0400
To: perl5-security-report [...] perl.org
Subject: Multiple suspicious Valgrind errors
From: Dan Collins <dcollinsn [...] gmail.com>
Download (untitled) / with headers
text/plain 32.1k

Message body is not shown because it is too large.

Download (untitled) / with headers
text/html 39.9k

Message body is not shown because it is too large.

From: Dan Collins <dcollinsn [...] gmail.com>
Subject: Re: [perl #129190] AutoReply: Multiple suspicious Valgrind errors
To: perl5-security-report [...] perl.org
Date: Sat, 3 Sep 2016 22:16:08 -0400
Download (untitled) / with headers
text/plain 5.2k
AFL crash explorer reports that replacing "exec" with any of the following strings will also reproduce this error:

grep
pipe
getc
read
open
stat
seek
send
tell
bind
recv

Several similar cases involving the following strings were also identified:

flock
write
0stat
fcntl
printf
select
socket

in general, it appears that this is the repro case:

perl -e 'printf "%-7s_\$", "flock"' | valgrind ../bin/perl

In other words, exactly 7 characters consisting of a builtin rightpadded by spaces, followed by a literal '_$'. It seems important that the '$' be the 9th character exactly. The characters between the string and the '$' seem irrelevant. For example, we have 'exec(eq0$' as one of the fuzzer-generated testcases, and 'exec(pow$' as another.

This seems to be so tight that it's unlikely to be exploitable. I'll let it keep running, and update this thread if I find any cases that don't fit this pattern.



On Sat, Sep 3, 2016 at 9:45 PM, <perl5-security-report@perl.org> wrote:
Show quoted text
Greetings,

This message has been automatically generated in response to the
creation of a perl security report regarding:
   "Multiple suspicious Valgrind errors".

There is no need to reply to this message right now.  Your ticket has been
assigned an ID of [perl #129190].

Please include the string:

   [perl #129190]

in the subject line of all future correspondence about this issue. To do so,
you may reply to this message (please delete unnecessary quotes and text.)

  Thank you,
  perl5-security-report@perl.org

-------------------------------------------------------------------------
X-Virus-Checked: Checked
X-Virus-Checked: Checked
X-GM-Message-State: AE9vXwPLbRwR0qJ6jUO75cN8Cl1JqPOH8PVIGaBhrB621e7mxTME+eY8bRi+EZTUWMWN+IS06a/ZsIqwENn3iw==
X-Old-Spam-Check-BY: la.mx.develooper.com
MIME-Version: 1.0
X-Received: by 10.36.16.138 with SMTP id 132mr14576013ity.60.1472953518162; Sat, 03 Sep 2016 18:45:18 -0700 (PDT)
Return-Path: <perlmail@x6.develooper.com>
Date: Sat, 3 Sep 2016 21:44:57 -0400
To: perl5-security-report@perl.org
Subject: Multiple suspicious Valgrind errors
Received: (qmail 2194 invoked from network); 4 Sep 2016 01:45:42 -0000
Received: from localhost (HELO la.mx.develooper.com) (127.0.0.1) by localhost with SMTP; 4 Sep 2016 01:45:42 -0000
Received: (qmail 2191 invoked by alias); 4 Sep 2016 01:45:42 -0000
Received: from x6.develooper.com (HELO x6.develooper.com) (207.171.7.86) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Sat, 03 Sep 2016 18:45:35 -0700
Received: by x6.develooper.com (Postfix, from userid 514) id 8CA381EA4; Sat,  3 Sep 2016 18:45:31 -0700 (PDT)
Received: (qmail 18725 invoked from network); 4 Sep 2016 01:45:31 -0000
Received: from x1.develooper.com (207.171.7.70) by x6.develooper.com with SMTP; 4 Sep 2016 01:45:31 -0000
Received: (qmail 2184 invoked by uid 225); 4 Sep 2016 01:45:30 -0000
Received: (qmail 2180 invoked by alias); 4 Sep 2016 01:45:30 -0000
Received: from mail-it0-f48.google.com (HELO mail-it0-f48.google.com) (209.85.214.48) by la.mx.develooper.com (qpsmtpd/0.28) with ESMTP; Sat, 03 Sep 2016 18:45:22 -0700
Received: by mail-it0-f48.google.com with SMTP id c198so99792236ith.1 for <perl5-security-report@perl.org>; Sat, 03 Sep 2016 18:45:22 -0700 (PDT)
Received: by 10.36.196.215 with HTTP; Sat, 3 Sep 2016 18:44:57 -0700 (PDT)
X-Spam-Check-BY: la.mx.develooper.com
X-Google-Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=3rjGLiHGpWkRVqoYmrTJ0T9qeVXebCN0rIvSOel07IU=; b=axBRm89wSuCtb0AP0pqda2o7/lvg1M8Qyt6NTqhcsIjeMHumq4PlEyfouyzhHsLYq9 yPZVt3aonaI9i+kHVE/248wCKtOqYXCvlVrNDmx0JCfQSZxGR/yUaW9rkPExJb1iiMKU rVaF+UIEW2nUKA+1owPFrKuLUcoew/sGlk9rERu9vfT/4ImcsuQKvL535xuYb6YxSLp4 pVAm2lnO2b6pIxEEs8gnW09XBRs8t7o+kbPOY2zLdAtjv52AOicULp09DwPKOKRB20Uy RGFyP+FukSxJce8b3KliwyasQQp5eONUYF3L2K+gXevHbH005lpsGUDTUwI3/q3CdWM6 hhMw==
Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:from:date:message-id:subject:to; bh=3rjGLiHGpWkRVqoYmrTJ0T9qeVXebCN0rIvSOel07IU=; b=zK6by3wH4+dfjHpb5MigKK2wRTiWM0c2wXOnjjBCjIOXTzSxIH9+mvDbn71KkZI8xt muUpXZYpWMa4SSg+3vNiJP/Ooo2E8zNUwB/7L02jsDTlV2QBEg6J+ktokzJg0tcHI7+M d6NpQPgNTDpRXOc2rUdYj5Fw3KrvLV7C3W1Pt4Mt2oqf18IooQE6E1QB7tn4OewC3fn7 ECUUmAkUflNLb215sqkN80Qlc7/VCWE2HNZqssAl72+PJ6AtNK5FPLS0hkFDQeBNfKlE GVJFfAZ4QlhlNzdmgChC8xJyzPUtHMfstFyk5aeL61pKCrFidsTyzexv1W64Zug1MxTW x2mA==
X-Old-Spam-Status: No, hits=-2.7 required=8.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS
Message-ID: <CA+tt54KnB50wyKJ_q7YhuRrTLd7=FpWBfbZSqDy0Gg2Mz9vQSA@mail.gmail.com>
From: Dan Collins <dcollinsn@gmail.com>
From perlmail@x6.develooper.com Sun Sep 04 01:45:43 2016
Content-Type: multipart/alternative; boundary="001a114382e8388b49053ba4b7bb"
X-Spam-Status: No, hits=-8.5 required=8.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM,RP_MATCHES_RCVD
Delivered-To: rt-perl5-security@rt.perl.org
Delivered-To: perlmail-perl5-security-report@onion.perl.org
Delivered-To: perl5-security-report@perl.org
X-RT-Interface: Email

RT-Send-CC: rt-deliver-to-perl5-security-report [...] rt.perl.org
Download (untitled) / with headers
text/plain 958b
On Sat Sep 03 21:26:50 2016, dcollinsn@gmail.com wrote: Show quoted text
> AFL crash explorer reports that replacing "exec" with any of the > following > strings will also reproduce this error: >
... Show quoted text
> > Several similar cases involving the following strings were also > identified: > > flock > write > 0stat > fcntl > printf > select > socket > > in general, it appears that this is the repro case: > > perl -e 'printf "%-7s_\$", "flock"' | valgrind ../bin/perl > > In other words, exactly 7 characters consisting of a builtin > rightpadded by > spaces, followed by a literal '_$'. It seems important that the '$' be > the > 9th character exactly. The characters between the string and the '$' > seem > irrelevant. For example, we have 'exec(eq0$' as one of the fuzzer- > generated > testcases, and 'exec(pow$' as another.
Does the attached fix all your test cases for this? As this involves feeding code to the perl parser, I don't think it's a security issue. Tony
Subject: 0001-perl-129190-intuit_method-can-move-the-line-buffer.patch
From e36eaa0b2f687d532fe3b2f0b0bbded8e8a1fa17 Mon Sep 17 00:00:00 2001 From: Tony Cook <tony@develop-help.com> Date: Thu, 8 Sep 2016 13:21:02 +1000 Subject: (perl #129190) intuit_method() can move the line buffer and broke PL_bufptr when it did. --- t/op/lex.t | 5 ++++- toke.c | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/t/op/lex.t b/t/op/lex.t index a667183..6eac888 100644 --- a/t/op/lex.t +++ b/t/op/lex.t @@ -7,7 +7,7 @@ use warnings; BEGIN { chdir 't' if -d 't'; require './test.pl'; } -plan(tests => 30); +plan(tests => 31); { no warnings 'deprecated'; @@ -241,3 +241,6 @@ fresh_perl_is( {}, '[perl #129069] - "Missing name" warning and valgrind clean' ); + +fresh_perl_like('flock _$', qr/Not enough arguments for flock/, {stderr => 1}, + "[perl #129190] intuit_method() invalidates PL_bufptr"); diff --git a/toke.c b/toke.c index 3ade32b..3779387 100644 --- a/toke.c +++ b/toke.c @@ -4079,11 +4079,12 @@ S_intuit_method(pTHX_ char *start, SV *ioname, CV *cv) } if (*start == '$') { + SSize_t start_off = start - SvPVX(PL_linestr); if (cv || PL_last_lop_op == OP_PRINT || PL_last_lop_op == OP_SAY || isUPPER(*PL_tokenbuf)) return 0; s = skipspace(s); - PL_bufptr = start; + PL_bufptr = SvPVX(PL_linestr) + start_off; PL_expect = XREF; return *s == '(' ? FUNCMETH : METHOD; } @@ -7034,17 +7035,24 @@ Perl_yylex(pTHX) == OA_FILEREF)) { bool immediate_paren = *s == '('; + SSize_t s_off; /* (Now we can afford to cross potential line boundary.) */ s = skipspace(s); /* Two barewords in a row may indicate method call. */ + /* intuit_method() can indirectly call lex_next_chunk(), + * invalidating s + */ + s_off = s - SvPVX(PL_linestr); if ((isIDFIRST_lazy_if(s,UTF) || *s == '$') && (tmp = intuit_method(s, lex ? NULL : sv, cv))) { + /* the code at method: doesn't use s */ goto method; } + s = SvPVX(PL_linestr) + s_off; /* If not a declared subroutine, it's an indirect object. */ /* (But it's an indir obj regardless for sort.) */ -- 2.1.4
Date: Sun, 11 Sep 2016 01:02:44 -0400
To: perl5-security-report [...] perl.org
Subject: Re: [perl #129190] Multiple suspicious Valgrind errors
From: Dan Collins <dcollinsn [...] gmail.com>
Download (untitled) / with headers
text/plain 3.8k
Sorry for the delay in responding to this. Yes, Tony, the patch you attached fixes my testcases.

On Wed, Sep 7, 2016 at 11:23 PM, Tony Cook via RT <perl5-security-report@perl.org> wrote:
Show quoted text
On Sat Sep 03 21:26:50 2016, dcollinsn@gmail.com wrote:
> AFL crash explorer reports that replacing "exec" with any of the
> following
> strings will also reproduce this error:
>
...
>
> Several similar cases involving the following strings were also
> identified:
>
> flock
> write
> 0stat
> fcntl
> printf
> select
> socket
>
> in general, it appears that this is the repro case:
>
> perl -e 'printf "%-7s_\$", "flock"' | valgrind ../bin/perl
>
> In other words, exactly 7 characters consisting of a builtin
> rightpadded by
> spaces, followed by a literal '_$'. It seems important that the '$' be
> the
> 9th character exactly. The characters between the string and the '$'
> seem
> irrelevant. For example, we have 'exec(eq0$' as one of the fuzzer-
> generated
> testcases, and 'exec(pow$' as another.

Does the attached fix all your test cases for this?

As this involves feeding code to the perl parser, I don't think it's
a security issue.

Tony

From e36eaa0b2f687d532fe3b2f0b0bbded8e8a1fa17 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 8 Sep 2016 13:21:02 +1000
Subject: (perl #129190) intuit_method() can move the line buffer

and broke PL_bufptr when it did.
---
 t/op/lex.t |  5 ++++-
 toke.c     | 10 +++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/t/op/lex.t b/t/op/lex.t
index a667183..6eac888 100644
--- a/t/op/lex.t
+++ b/t/op/lex.t
@@ -7,7 +7,7 @@ use warnings;

 BEGIN { chdir 't' if -d 't'; require './test.pl'; }

-plan(tests => 30);
+plan(tests => 31);

 {
     no warnings 'deprecated';
@@ -241,3 +241,6 @@ fresh_perl_is(
     {},
     '[perl #129069] - "Missing name" warning and valgrind clean'
 );
+
+fresh_perl_like('flock  _$', qr/Not enough arguments for flock/, {stderr => 1},
+                "[perl #129190] intuit_method() invalidates PL_bufptr");
diff --git a/toke.c b/toke.c
index 3ade32b..3779387 100644
--- a/toke.c
+++ b/toke.c
@@ -4079,11 +4079,12 @@ S_intuit_method(pTHX_ char *start, SV *ioname, CV *cv)
     }

     if (*start == '$') {
+        SSize_t start_off = start - SvPVX(PL_linestr);
        if (cv || PL_last_lop_op == OP_PRINT || PL_last_lop_op == OP_SAY
             || isUPPER(*PL_tokenbuf))
            return 0;
        s = skipspace(s);
-       PL_bufptr = start;
+       PL_bufptr = SvPVX(PL_linestr) + start_off;
        PL_expect = XREF;
        return *s == '(' ? FUNCMETH : METHOD;
     }
@@ -7034,17 +7035,24 @@ Perl_yylex(pTHX)
                                                                == OA_FILEREF))
                {
                    bool immediate_paren = *s == '(';
+                    SSize_t s_off;

                    /* (Now we can afford to cross potential line boundary.) */
                    s = skipspace(s);

                    /* Two barewords in a row may indicate method call. */

+                    /* intuit_method() can indirectly call lex_next_chunk(),
+                     * invalidating s
+                     */
+                    s_off = s - SvPVX(PL_linestr);
                    if ((isIDFIRST_lazy_if(s,UTF) || *s == '$')
                         && (tmp = intuit_method(s, lex ? NULL : sv, cv)))
                     {
+                        /* the code at method: doesn't use s */
                        goto method;
                    }
+                    s = SvPVX(PL_linestr) + s_off;

                    /* If not a declared subroutine, it's an indirect object. */
                    /* (But it's an indir obj regardless for sort.) */
--
2.1.4



To: Dan Collins <dcollinsn [...] gmail.com>
Date: Mon, 12 Dec 2016 15:54:06 +0000
From: Dave Mitchell <davem [...] iabyn.com>
CC: perl5-security-report [...] perl.org
Subject: Re: [perl #129190] Multiple suspicious Valgrind errors
Download (untitled) / with headers
text/plain 334b
On Sun, Sep 11, 2016 at 01:02:44AM -0400, Dan Collins wrote: Show quoted text
> Sorry for the delay in responding to this. Yes, Tony, the patch you > attached fixes my testcases.
Tony, any particular reason you haven't applied your patch yet? -- But Pity stayed his hand. "It's a pity I've run out of bullets", he thought. -- "Bored of the Rings"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 479b
On Mon, 12 Dec 2016 07:54:39 -0800, davem wrote: Show quoted text
> On Sun, Sep 11, 2016 at 01:02:44AM -0400, Dan Collins wrote:
> > Sorry for the delay in responding to this. Yes, Tony, the patch you > > attached fixes my testcases.
> > Tony, any particular reason you haven't applied your patch yet?
I lost track of it. Applied as 743e3e72117ab1d168cbf4ef15bcde67ca41e26a (with some noise.) Since this isn't a security issue, the ticket is now public, and closed since it's patched. Tony
Download (untitled) / with headers
text/plain 313b
Thank 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 resolved. Perl 5.26.0 may be downloaded via: https://metacpan.org/release/XSAWYERX/perl-5.26.0 If you find that the problem persists, feel free to reopen this ticket.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

For issues related to this RT instance (aka "perlbug"), please contact perlbug-admin at perl.org