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

Owner: Nobody
Requestors: hanno [at] hboeck.de
Cc:
AdminCc:

Operating System: Linux
PatchStatus: (no value)
Severity: medium
Type: core
Perl Version: 5.24.1
Fixed In: 5.27.7



Date: Fri, 13 Jan 2017 16:48:54 +0100
To: perlbug [...] perl.org
From: Hanno Böck <hanno [...] hboeck.de>
Subject: Out of bounds read when calling perl from C
This is a bug report for perl from hanno@hboeck.de, generated with the help of perlbug 1.40 running under perl 5.24.1. ----------------------------------------------------------------- [Please describe your issue here] Out of bounds read when calling perl from C When calling perl from C with parameters an out of bounds array access can happen. Here's some example code triggering that bug: #include <EXTERN.h> #include <perl.h> #include <string.h> #include <stdlib.h> char *perl_args[] = { "", "-e", "0" }; int main() { PerlInterpreter *my_perl = perl_alloc(); perl_construct(my_perl); char **foo = malloc(3 * sizeof(char *)); memcpy(foo, perl_args, 3 * sizeof(char *)); perl_parse(my_perl, 0, 3, foo, NULL); } When you run that with valgrind (alternatively also with perl built with address sanitizer) you'll see a bug like this: ==9608== Invalid read of size 8 ==9608== at 0x4E92417: S_parse_body (perl.c:2166) ==9608== by 0x4E92417: perl_parse (perl.c:1650) ==9608== by 0x400831: main (perl-oob.c:16) ==9608== Address 0x5f09408 is 0 bytes after a block of size 24 alloc'd ==9608== at 0x4C2BF50: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==9608== by 0x4007F4: main (perl-oob.c:12) The problem is this code (perl.c line 2165/2166, perl-5.24.1-RC4): if (!scriptname) scriptname = argv[0]; This assumes argv points to the beginning of the parameter array. However it does not at this point in the code. A few lines earlier (line 1888) we can see this: for (argc--,argv++; argc > 0; argc--,argv++) { This loop will let the argv pointer point to the end of the parameter array. Thus a subsequent access to argv[0] will read invalid memory. I think the proper fix is to replace argv[0] with PL_origargv[0] in line 2166. [Please do not change anything below this line] ----------------------------------------------------------------- --- Flags: category=core severity=medium --- Site configuration information for perl 5.24.1: Configured by Gentoo at Mon Jan 9 11:45:06 CET 2017. Summary of my perl5 (revision 5 version 24 subversion 1) configuration: Platform: osname=linux, osvers=4.8.15, archname=x86_64-linux uname='linux pc1 4.8.15 #1 smp mon dec 26 01:24:59 cet 2016 x86_64 intel(r) core(tm) i7-4600u cpu @ 2.10ghz genuineintel gnulinux ' config_args='-des -Dinstallprefix=/usr -Dinstallusrbinperl=n -Di_ndbm -Di_gdbm -Di_db -DDEBUGGING=-g -Dinc_version_list=5.24.0/x86_64-linux 5.24.0 -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Dnoextensions=ODBM_File -Duseshrplib -Darchname=x86_64-linux -Dcc=x86_64-pc-linux-gnu-gcc -Doptimize=-O2 -pipe -march=native -fomit-frame-pointer -g -Dldflags=-Wl,-O1 -Wl,--as-needed -Dprefix=/usr -Dsiteprefix=/usr/local -Dvendorprefix=/usr -Dscriptdir=/usr/bin -Dprivlib=/usr/lib64/perl5/5.24.1 -Darchlib=/usr/lib64/perl5/5.24.1/x86_64-linux -Dsitelib=/usr/local/lib64/perl5/5.24.1 -Dsitearch=/usr/local/lib64/perl5/5.24.1/x86_64-linux -Dvendorlib=/usr/lib64/perl5/vendor_perl/5.24.1 -Dvendorarch=/usr/lib64/perl5/vendor_perl/5.24.1/x86_64-linux -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Dsiteman3dir=/usr/local/man/man3 -Dvendorman1dir=/usr/share/man/man1 -Dvendorman3dir=/usr/share/man/man3 -Dman1ext=1 -Dman3ext=3pm -Dlibperl=libperl.so.5.24.1 -Dlocincpth=/usr/include -Dglibpth=/lib64 /usr/lib64 -Duselargefiles -Dd_semctl_semun -Dcf_by=Gentoo -Dmyhostname=localhost -Dperladmin=root@localhost -Ud_csh -Dsh=/bin/sh -Dtargetsh=/bin/sh -Uusenm -Di_ndbm -Di_gdbm -Di_db -DDEBUGGING=-g -Dinc_version_list=5.24.0/x86_64-linux 5.24.0 -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Dnoextensions=ODBM_File' hint=recommended, useposix=true, d_sigaction=define useithreads=undef, usemultiplicity=undef use64bitint=define, use64bitall=define, uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='x86_64-pc-linux-gnu-gcc', ccflags ='-fwrapv -fno-strict-aliasing -pipe -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2 -pipe -march=native -fomit-frame-pointer -g', cppflags='-fwrapv -fno-strict-aliasing -pipe' ccversion='', gccversion='6.3.0', 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='x86_64-pc-linux-gnu-gcc', ldflags ='-Wl,-O1 -Wl,--as-needed' libpth=/usr/local/lib64 /lib64 /usr/lib64 /usr/local/lib /usr/lib/gcc/x86_64-pc-linux-gnu/6.3.0/include-fixed /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.23.so, so=so, useshrplib=true, libperl=libperl.so.5.24.1 gnulibc_version='2.23' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E' cccdlflags='-fPIC', lddlflags='-shared -O2 -pipe -march=native -fomit-frame-pointer -g -Wl,-O1 -Wl,--as-needed' Locally applied patches: RC4 gentoo/hints_hpux - Fix hpux hints gentoo/aix_soname - aix gcc detection and shared library soname support gentoo/EUMM-RUNPATH - https://bugs.gentoo.org/105054 cpan/ExtUtils-MakeMaker: drop $PORTAGE_TMPDIR from LD_RUN_PATH gentoo/config_over - Remove -rpath and append LDFLAGS to lddlflags gentoo/opensolaris_headers - [PATCH] Add headers for opensolaris gentoo/patchlevel - List packaged patches for perl-5.24.1_rc4(#2) in patchlevel.h gentoo/cpanplus_definstalldirs - Configure CPANPLUS to use the site directories by default. gentoo/cleanup-paths - [PATCH] Cleanup PATH and shrpenv gentoo/enc2xs - Tweak enc2xs to follow symlinks and ignore missing @INC directories. gentoo/darwin-cc-ld - https://bugs.gentoo.org/297751 [PATCH] darwin: Use $CC to link gentoo/cpan_definstalldirs - Provide a sensible INSTALLDIRS default for modules installed from CPAN. gentoo/interix - [PATCH] Fix interix hints gentoo/create_libperl_soname - https://bugs.gentoo.org/286840 [PATCH] Set libperl soname gentoo/mod_paths - Add /etc/perl to @INC gentoo/EUMM_perllocalpod - gentoo/drop_fstack_protector - https://bugs.gentoo.org/348557 [PATCH] Don't force -fstack-protector on everyone gentoo/usr_local - gentoo/D-SHA-CFLAGS - https://bugs.gentoo.org/506818 [PATCH] [PATCH] Do not set custom CFLAGS in cpan/Digest-SHA gentoo/io_socket_ip_tests - gentoo/cygwin-libperl - [PATCH] Cygwin: avoid libperl.dll.dll.a gentoo/tests - Minimal changes to test framework so everything passes with Gentoo patchset debian/cpan-missing-site-dirs - Fix CPAN::FirstTime defaults with nonexisting site dirs if a parent is writable debian/makemaker-pasthru - Pass LD settings through to subdirectories fixes/memoize_storable_nstore - [rt.cpan.org #77790] Memoize::Storable: respect 'nstore' option not respected fixes/podman-pipe - Better errors for man pages from standard input fixes/respect_umask - Respect umask during installation fixes/net_smtp_docs - [rt.cpan.org #36038] Document the Net::SMTP 'Port' option fixes/document_makemaker_ccflags - [rt.cpan.org #68613] Document that CCFLAGS should include $Config{ccflags} fixes/parallel-manisort.patch - [PATCH] Fix parallel building --- @INC for perl 5.24.1: /home/hanno/perl5/lib/perl5/x86_64-linux /home/hanno/perl5/lib/perl5 /etc/perl /usr/local/lib64/perl5/5.24.1/x86_64-linux /usr/local/lib64/perl5/5.24.1 /usr/lib64/perl5/vendor_perl/5.24.1/x86_64-linux /usr/lib64/perl5/vendor_perl/5.24.1 /usr/local/lib64/perl5 /usr/lib64/perl5/vendor_perl /usr/lib64/perl5/5.24.1/x86_64-linux /usr/lib64/perl5/5.24.1 --- Environment for perl 5.24.1: HOME=/home/hanno LANG=en_US.utf8 LANGUAGE (unset) LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/hanno/perl5/bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/6.3.0:/home/hanno/bin PERL5LIB=/home/hanno/perl5/lib/perl5 PERL_BADLANG (unset) PERL_LOCAL_LIB_ROOT=/home/hanno/perl5 PERL_MB_OPT=--install_base "/home/hanno/perl5" PERL_MM_OPT=INSTALL_BASE=/home/hanno/perl5 SHELL=/bin/bash
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 3.3k
On Fri, 13 Jan 2017 15:54:24 GMT, hanno@hboeck.de wrote: Show quoted text
> This is a bug report for perl from hanno@hboeck.de, > generated with the help of perlbug 1.40 running under perl 5.24.1. > > > ----------------------------------------------------------------- > [Please describe your issue here] > > Out of bounds read when calling perl from C > > When calling perl from C with parameters an out of bounds array access > can happen. > > Here's some example code triggering that bug: > #include <EXTERN.h> > #include <perl.h> > #include <string.h> > #include <stdlib.h> > char *perl_args[] = { "", "-e", "0" }; > int main() > { > PerlInterpreter *my_perl = perl_alloc(); > perl_construct(my_perl); > char **foo = malloc(3 * sizeof(char *)); > memcpy(foo, perl_args, 3 * sizeof(char *)); > perl_parse(my_perl, 0, 3, foo, NULL); > } > > When you run that with valgrind (alternatively also with perl built > with address sanitizer) you'll see a bug like this: > ==9608== Invalid read of size 8 > ==9608== at 0x4E92417: S_parse_body (perl.c:2166) > ==9608== by 0x4E92417: perl_parse (perl.c:1650) > ==9608== by 0x400831: main (perl-oob.c:16) > ==9608== Address 0x5f09408 is 0 bytes after a block of size 24 > alloc'd > ==9608== at 0x4C2BF50: malloc > (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==9608== > by 0x4007F4: main (perl-oob.c:12) > > > The problem is this code (perl.c line 2165/2166, perl-5.24.1-RC4): > if (!scriptname) > scriptname = argv[0]; > > This assumes argv points to the beginning of the parameter array. > However it does not at this point in the code. > > A few lines earlier (line 1888) we can see this: > for (argc--,argv++; argc > 0; argc--,argv++) { > > This loop will let the argv pointer point to the end of the parameter > array. Thus a subsequent access to argv[0] will read invalid memory. > > I think the proper fix is to replace argv[0] with PL_origargv[0] in > line 2166. > >
Making the change you suggested -- PL_origargv[0] -- and no other, I attempted to build Perl 5 blead. 'make' died on both FreeBSD and Linux at this point: ##### /bin/ln -s perldelta.pod pod/perl5259delta.pod cc -c -DPERL_CORE -fwrapv -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -std=c89 -O2 -Wall -Werror=declaration-after-statement -Wextra -Wc++-compat -Wwrite-strings op.c cc -fstack-protector-strong -L/usr/local/lib -o miniperl \ opmini.o perlmini.o gv.o toke.o perly.o pad.o regcomp.o dump.o util.o mg.o reentr.o mro_core.o keywords.o hv.o av.o run.o pp_hot.o sv.o pp.o scope.o pp_ctl.o pp_sys.o doop.o doio.o regexec.o utf8.o taint.o deb.o universal.o globals.o perlio.o perlapi.o numeric.o mathoms.o locale.o pp_pack.o pp_sort.o caretx.o dquote.o time64.o miniperlmain.o -lpthread -lnsl -ldl -lm -lcrypt -lutil -lc ./miniperl -w -Ilib -Idist/Exporter/lib -MExporter -e '<?>' || sh -c 'echo >&2 Failed to build miniperl. Please run make minitest; exit 1' ./miniperl -Ilib -f write_buildcustomize.pl Unrecognized character \x7F; marked by <-- HERE after <-- HERE near column 1 at ./miniperl line 1. makefile:362: recipe for target 'lib/buildcustomize.pl' failed make: *** [lib/buildcustomize.pl] Error 255 make: *** Waiting for unfinished jobs.... ##### See smoke-me/jkeenan/130550-out-of-bounds branch. Thank you very much. -- James E Keenan (jkeenan@cpan.org)
RT-Send-CC: perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 1.7k
On Fri, 13 Jan 2017 07:54:24 -0800, hanno@hboeck.de wrote: Show quoted text
> The problem is this code (perl.c line 2165/2166, perl-5.24.1-RC4): > if (!scriptname) > scriptname = argv[0]; > > This assumes argv points to the beginning of the parameter array.
Not quite - if you run "perl -Ilib foo", it wants to pick up the scriptname 'foo'. However it shouldn't attempt to pick that up if there are no more arguments. I suspect all platforms tend to terminate the argv array with a spare NULL, else we'd surely have seen this before: that behaviour has been in the code for at least 20 years. I think the fix should look something like the below, I'll try to wrap a test around it. The later uses of arg{c,v} also look dubious, I'll look at those some more as well. Hugo [perl #130550] don't read non-existent scriptname If perl is invoked without a scriptname (eg "perl -e 1" or just "perl") we read past the argument list before noticing that we didn't need to. diff --git a/perl.c b/perl.c index 09eb2f4..0ee6730 100644 --- a/perl.c +++ b/perl.c @@ -2206,18 +2206,20 @@ S_parse_body(pTHX_ char **env, XSINIT_t xsinit) } #endif - if (!scriptname) - scriptname = argv[0]; if (PL_e_script) { argc++,argv--; scriptname = BIT_BUCKET; /* don't look for script or read stdin */ } else if (scriptname == NULL) { + if (argc > 0) + scriptname = argv[0]; + else { #ifdef MSDOS - if ( PerlLIO_isatty(PerlIO_fileno(PerlIO_stdin())) ) - moreswitches("h"); + if ( PerlLIO_isatty(PerlIO_fileno(PerlIO_stdin())) ) + moreswitches("h"); #endif - scriptname = "-"; + scriptname = "-"; + } } assert (!TAINT_get);
CC: perl5-porters [...] perl.org
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
From: Dave Mitchell <davem [...] iabyn.com>
Date: Sat, 14 Jan 2017 11:33:54 +0000
To: James E Keenan via RT <perlbug-followup [...] perl.org>
Download (untitled) / with headers
text/plain 500b
Show quoted text
> On Fri, 13 Jan 2017 15:54:24 GMT, hanno@hboeck.de wrote:
> > memcpy(foo, perl_args, 3 * sizeof(char *)); > > perl_parse(my_perl, 0, 3, foo, NULL);
From perlembed.pod: Mind that argv[argc] must be NULL, same as those passed to a main function in C. You need an extra NULL in your args list. -- The crew of the Enterprise encounter an alien life form which is surprisingly neither humanoid nor made from pure energy. -- Things That Never Happen in "Star Trek" #22
RT-Send-CC: davem [...] iabyn.com, perl5-porters [...] perl.org
Download (untitled) / with headers
text/plain 723b
On Sat, 14 Jan 2017 03:34:43 -0800, davem wrote: Show quoted text
> > On Fri, 13 Jan 2017 15:54:24 GMT, hanno@hboeck.de wrote:
> > > memcpy(foo, perl_args, 3 * sizeof(char *)); > > > perl_parse(my_perl, 0, 3, foo, NULL);
> > > From perlembed.pod: > > Mind that argv[argc] must be NULL, same as those passed to a main > function in C. > > You need an extra NULL in your args list.
Is there any reason we _should_ require that, other than that it would require us to tighten up our argv/argc handling? It feels like a somewhat unperlish stipulation. If for example we're sometimes handing it off to libc calls that already require it then fair enough, but I'm not aware that we're doing anything like that. Hugo
To: Hugo van der Sanden via RT <perlbug-followup [...] perl.org>
Date: Sun, 15 Jan 2017 10:59:06 +0000
From: Dave Mitchell <davem [...] iabyn.com>
CC: perl5-porters [...] perl.org
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
Download (untitled) / with headers
text/plain 1.1k
On Sat, Jan 14, 2017 at 12:11:02PM -0800, Hugo van der Sanden via RT wrote: Show quoted text
> On Sat, 14 Jan 2017 03:34:43 -0800, davem wrote:
> > > On Fri, 13 Jan 2017 15:54:24 GMT, hanno@hboeck.de wrote:
> > > > memcpy(foo, perl_args, 3 * sizeof(char *)); > > > > perl_parse(my_perl, 0, 3, foo, NULL);
> > > > > > From perlembed.pod: > > > > Mind that argv[argc] must be NULL, same as those passed to a main > > function in C. > > > > You need an extra NULL in your args list.
> > Is there any reason we _should_ require that, other than that it would require us to tighten up our argv/argc handling? It feels like a somewhat unperlish stipulation. > > If for example we're sometimes handing it off to libc calls that already require it then fair enough, but I'm not aware that we're doing anything like that.
We assign argv to PL_origargv, which is then available for the rest of core or XS modules to use as they wish; and some CPAN XS modules certainly seem to make use of it. They would have a reasonable expectation that the list is null-terminated. -- Counsellor Troi states something other than the blindingly obvious. -- Things That Never Happen in "Star Trek" #16
To: perlbug-followup [...] perl.org
Date: Sat, 14 Jan 2017 12:58:50 +0100
From: Hanno Böck <hanno [...] hboeck.de>
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
Download (untitled) / with headers
text/plain 362b
On Sat, 14 Jan 2017 03:34:43 -0800 "Dave Mitchell via RT" <perlbug-followup@perl.org> wrote: Show quoted text
> From perlembed.pod: > > Mind that argv[argc] must be NULL, same as those passed to a main > function in C. > > You need an extra NULL in your args list.
Ah, sorry... I thought I had checked the docs whether this is legit code, seems I didn't properly...
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
From: Hanno Böck <hanno [...] hboeck.de>
Date: Sat, 14 Jan 2017 10:34:56 +0100
To: perlbug-followup [...] perl.org
Download (untitled) / with headers
text/plain 696b
On Fri, 13 Jan 2017 18:21:01 -0800 "James E Keenan via RT" <perlbug-followup@perl.org> wrote: Show quoted text
> Making the change you suggested -- PL_origargv[0] -- and no other, I > attempted to build Perl 5 blead. 'make' died on both FreeBSD and > Linux at this point:
Yeah, you're right, it seems it's not as simple as I thought on first analysis. The code looks pretty confusing to me, it should probably be looked at by someone more familiar with it. It's definitely not correct to read invalid memory :-) But it seems that only happens when called from C, not when perl is directly called. -- Hanno Böck https://hboeck.de/ mail/jabber: hanno@hboeck.de GPG: FE73757FA60E4E21B937579FA5880072BBB51E42
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
From: Hanno Böck <hanno [...] hboeck.de>
To: perlbug-followup [...] perl.org
Date: Mon, 16 Jan 2017 11:02:01 +0100
Download (untitled) / with headers
text/plain 944b
On Sun, 15 Jan 2017 02:59:55 -0800 "Dave Mitchell via RT" <perlbug-followup@perl.org> wrote: Show quoted text
> We assign argv to PL_origargv, which is then available for the rest of > core or XS modules to use as they wish; and some CPAN XS modules > certainly seem to make use of it. They would have a reasonable > expectation that the list is null-terminated.
If there's an expectation by modules then I'd actually argue that it's better if perl core also reads the null terminator, because it makes it more likely that applications not supplying a null will be detected. I think there's nothing to do here and it was my mistake having misread the docs. (By the way: I found this bug because irssi had a non-null-terminated parameter list. I now send a patch to them which has just been applied at https://github.com/irssi/irssi/pull/619 ). -- Hanno Böck https://hboeck.de/ mail/jabber: hanno@hboeck.de GPG: FE73757FA60E4E21B937579FA5880072BBB51E42
Date: Mon, 16 Jan 2017 15:03:53 +0000
To: Hanno Böck <hanno [...] hboeck.de>
From: Dave Mitchell <davem [...] iabyn.com>
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
CC: perlbug-followup [...] perl.org
Download (untitled) / with headers
text/plain 461b
On Mon, Jan 16, 2017 at 11:02:01AM +0100, Hanno Böck wrote: Show quoted text
> If there's an expectation by modules then I'd actually argue that it's > better if perl core also reads the null terminator, because it makes it > more likely that applications not supplying a null will be detected.
In fact, perhaps parse_body() should have an explicit assert(!argv[argc]) to make it clearer. -- Modern art: "That's easy, I could have done that!" "Ah, but you didn't!"
To: perl5-porters [...] perl.org
Date: Mon, 18 Dec 2017 02:52:06 +0000
From: Zefram <zefram [...] fysh.org>
Subject: Re: [perl #130550] Out of bounds read when calling perl from C
Download (untitled) / with headers
text/plain 232b
The need for null argv[argc] was more fully documented in commit 0301e899536a22752f40481d8a1d141b7a7dda82, and has been asserted at runtime since commit cc85e83f9e22c43fcb37b072c8d9d20a3e8d9a64. This ticket can be closed. -zefram


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