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

Owner: Nobody
Requestors: TODDR <toddr [at] cpan.org>
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: low
Type: core
Perl Version: 5.22.1
Fixed In: (no value)

Attachments
0001-Prevent-Perl_gv_fetchmethod_pvn_flags-from-running-p.patch



Subject: Possible string overrun with invalid len in gv.c
From: Todd Rinaldo <toddr [...] cpan.org>
To: perlbug [...] perl.org
Date: Tue, 13 Sep 2016 13:18:17 -0500
This is a bug report for perl from toddr@cpan.org, generated with the help of perlbug 1.40 running under perl 5.22.1. ----------------------------------------------------------------- [Please describe your issue here] We ran across this issue during B::C development. It turns out that if the wrong len (too short OR too long) is passed into Perl_gv_fetchmethod_pvn_flags, then you end up with a seg fault when it eventually hits memory it doesn't own. I am not saying there is any known problem in Perl, but IMO this should still be cleaned up. for (nend = name; *nend || nend != (origname + len); nend++) { if (*nend == '\'') { nsplit = nend; name = nend + 1; } else if (*nend == ':' && *(nend + 1) == ':') { nsplit = nend++; name = nend + 1; } } I did not search for other examples. I thought it might be best to open a discussion before proceeding on any work. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=low --- Site configuration information for perl 5.22.1: Configured by cPanel at Sun Aug 28 23:44:24 CDT 2016. Summary of my perl5 (revision 5 version 22 subversion 1) configuration: Platform: osname=linux, osvers=3.10.0-123.20.1.el7.x86_64, archname=x86_64-linux-64int uname='linux rpmbuild-64-centos-7.dev.cpanel.net 3.10.0-123.20.1.el7.x86_64 #1 smp thu jan 29 18:05:33 utc 2015 x86_64 x86_64 x86_64 gnulinux ' config_args='-des -Dusedevel -Darchname=x86_64-linux-64int -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -DDEBUGGING=none -Doptimize=-Os -Dusemymalloc=n -Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dccflags=-DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib64 -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib64 -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/lib64 -Dprefix=/usr/local/cpanel/3rdparty/perl/522 -Dsiteprefix=/opt/cpanel/perl5/522 -Dsitebin=/opt/cpanel/perl5/522/bin -Dsitelib=/opt/cpanel/perl5/522/site_lib -Dusevendorprefix=true -Dvendorbin=/usr/local/cpanel/3rdparty/perl/522/bin -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5 -Dvendorlib=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib -Dprivlib=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1 -Dman1dir=none -Dman3dir=none -Dscriptdir=/usr/local/cpanel/3rdparty/perl/522/bin -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/522/bin -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@localhost -Dcf_email=support@cpanel.net -Di_dbm=/usr/local/cpanel/3rdparty/include -Di_gdbm=/usr/local/cpanel/3rdparty/include -Di_ndbm=/usr/local/cpanel/3rdparty/include -DDB_File=true -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Aldflags=-L/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm -Dlocincpth=/usr/local/cpanel/3rdparty/perl/522/include /usr/local/cpanel/3rdparty/include /usr/local/include -Duse64bitint -Uuse64bitall -Acflags=-fPIC -DPIC -m64 -I/usr/local/cpanel/3rdparty/perl/522/include /usr/local/cpanel/3rdparty/include -Dlibpth=/usr/local/cpanel/3rdparty/perl/522/lib64 /usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64 ' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=undef, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='/usr/bin/gcc', ccflags ='-DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib64 -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2', optimize='-Os', cppflags='-I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib64 -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -DPERL_DISABLE_PMC -I/usr/local/cpanel/3rdparty/perl/522/include -L/usr/local/cpanel/3rdparty/perl/522/lib64 -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib64 -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='', gccversion='4.8.2 20140120 (Red Hat 4.8.2-16)', gccosandvers='' intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3 ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=8, prototype=define Linker and Libraries: ld='/usr/bin/gcc', ldflags ='-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/lib64 -L/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm -fstack-protector-strong -L/usr/local/lib' libpth=/usr/local/cpanel/3rdparty/perl/522/lib64 /usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib /lib64 /usr/lib64 /usr/local/lib /usr/lib /lib/../lib64 /usr/lib/../lib64 /lib libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=libc-2.17.so, so=so, useshrplib=true, libperl=libperl.so gnulibc_version='2.17' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int/CORE' cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/cpanel/3rdparty/perl/522/lib64 -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -L/usr/local/lib -fstack-protector-strong' Locally applied patches: cPanel patches cPanel INC path changes Remove . from @INC --- @INC for perl 5.22.1: /usr/local/cpanel /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib/x86_64-linux-64int /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1 /opt/cpanel/perl5/522/site_lib/x86_64-linux-64int /opt/cpanel/perl5/522/site_lib --- Environment for perl 5.22.1: HOME=/root LANG=en_US.UTF-8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/cpanel/perl5/524/bin PERL_BADLANG (unset) SHELL=/bin/zsh
Date: Tue, 13 Sep 2016 20:25:44 +0200
To: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
From: demerphq <demerphq [...] gmail.com>
Download (untitled) / with headers
text/plain 7.7k

On 13 Sep 2016 14:19, "Todd Rinaldo" <perlbug-followup@perl.org> wrote:
Show quoted text

>
> # New Ticket Created by  "Todd Rinaldo"
> # Please include the string:  [perl #129267]
> # in the subject line of all future correspondence about this issue.
> # <URL: https://rt.perl.org/Ticket/Display.html?id=129267 >
>
>
> This is a bug report for perl from toddr@cpan.org,
> generated with the help of perlbug 1.40 running under perl 5.22.1.
>
>
> -----------------------------------------------------------------
> [Please describe your issue here]
>
>
> We ran across this issue during B::C development. It turns out that if
> the wrong len (too short OR too long) is passed into
> Perl_gv_fetchmethod_pvn_flags, then you end up with a seg fault when
> it eventually hits memory it doesn't own. I am not saying there is any
> known problem in Perl, but IMO this should still be cleaned up.

I agree.  It looks wrong in several ways.

>   for (nend = name; *nend || nend != (origname + len); nend++) {
Show quoted text

>     if (*nend == '\'') {
>       nsplit = nend;
>       name = nend + 1;
>     }
>     else if (*nend == ':' && *(nend + 1) == ':') {
>       nsplit = nend++;
>       name = nend + 1;
>     }
>   }
>
> I did not search for other examples. I thought it might be best to
> open a discussion before proceeding on any work.

Not sure what there is to discuss really.  Wrong is wrong. ☺️

Yves
Show quoted text

> [Please do not change anything below this line]
> -----------------------------------------------------------------
> ---
> Flags:
>     category=core
>     severity=low
> ---
> Site configuration information for perl 5.22.1:
>
> Configured by cPanel at Sun Aug 28 23:44:24 CDT 2016.
>
> Summary of my perl5 (revision 5 version 22 subversion 1) configuration:
>
>   Platform:
>     osname=linux, osvers=3.10.0-123.20.1.el7.x86_64, archname=x86_64-linux-64int
>     uname='linux rpmbuild-64-centos-7.dev.cpanel.net
> 3.10.0-123.20.1.el7.x86_64 #1 smp thu jan 29 18:05:33 utc 2015 x86_64
> x86_64 x86_64 gnulinux '
>     config_args='-des -Dusedevel -Darchname=x86_64-linux-64int
> -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -DDEBUGGING=none -Doptimize=-Os
> -Dusemymalloc=n -Duseshrplib -Duselargefiles=yes -Duseposix=true
> -Dhint=recommended -Duseperlio=yes -Dccflags=-DPERL_DISABLE_PMC
> -I/usr/local/cpanel/3rdparty/perl/522/include
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -I/usr/local/cpanel/3rdparty/include
> -L/usr/local/cpanel/3rdparty/lib64
> -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/522/include
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -I/usr/local/cpanel/3rdparty/include
> -L/usr/local/cpanel/3rdparty/lib64 -Dldflags=-Wl,-rpath
> -Wl,/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/lib64
> -Dprefix=/usr/local/cpanel/3rdparty/perl/522
> -Dsiteprefix=/opt/cpanel/perl5/522 -Dsitebin=/opt/cpanel/perl5/522/bin
> -Dsitelib=/opt/cpanel/perl5/522/site_lib -Dusevendorprefix=true
> -Dvendorbin=/usr/local/cpanel/3rdparty/perl/522/bin
> -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5
> -Dvendorlib=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib
> -Dprivlib=/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1
> -Dman1dir=none -Dman3dir=none
> -Dscriptdir=/usr/local/cpanel/3rdparty/perl/522/bin
> -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/522/bin
> -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none
> -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel
> -Dmyhostname=localhost -Dperladmin=root@localhost
> -Dcf_email=support@cpanel.net
> -Di_dbm=/usr/local/cpanel/3rdparty/include
> -Di_gdbm=/usr/local/cpanel/3rdparty/include
> -Di_ndbm=/usr/local/cpanel/3rdparty/include -DDB_File=true -Ud_dosuid
> -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks
> -Uuselongdouble -Aldflags=-L/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm
> -Dlocincpth=/usr/local/cpanel/3rdparty/perl/522/include
> /usr/local/cpanel/3rdparty/include /usr/local/include  -Duse64bitint
> -Uuse64bitall -Acflags=-fPIC -DPIC -m64
> -I/usr/local/cpanel/3rdparty/perl/522/include
> /usr/local/cpanel/3rdparty/include
> -Dlibpth=/usr/local/cpanel/3rdparty/perl/522/lib64
> /usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib
> /lib64 /usr/lib64 '
>     hint=recommended, useposix=true, d_sigaction=define
>     useithreads=undef, usemultiplicity=undef
>     use64bitint=define, use64bitall=undef, uselongdouble=undef
>     usemymalloc=n, bincompat5005=undef
>   Compiler:
>     cc='/usr/bin/gcc', ccflags ='-DPERL_DISABLE_PMC
> -I/usr/local/cpanel/3rdparty/perl/522/include
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -I/usr/local/cpanel/3rdparty/include
> -L/usr/local/cpanel/3rdparty/lib64 -fwrapv -fno-strict-aliasing -pipe
> -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
> -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2',
>     optimize='-Os',
>     cppflags='-I/usr/local/cpanel/3rdparty/perl/522/include
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -I/usr/local/cpanel/3rdparty/include
> -L/usr/local/cpanel/3rdparty/lib64 -DPERL_DISABLE_PMC
> -I/usr/local/cpanel/3rdparty/perl/522/include
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -I/usr/local/cpanel/3rdparty/include
> -L/usr/local/cpanel/3rdparty/lib64 -fwrapv -fno-strict-aliasing -pipe
> -fstack-protector-strong -I/usr/local/include'
>     ccversion='', gccversion='4.8.2 20140120 (Red Hat 4.8.2-16)',
> gccosandvers=''
>     intsize=4, longsize=8, ptrsize=8, doublesize=8,
> byteorder=12345678, doublekind=3
>     d_longlong=define, longlongsize=8, d_longdbl=define,
> longdblsize=16, longdblkind=3
>     ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t',
> lseeksize=8
>     alignbytes=8, prototype=define
>   Linker and Libraries:
>     ld='/usr/bin/gcc', ldflags ='-Wl,-rpath
> -Wl,/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/lib64
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64 -lgdbm
> -fstack-protector-strong -L/usr/local/lib'
>     libpth=/usr/local/cpanel/3rdparty/perl/522/lib64
> /usr/local/cpanel/3rdparty/lib64 /usr/local/lib64 /usr/local/lib
> /lib64 /usr/lib64 /usr/local/lib /usr/lib /lib/../lib64
> /usr/lib/../lib64 /lib
>     libs=-lpthread -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc -lgdbm_compat
>     perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
>     libc=libc-2.17.so, so=so, useshrplib=true, libperl=libperl.so
>     gnulibc_version='2.17'
>   Dynamic Linking:
>     dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E
> -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int/CORE'
>     cccdlflags='-fPIC', lddlflags='-shared -Os
> -L/usr/local/cpanel/3rdparty/perl/522/lib64
> -L/usr/local/cpanel/3rdparty/lib64 -L/usr/lib64 -L/lib64
> -L/usr/local/lib -fstack-protector-strong'
>
> Locally applied patches:
>     cPanel patches
>     cPanel INC path changes
>     Remove . from @INC
>
> ---
> @INC for perl 5.22.1:
>     /usr/local/cpanel
>     /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib/x86_64-linux-64int
>     /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/cpanel_lib
>     /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1/x86_64-linux-64int
>     /usr/local/cpanel/3rdparty/perl/522/lib64/perl5/5.22.1
>     /opt/cpanel/perl5/522/site_lib/x86_64-linux-64int
>     /opt/cpanel/perl5/522/site_lib
>
> ---
> Environment for perl 5.22.1:
>     HOME=/root
>     LANG=en_US.UTF-8
>     LANGUAGE (unset)
>     LD_LIBRARY_PATH (unset)
>     LOGDIR (unset)
>     PATH=/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/cpanel/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/perl/524/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/cpanel/perl5/524/bin
>     PERL_BADLANG (unset)
>     SHELL=/bin/zsh
>

RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 370b
On Tue Sep 13 11:26:24 2016, demerphq wrote: Show quoted text
> > I did not search for other examples. I thought it might be best to > > open a discussion before proceeding on any work.
> > Not sure what there is to discuss really. Wrong is wrong. ☺️ > > Yves
Proposed patch. I'm 99% certain we need <= not <. However this is why I use Perl. So I don't have to remember that ;)
Subject: 0001-Prevent-Perl_gv_fetchmethod_pvn_flags-from-running-p.patch
From 1c722feb82f0b29d5b439ddc1dd3f05a7a3e115f Mon Sep 17 00:00:00 2001 From: Todd Rinaldo <toddr@cpan.org> Date: Tue, 13 Sep 2016 13:21:03 -0500 Subject: [PATCH] Prevent Perl_gv_fetchmethod_pvn_flags from running past the end of a string Addressed reported issue from Perl #129267 --- gv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gv.c b/gv.c index 1bc8bf2..c9b706c 100644 --- a/gv.c +++ b/gv.c @@ -1028,7 +1028,7 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, const STRLEN le the error reporting code. */ } - for (nend = name; *nend || nend != (origname + len); nend++) { + for (nend = name; *nend && nend <= (origname + len); nend++) { if (*nend == '\'') { nsplit = nend; name = nend + 1; -- 2.10.0
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
From: Andy Lester <andy [...] petdance.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
To: demerphq <demerphq [...] gmail.com>
Date: Tue, 13 Sep 2016 13:28:33 -0500
Download (untitled) / with headers
text/plain 559b

Show quoted text
On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote:

> I did not search for other examples. I thought it might be best to
> open a discussion before proceeding on any work.

Not sure what there is to discuss really.  Wrong is wrong. ☺️


I took the comment to mean “For all I know, there may be other examples elsewhere in the codebase, and it might even be a security hole, but I haven’t investigated further, but someone probably should before we just patch this and call it done."

--
Andy Lester => www.petdance.com

