Skip to content
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

Possible string overrun with invalid len in gv.c #15598

Closed
p5pRT opened this issue Sep 13, 2016 · 28 comments
Closed

Possible string overrun with invalid len in gv.c #15598

p5pRT opened this issue Sep 13, 2016 · 28 comments

Comments

@p5pRT
Copy link

p5pRT commented Sep 13, 2016

Migrated from rt.perl.org#129267 (status was 'resolved')

Searchable as RT129267$

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

Created by @toddr

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.

Perl Info

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

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

# 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-archive.perl.org/perl5/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++) {
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

[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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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 ;)

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @petdance

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 20​:29, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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-archive.perl.org/perl5/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

Inline Patch
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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

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.

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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.

    \}

- 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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 20​:29, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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.

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 20​:55, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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.

    \}

- 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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 20​:59, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 20​:55, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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?

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

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?

You find 'em, and I fix 'em?

:-)

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

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?

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @cpansprout

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?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 21​:29, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

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.

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @demerphq

On 13 September 2016 at 22​:00, Todd Rinaldo via RT
<perlbug-followup@​perl.org> wrote​:

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/"

@p5pRT
Copy link
Author

p5pRT commented Sep 13, 2016

From @toddr

On Tue Sep 13 14​:30​:52 2016, demerphq wrote​:

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!

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2016

From @cpansprout

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 1665b71 crashed quite nicely before I rebased on top of your fix.

(I think this ticket can be closed tho.)

Done.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 14, 2016

@cpansprout - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2016

From @toddr

On Tue Sep 13 22​:44​:25 2016, sprout wrote​:

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 1665b71 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 ee33cc1..cfb7367 gv.c

@p5pRT
Copy link
Author

p5pRT commented Sep 21, 2016

From [Unknown Contact. See original ticket]

On Tue Sep 13 22​:44​:25 2016, sprout wrote​:

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 1665b71 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 ee33cc1..cfb7367 gv.c

@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

From @khwilliamson

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.

@p5pRT p5pRT closed this as completed May 30, 2017
@p5pRT
Copy link
Author

p5pRT commented May 30, 2017

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant