Out of bounds read when calling perl from C

[p5pRT]

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

Searchable as RT130550$

[p5pRT]

From hanno@hboeck.de

Created by hanno@hboeck.de

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.

Perl Info

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

[p5pRT]

From @jkeenan

On Fri, 13 Jan 2017 15​:54​:24 GMT, hanno@​hboeck.de wrote​:

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)

[p5pRT]

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

[p5pRT]

From @hvds

On Fri, 13 Jan 2017 07​:54​:24 -0800, hanno@​hboeck.de wrote​:

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.

Inline Patch

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

[p5pRT]

From @iabyn

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

[p5pRT]

From @hvds

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.

Hugo

[p5pRT]

From @iabyn

On Sat, Jan 14, 2017 at 12​:11​:02PM -0800, Hugo van der Sanden via RT wrote​:

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

[p5pRT]

From hanno@hboeck.de

On Sat, 14 Jan 2017 03​:34​:43 -0800
"Dave Mitchell via RT" <perlbug-followup@​perl.org> wrote​:

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...

[p5pRT]

From hanno@hboeck.de

On Fri, 13 Jan 2017 18​:21​:01 -0800
"James E Keenan via RT" <perlbug-followup@​perl.org> wrote​:

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

[p5pRT]

From hanno@hboeck.de

On Sun, 15 Jan 2017 02​:59​:55 -0800
"Dave Mitchell via RT" <perlbug-followup@​perl.org> wrote​:

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 irssi/irssi#619 ).

--
Hanno Böck
https://hboeck.de/

mail/jabber​: hanno@​hboeck.de
GPG​: FE73757FA60E4E21B937579FA5880072BBB51E42

[p5pRT]

From @iabyn

On Mon, Jan 16, 2017 at 11​:02​:01AM +0100, Hanno Böck wrote​:

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

[p5pRT]

From zefram@fysh.org

The need for null argv[argc] was more fully documented in commit
0301e89, and has been asserted at runtime
since commit cc85e83. This ticket can
be closed.

-zefram

[p5pRT]

@iabyn - Status changed from 'open' to 'resolved'