Date: Tue, 13 Sep 2016 20:50:45 +0200
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Download (untitled) / with headers
text/plain 2.8k
On 13 September 2016 at 20:29, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:26:24 2016, demerphq wrote: >
>> > I did not search for other examples. I thought it might be best to >> > open a discussion before proceeding on any work.
>> >> Not sure what there is to discuss really. Wrong is wrong. ☺️ >> >> Yves
> > > Proposed patch. I'm 99% certain we need <= not <. However this is why I use Perl. So I don't have to remember that ;) > > --- > via perlbug: queue: perl5 status: open > https://rt.perl.org/Ticket/Display.html?id=129267 > > From 1c722feb82f0b29d5b439ddc1dd3f05a7a3e115f Mon Sep 17 00:00:00 2001 > From: Todd Rinaldo <toddr@cpan.org> > Date: Tue, 13 Sep 2016 13:21:03 -0500 > Subject: [PATCH] Prevent Perl_gv_fetchmethod_pvn_flags from running past the > end of a string > > Addressed reported issue from Perl #129267 > --- > gv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gv.c b/gv.c > index 1bc8bf2..c9b706c 100644 > --- a/gv.c > +++ b/gv.c > @@ -1028,7 +1028,7 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, const STRLEN le > the error reporting code. */ > } > > - for (nend = name; *nend || nend != (origname + len); nend++) { > + for (nend = name; *nend && nend <= (origname + len); nend++) { > if (*nend == '\'') { > nsplit = nend; > name = nend + 1;
Well, that fixes /one/ issue. But I think there are more. I was putting together a patch like this: $ git diff gv.c diff --git a/gv.c b/gv.c index 1bc8bf2..23700a0 100644 --- a/gv.c +++ b/gv.c @@ -1009,6 +1009,7 @@ GV * Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, const STRLEN len, U32 flags) { const char *nend; + const char * const name_end= name + len; const char *nsplit = NULL; GV* gv; HV* ostash = stash; @@ -1028,15 +1029,15 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, const STRLE the error reporting code. */ } - for (nend = name; *nend || nend != (origname + len); nend++) { + for (nend = name; nend < name_end && *nend; nend++) { if (*nend == '\'') { nsplit = nend; name = nend + 1; } - else if (*nend == ':' && *(nend + 1) == ':') { - nsplit = nend++; - name = nend + 1; - } + else if (*nend == ':' && nend+1 < name_end && *(nend + 1) == ':') { + nsplit = nend++; + name = nend + 1; + } } if (nsplit) { if ((nsplit - origname) == 5 && memEQ(origname, "SUPER", 5)) { Although I will probably follow it will another patch to rename "nend". Also, somewhat concerning is the comment right above this function: /* Don't merge this yet, as it's likely to get a len parameter, and possibly even a U32 hash */ Sigh. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
To: Andy Lester <andy [...] petdance.com>
Date: Tue, 13 Sep 2016 20:54:31 +0200
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
On 13 September 2016 at 20:28, Andy Lester <andy@petdance.com> wrote:
Show quoted text

On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote:

> I did not search for other examples. I thought it might be best to
> open a discussion before proceeding on any work.

Not sure what there is to discuss really.  Wrong is wrong. ☺️


I took the comment to mean “For all I know, there may be other examples elsewhere in the codebase, and it might even be a security hole, but I haven’t investigated further, but someone probably should before we just patch this and call it done."

Ah, good catch. Well, maybe there is a security hole here, I don't know.

But there are a lot of issues with the code as written. In several places it accesses memory it can't know that we own.

It looks to me like if you called this function with a string which ended in exactly one colon that we would continue reading until we hit a null or segfaulted.

What would /then/ happen is not clear.

Yves



--
perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Tue Sep 13 11:51:18 2016, demerphq wrote: Show quoted text
> Well, that fixes /one/ issue. But I think there are more. I was > putting together a patch like this: > > $ git diff gv.c > diff --git a/gv.c b/gv.c > index 1bc8bf2..23700a0 100644 > --- a/gv.c > +++ b/gv.c > @@ -1009,6 +1009,7 @@ GV * > Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, > const STRLEN len, U32 flags) > { > const char *nend; > + const char * const name_end= name + len;
s/char * const/char */ Show quoted text
> const char *nsplit = NULL; > GV* gv; > HV* ostash = stash; > @@ -1028,15 +1029,15 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, > const char *name, const STRLE > the error reporting code. */ > } > > - for (nend = name; *nend || nend != (origname + len); nend++) { > + for (nend = name; nend < name_end && *nend; nend++) { > if (*nend == '\'') { > nsplit = nend; > name = nend + 1;
I assume you changed <= to < to address this? It's unclear to me the effects of name being an empty string. Show quoted text
> } > - else if (*nend == ':' && *(nend + 1) == ':') { > - nsplit = nend++; > - name = nend + 1; > - } > + else if (*nend == ':' && nend+1 < name_end && *(nend + 1) == ':') { > + nsplit = nend++; > + name = nend + 1; > + } > } > if (nsplit) { > if ((nsplit - origname) == 5 && memEQ(origname, "SUPER", 5)) { > > > Although I will probably follow it will another patch to rename > "nend". > > Also, somewhat concerning is the comment right above this function: > > /* Don't merge this yet, as it's likely to get a len parameter, and > possibly > even a U32 hash */ > > Sigh. > > Yves
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Date: Tue, 13 Sep 2016 20:56:52 +0200
Download (untitled) / with headers
text/plain 801b
On 13 September 2016 at 20:29, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:26:24 2016, demerphq wrote: >
>> > I did not search for other examples. I thought it might be best to >> > open a discussion before proceeding on any work.
>> >> Not sure what there is to discuss really. Wrong is wrong. ☺️ >> >> Yves
> > > Proposed patch. I'm 99% certain we need <= not <. However this is why I use Perl. So I don't have to remember that ;)
FWIW. I dont think so. We need <. Assume name = 0, and len = 10, then we should stop processing when name hits 10, as the 10 chars we are allowed to look at will be at memory positions 0 through 9. If you use <= then we would try to dereference *10, which we dont own. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 701b
On Tue Sep 13 11:29:21 2016, petdance wrote: Show quoted text
>
> > On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote: > >
> > > I did not search for other examples. I thought it might be best to > > > open a discussion before proceeding on any work.
> > > > Not sure what there is to discuss really. Wrong is wrong. ☺️ > >
> > > I took the comment to mean “For all I know, there may be other > examples elsewhere in the codebase, and it might even be a security > hole, but I haven’t investigated further, but someone probably should > before we just patch this and call it done." >
Correct. I was a little concerned this code pattern might be being used to walk other string incorrectly.
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Tue, 13 Sep 2016 20:59:43 +0200
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Download (untitled) / with headers
text/plain 1.8k
On 13 September 2016 at 20:55, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:51:18 2016, demerphq wrote: >
>> Well, that fixes /one/ issue. But I think there are more. I was >> putting together a patch like this: >> >> $ git diff gv.c >> diff --git a/gv.c b/gv.c >> index 1bc8bf2..23700a0 100644 >> --- a/gv.c >> +++ b/gv.c >> @@ -1009,6 +1009,7 @@ GV * >> Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, >> const STRLEN len, U32 flags) >> { >> const char *nend; >> + const char * const name_end= name + len;
> s/char * const/char */ >
>> const char *nsplit = NULL; >> GV* gv; >> HV* ostash = stash; >> @@ -1028,15 +1029,15 @@ Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, >> const char *name, const STRLE >> the error reporting code. */ >> } >> >> - for (nend = name; *nend || nend != (origname + len); nend++) { >> + for (nend = name; nend < name_end && *nend; nend++) { >> if (*nend == '\'') { >> nsplit = nend; >> name = nend + 1;
> I assume you changed <= to < to address this? It's unclear to me the effects of name being an empty string.
If its an empty string it should be len == 0. Notice i reversed the clauses as well. We dereference AFTER we have assured ourselves that we are looking at memory we own. Show quoted text
> >
>> } >> - else if (*nend == ':' && *(nend + 1) == ':') { >> - nsplit = nend++; >> - name = nend + 1; >> - } >> + else if (*nend == ':' && nend+1 < name_end && *(nend + 1) == ':') { >> + nsplit = nend++; >> + name = nend + 1; >> + }
And again here. We have to recheck things so that we are sure we can even look at nend + 1. nend is a terrible name. Should be name_ptr or something like that. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
CC: Perl5 Porteros <perl5-porters [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Tue, 13 Sep 2016 21:01:00 +0200
On 13 September 2016 at 20:59, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:29:21 2016, petdance wrote:
>>
>> > On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote: >> >
>> > > I did not search for other examples. I thought it might be best to >> > > open a discussion before proceeding on any work.
>> > >> > Not sure what there is to discuss really. Wrong is wrong. ☺️ >> >
>> >> >> I took the comment to mean “For all I know, there may be other >> examples elsewhere in the codebase, and it might even be a security >> hole, but I haven’t investigated further, but someone probably should >> before we just patch this and call it done." >>
> > Correct. I was a little concerned this code pattern might be being used to walk other string incorrectly.
We can and should audit for similar patterns, but my gut feeling is that this code is pretty unusual, as it is trying to extract the function part of a fully qualified name. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Tue, 13 Sep 2016 21:02:59 +0200
From: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Download (untitled) / with headers
text/plain 759b
On 13 September 2016 at 20:55, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:51:18 2016, demerphq wrote: >
>> Well, that fixes /one/ issue. But I think there are more. I was >> putting together a patch like this: >> >> $ git diff gv.c >> diff --git a/gv.c b/gv.c >> index 1bc8bf2..23700a0 100644 >> --- a/gv.c >> +++ b/gv.c >> @@ -1009,6 +1009,7 @@ GV * >> Perl_gv_fetchmethod_pvn_flags(pTHX_ HV *stash, const char *name, >> const STRLEN len, U32 flags) >> { >> const char *nend; >> + const char * const name_end= name + len;
> s/char * const/char */
Is that required for some reason I haven't noticed? My version feels right as we aren't allowed to write to *name_end as it points to memory we dont know we own. Yves
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 454b
On Tue Sep 13 12:01:23 2016, demerphq wrote: Show quoted text
> > We can and should audit for similar patterns, but my gut feeling is > that this code is pretty unusual, as it is trying to extract the > function part of a fully qualified name. >
S_parse_gv_stash_name is making a similar look ahead mistake with name_cursor[1]. That looks messier to fix but it should probably be another case or a committer should just go through and make the corrections sans perlbug?
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Tue, 13 Sep 2016 21:08:23 +0200
Download (untitled) / with headers
text/plain 646b
On 13 September 2016 at 21:06, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 12:01:23 2016, demerphq wrote:
>> >> We can and should audit for similar patterns, but my gut feeling is >> that this code is pretty unusual, as it is trying to extract the >> function part of a fully qualified name. >>
> > S_parse_gv_stash_name is making a similar look ahead mistake with name_cursor[1]. That looks messier to fix but it should probably be another case or a committer should just go through and make the corrections sans perlbug?
You find 'em, and I fix 'em? :-) Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Date: Tue, 13 Sep 2016 21:14:04 +0200
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Download (untitled) / with headers
text/plain 784b
On 13 September 2016 at 21:06, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 12:01:23 2016, demerphq wrote:
>> >> We can and should audit for similar patterns, but my gut feeling is >> that this code is pretty unusual, as it is trying to extract the >> function part of a fully qualified name. >>
> > S_parse_gv_stash_name is making a similar look ahead mistake with name_cursor[1]. That looks messier to fix but it should probably be another case or a committer should just go through and make the corrections sans perlbug?
A quick look didnt reveal to me any issues here. If you look at the way it uses name_em1 and name_end it looks fine. Can you point me more closely at the code you suspect? Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.1k
On Tue Sep 13 11:54:55 2016, demerphq wrote: Show quoted text
> On 13 September 2016 at 20:28, Andy Lester <andy@petdance.com> wrote: >
> > > > On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote: > >
> > > I did not search for other examples. I thought it might be best to > > > open a discussion before proceeding on any work.
> > > > Not sure what there is to discuss really. Wrong is wrong. ☺️ > > > > > > I took the comment to mean “For all I know, there may be other examples > > elsewhere in the codebase, and it might even be a security hole, but I > > haven’t investigated further, but someone probably should before we just > > patch this and call it done." > >
> > Ah, good catch. Well, maybe there is a security hole here, I don't know. > > But there are a lot of issues with the code as written. In several places > it accesses memory it can't know that we own. > > It looks to me like if you called this function with a string which ended > in exactly one colon that we would continue reading until we hit a null or > segfaulted. > > What would /then/ happen is not clear.
This is an API, function, right? So we can add a test to XS::APItest? -- Father Chrysostomos
Date: Tue, 13 Sep 2016 21:55:26 +0200
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
From: demerphq <demerphq [...] gmail.com>
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
Download (untitled) / with headers
text/plain 1.4k
On 13 September 2016 at 21:29, Father Chrysostomos via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 11:54:55 2016, demerphq wrote:
>> On 13 September 2016 at 20:28, Andy Lester <andy@petdance.com> wrote: >>
>> > >> > On Sep 13, 2016, at 1:25 PM, demerphq <demerphq@gmail.com> wrote: >> >
>> > > I did not search for other examples. I thought it might be best to >> > > open a discussion before proceeding on any work.
>> > >> > Not sure what there is to discuss really. Wrong is wrong. ☺️ >> > >> > >> > I took the comment to mean “For all I know, there may be other examples >> > elsewhere in the codebase, and it might even be a security hole, but I >> > haven’t investigated further, but someone probably should before we just >> > patch this and call it done." >> >
>> >> Ah, good catch. Well, maybe there is a security hole here, I don't know. >> >> But there are a lot of issues with the code as written. In several places >> it accesses memory it can't know that we own. >> >> It looks to me like if you called this function with a string which ended >> in exactly one colon that we would continue reading until we hit a null or >> segfaulted. >> >> What would /then/ happen is not clear.
> > This is an API, function, right? So we can add a test to XS::APItest?
I think so yes. FWIW, I accidentally pushed my patches before tests completed, and the tests failed, so i have reverted to see why. Sorry for the commit noise. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 871b
On Tue Sep 13 12:14:27 2016, demerphq wrote: Show quoted text
> On 13 September 2016 at 21:06, Todd Rinaldo via RT > <perlbug-followup@perl.org> wrote:
> > On Tue Sep 13 12:01:23 2016, demerphq wrote:
> >> > >> We can and should audit for similar patterns, but my gut feeling is > >> that this code is pretty unusual, as it is trying to extract the > >> function part of a fully qualified name. > >>
> > > > S_parse_gv_stash_name is making a similar look ahead mistake with > > name_cursor[1]. That looks messier to fix but it should probably be > > another case or a committer should just go through and make the > > corrections sans perlbug?
> > A quick look didnt reveal to me any issues here. If you look at the > way it uses name_em1 and name_end it looks fine. Can you point me more > closely at the code you suspect? >
Yep. Apologies. I pulled the trigger too quick on that one.
Subject: Re: [perl #129267] Possible string overrun with invalid len in gv.c
From: demerphq <demerphq [...] gmail.com>
CC: Perl5 Porteros <perl5-porters [...] perl.org>
To: Perl RT Bug Tracker <perlbug-followup [...] perl.org>
Date: Tue, 13 Sep 2016 23:30:20 +0200
Download (untitled) / with headers
text/plain 1.6k
On 13 September 2016 at 22:00, Todd Rinaldo via RT <perlbug-followup@perl.org> wrote: Show quoted text
> On Tue Sep 13 12:14:27 2016, demerphq wrote:
>> On 13 September 2016 at 21:06, Todd Rinaldo via RT >> <perlbug-followup@perl.org> wrote:
>> > On Tue Sep 13 12:01:23 2016, demerphq wrote:
>> >> >> >> We can and should audit for similar patterns, but my gut feeling is >> >> that this code is pretty unusual, as it is trying to extract the >> >> function part of a fully qualified name. >> >>
>> > >> > S_parse_gv_stash_name is making a similar look ahead mistake with >> > name_cursor[1]. That looks messier to fix but it should probably be >> > another case or a committer should just go through and make the >> > corrections sans perlbug?
>> >> A quick look didnt reveal to me any issues here. If you look at the >> way it uses name_em1 and name_end it looks fine. Can you point me more >> closely at the code you suspect? >>
> > Yep. Apologies. I pulled the trigger too quick on that one.
Yeah, well so did I with that premature push. :-) Anyway, I repushed my changes: cfb7367 fix #129267: rework gv_fetchmethod_pvn_flags separator parsing e2cace1 clean up gv_fetchmethod_pvn_flags: rename nsplit to last_separator d5ea0ef clean up gv_fetchmethod_pvn_flags: move origname init to function start 65308f8 clean up gv_fetchmethod_pvn_flags: introduce name_end I think the code is much easier to understand now. FC, I didnt really see a way to test for a string overrun via XS::APItest, maybe I didnt try hard enough tho. Can you think of something? (I think this ticket can be closed tho.) Yves -- perl -Mre=debug -e "/just|another|perl|hacker/"
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 670b
On Tue Sep 13 14:30:52 2016, demerphq wrote: Show quoted text
> > Anyway, I repushed my changes: > > cfb7367 fix #129267: rework gv_fetchmethod_pvn_flags separator parsing > e2cace1 clean up gv_fetchmethod_pvn_flags: rename nsplit to > last_separator > d5ea0ef clean up gv_fetchmethod_pvn_flags: move origname init to > function start > 65308f8 clean up gv_fetchmethod_pvn_flags: introduce name_end > > I think the code is much easier to understand now. > > FC, I didnt really see a way to test for a string overrun via > XS::APItest, maybe I didnt try hard enough tho. Can you think of > something? > > (I think this ticket can be closed tho.) > > Yves
Looks really good. Thanks!
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 418b
On Tue Sep 13 14:30:52 2016, demerphq wrote: Show quoted text
> I think the code is much easier to understand now.
Bummer. Show quoted text
> FC, I didnt really see a way to test for a string overrun via > XS::APItest, maybe I didnt try hard enough tho. Can you think of > something?
The test I added in 1665b718d crashed quite nicely before I rebased on top of your fix. Show quoted text
> (I think this ticket can be closed tho.)
Done. -- Father Chrysostomos
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 703b
On Tue Sep 13 22:44:25 2016, sprout wrote: Show quoted text
> On Tue Sep 13 14:30:52 2016, demerphq wrote:
> > I think the code is much easier to understand now.
> > Bummer. >
> > FC, I didnt really see a way to test for a string overrun via > > XS::APItest, maybe I didnt try hard enough tho. Can you think of > > something?
> > The test I added in 1665b718d crashed quite nicely before I rebased on > top of your fix. >
> > (I think this ticket can be closed tho.)
> > Done.
Not sure if it's worth it but this backports cleanly to 5.22. Should it be considered for backport? Best I can tell, the backport would be: git diff ee33cc1af47fcf535fa762422a110c2782c45935..cfb736762c1becf344ce6beaa701ff2e1abd5f9c gv.c
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