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

unknown-crash in S_format_hexfp #16942

Closed
p5pRT opened this issue Apr 9, 2019 · 12 comments
Closed

unknown-crash in S_format_hexfp #16942

p5pRT opened this issue Apr 9, 2019 · 12 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 9, 2019

Migrated from rt.perl.org#134008 (status was 'pending release')

Searchable as RT134008$

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2019

From @dur-randir

Created by @dur-randir

While fuzzing perl v5.29.9-63-g2496d8f3f7 built with afl and run
under libdislocator, I found the following progam

printf "%.*a", 1.84466502487128e+19,0

to cause unknown-crash write diagnostics by ASAN. GDB stack trace is following

S_format_hexfp (buf=0x555555b75b80 "0x0.", '0' <repeats 196 times>...,
bufsize=69, c=97 'a', nv=0, fv=0, has_precis=false,
precis=93824996751360, width=0,
  alt=false, plus=0 '\000', left=false, fill=false) at sv.c​:11792
11792 *p++ = '0';
(gdb) bt
#0 S_format_hexfp (buf=0x555555b75b80 "0x0.", '0' <repeats 196
times>..., bufsize=69, c=97 'a', nv=0, fv=0, has_precis=false,
precis=93824996751360,
  width=0, alt=false, plus=0 '\000', left=false, fill=false) at sv.c​:11792
#1 0x00005555557d6c1c in Perl_sv_vcatpvfn_flags (sv=0x555555b4e740,
pat=0x555555b75b10 "%.*a", patlen=4, args=0x0, svargs=0x555555b50db0,
sv_count=2,
  maybe_tainted=0x7fffffffdd97, flags=0) at sv.c​:13093
#2 0x00005555557cff0e in Perl_sv_vsetpvfn (sv=0x555555b4e740,
pat=0x555555b75b10 "%.*a", patlen=4, args=0x0, svargs=0x555555b50db0,
sv_count=2,
  maybe_tainted=0x7fffffffdd97) at sv.c​:10977
#3 0x000055555587eb55 in Perl_do_sprintf (sv=0x555555b4e740, len=3,
sarg=0x555555b50da8) at doop.c​:734
#4 0x000055555585cad0 in Perl_pp_prtf () at pp_sys.c​:1628
#5 0x000055555570b895 in Perl_runops_debug () at dump.c​:2537
#6 0x00005555555ed560 in S_run_body (oldscope=1) at perl.c​:2716
#7 0x00005555555ecade in perl_run (my_perl=0x555555b4c260) at perl.c​:2639
#8 0x00005555555a114e in main (argc=3, argv=0x7fffffffe1a8,
env=0x7fffffffe1c8) at perlmain.c​:127

This is a regression between 5.26 and 5.28, bisect points to​:

commit 50a7222 (HEAD, refs/bisect/bad)
Author​: David Mitchell <davem@​iabyn.com>
Date​: Tue May 30 13​:45​:35 2017 +0100

  Perl_sv_vcatpvfn_flags​: width/precis arg wrap

  When the width or precision is specified via an argument rather than
  literally, check whether the value wraps.

Perl Info

Flags:
    category=core
    severity=high

Site configuration information for perl 5.29.9:

Configured by dur-randir at Wed Feb 27 14:51:01 MSK 2019.

Summary of my perl5 (revision 5 version 29 subversion 9) configuration:
  Commit id: c1e47bad34ce1d9c84ed57c9b8978bcbd5a02e98
  Platform:
    osname=darwin
    osvers=13.4.0
    archname=darwin-thread-multi-2level
    uname='darwin isengard.local 13.4.0 darwin kernel version 13.4.0:
mon jan 11 18:17:34 pst 2016; root:xnu-2422.115.15~1release_x86_64
x86_64 '
    config_args='-de -Dusedevel -DDEBUGGING -Dusethreads'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9
-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include -DPERL_USE_SAFE_PUTENV'
    optimize='-O3 -g'
    cppflags='-fno-common -DPERL_DARWIN -mmacosx-version-min=10.9
-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector
-I/usr/local/include'
    ccversion=''
    gccversion='4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)'
    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='cc'
    ldflags =' -mmacosx-version-min=10.9 -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/clang/6.0/lib
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib
/usr/lib
    libs=-lpthread -lgdbm -ldbm -ldl -lm -lutil -lc
    perllibs=-lpthread -ldl -lm -lutil -lc
    libc=
    so=dylib
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=bundle
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags=' -mmacosx-version-min=10.9 -bundle -undefined
dynamic_lookup -L/usr/local/lib -fstack-protector'



@INC for perl 5.29.9:
    lib
    /usr/local/lib/perl5/site_perl/5.29.9/darwin-thread-multi-2level
    /usr/local/lib/perl5/site_perl/5.29.9
    /usr/local/lib/perl5/5.29.9/darwin-thread-multi-2level
    /usr/local/lib/perl5/5.29.9


Environment for perl 5.29.9:
    DYLD_LIBRARY_PATH (unset)
    HOME=/Users/dur-randir
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/Users/dur-randir/perlbrew/bin:/Users/dur-randir/perlbrew/perls/perl-5.22.1/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/texbin
    PERLBREW_HOME=/Users/dur-randir/.perlbrew
    PERLBREW_MANPATH=/Users/dur-randir/perlbrew/perls/perl-5.22.1/man
    PERLBREW_PATH=/Users/dur-randir/perlbrew/bin:/Users/dur-randir/perlbrew/perls/perl-5.22.1/bin
    PERLBREW_PERL=perl-5.22.1
    PERLBREW_ROOT=/Users/dur-randir/perlbrew
    PERLBREW_SHELLRC_VERSION=0.84
    PERLBREW_VERSION=0.84
    PERL_BADLANG (unset)
    SHELL=/usr/local/bin/zsh

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2019

From @hvds

On Mon, 08 Apr 2019 23​:40​:01 -0700, randir wrote​:

While fuzzing perl v5.29.9-63-g2496d8f3f7 built with afl and run
under libdislocator, I found the following progam

printf "%.*a", 1.84466502487128e+19,0

to cause unknown-crash write diagnostics by ASAN. GDB stack trace is
following

We don't need wraparound, it's enough to provide the negative precision directly, with various outcomes​:
  % ./miniperl -e 'sprintf "%.*a", -0x18800,0'
  Segmentation fault (core dumped)
  % ./miniperl -e 'sprintf "%.*a", -0x18400,0'
  *** Error in `./miniperl'​: realloc()​: invalid next size​: 0x000000000167cb10 ***
  Aborted (core dumped)
  % /miniperl -e 'sprintf "%.*a", -0x40,0'
  panic​: snprintf buffer overflow at -e line 1.
  %

I'm not aware of any intended meaning for a negative precision, but in the sv_vcatpvfn_flags() code we set 'precis' to the absolute value while leaving 'has_precis' false. Most of the rest then ignores precis unless has_precis, but this code in S_format_hexfp() uses it, as does the tail code in sv_vcatpvfn_flags() after the comment​:
  /* append esignbuf, filler, zeros, eptr and dotstr to sv */

The trivial patch below protects from this core dump and passes tests, but I think we need to consider the potential impact here carefully, as well as clean up and clarify the code.

Hugo

Inline Patch
diff --git a/sv.c b/sv.c
index b6d9123..fe26d13 100644
--- a/sv.c
+++ b/sv.c
@@ -12215,7 +12215,10 @@ Perl_sv_vcatpvfn_flags(pTHX_ SV *const sv, const char *const pat, const STRLEN p
                                           : (arg_missing = TRUE, (SV*)NULL);
                     }
                     precis = S_sprintf_arg_num_val(aTHX_ args, i, sv, &neg);
-                    has_precis = !neg;
+                    if (neg)
+                        precis = 0;
+                    else
+                        has_precis = true;
                 }
            }
            else {

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2019

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

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2019

From @hvds

On Tue, 09 Apr 2019 04​:47​:13 -0700, hv wrote​:

the tail code in sv_vcatpvfn_flags() after the comment​:
/* append esignbuf, filler, zeros, eptr and dotstr to sv */

It turns out the only use here is if 'zeros' is true, which is only set in a case where has_precis is true.

So as far as I can tell, the only problem here is for hexfp, and the payload is to overwrite a number of bytes beyond the PL_efloatbuf buffer with '0' characters, the number being controlled by a sprintf argument.

As such I think the risk here is low - if a user can provide the sprintf pattern, you've probably already lost, and likelihood of a '%.*a' pattern with user-supplied arguments seems remote. Nothing in the test suite uses negative precision except specific sprintf{,2} tests asserting it should be ignored (in the absence of which I'd be tempted to make it fatal instead).

So I propose that we commit the attached belt-and-braces patch, open the ticket, and consider this for backporting. I'd welcome other opinions.

Hugo

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2019

From @hvds

0001-134008-More-carefully-ignore-negative-precision-in-s.patch
From 6a3439c5a14d6026d16995e539c44f58e52a102a Mon Sep 17 00:00:00 2001
From: Hugo van der Sanden <hv@crypt.org>
Date: Tue, 9 Apr 2019 14:27:41 +0100
Subject: [PATCH] [#134008] More carefully ignore negative precision in sprintf

Check has_precis more consistently; ensure precis is left as 0 if provided
as a negative number.
---
 sv.c            | 7 +++++--
 t/op/sprintf2.t | 3 +++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/sv.c b/sv.c
index b6d9123..611a9f4 100644
--- a/sv.c
+++ b/sv.c
@@ -11758,11 +11758,11 @@ S_format_hexfp(pTHX_ char * const buf, const STRLEN bufsize, const char c,
     else {
         *p++ = '0';
         exponent = 0;
-        zerotail = precis;
+        zerotail = has_precis ? precis : 0;
     }
 
     /* The radix is always output if precis, or if alt. */
-    if (precis > 0 || alt) {
+    if ((has_precis && precis > 0) || alt) {
       hexradix = TRUE;
     }
 
@@ -12216,6 +12216,9 @@ Perl_sv_vcatpvfn_flags(pTHX_ SV *const sv, const char *const pat, const STRLEN p
                     }
                     precis = S_sprintf_arg_num_val(aTHX_ args, i, sv, &neg);
                     has_precis = !neg;
+                    /* ignore negative precision */
+                    if (!has_precis)
+                        precis = 0;
                 }
 	    }
 	    else {
diff --git a/t/op/sprintf2.t b/t/op/sprintf2.t
index 3f4c126..891eb05 100644
--- a/t/op/sprintf2.t
+++ b/t/op/sprintf2.t
@@ -830,6 +830,9 @@ SKIP: {
     # [rt.perl.org #128889]
     is(sprintf("%.*a", -1, 1.03125), "0x1.08p+0", "[rt.perl.org #128889]");
 
+    # [rt.perl.org #134008]
+    is(sprintf("%.*a", -99999, 1.03125), "0x1.08p+0", "[rt.perl.org #134008]");
+
     # [rt.perl.org #128890]
     is(sprintf("%a", 0x1.18p+0), "0x1.18p+0");
     is(sprintf("%.1a", 0x1.08p+0), "0x1.0p+0");
-- 
2.10.2

@p5pRT
Copy link
Author

p5pRT commented Apr 16, 2019

From @tonycoz

On Mon, 08 Apr 2019 23​:40​:01 -0700, randir wrote​:

This is a bug report for perl from sergey.aleynikov@​gmail.com,
generated with the help of perlbug 1.41 running under perl 5.29.9.

-----------------------------------------------------------------
[Please describe your issue here]

While fuzzing perl v5.29.9-63-g2496d8f3f7 built with afl and run
under libdislocator, I found the following progam

printf "%.*a", 1.84466502487128e+19,0

to cause unknown-crash write diagnostics by ASAN. GDB stack trace is
following

S_format_hexfp (buf=0x555555b75b80 "0x0.", '0' <repeats 196 times>...,
bufsize=69, c=97 'a', nv=0, fv=0, has_precis=false,
precis=93824996751360, width=0,
alt=false, plus=0 '\000', left=false, fill=false) at sv.c​:11792
11792 *p++ = '0';
(gdb) bt
#0 S_format_hexfp (buf=0x555555b75b80 "0x0.", '0' <repeats 196
times> ..., bufsize=69, c=97 'a', nv=0, fv=0, has_precis=false,
precis=93824996751360,
width=0, alt=false, plus=0 '\000', left=false, fill=false) at
sv.c​:11792
#1 0x00005555557d6c1c in Perl_sv_vcatpvfn_flags (sv=0x555555b4e740,
pat=0x555555b75b10 "%.*a", patlen=4, args=0x0, svargs=0x555555b50db0,
sv_count=2,
maybe_tainted=0x7fffffffdd97, flags=0) at sv.c​:13093
#2 0x00005555557cff0e in Perl_sv_vsetpvfn (sv=0x555555b4e740,
pat=0x555555b75b10 "%.*a", patlen=4, args=0x0, svargs=0x555555b50db0,
sv_count=2,
maybe_tainted=0x7fffffffdd97) at sv.c​:10977
#3 0x000055555587eb55 in Perl_do_sprintf (sv=0x555555b4e740, len=3,
sarg=0x555555b50da8) at doop.c​:734
#4 0x000055555585cad0 in Perl_pp_prtf () at pp_sys.c​:1628
#5 0x000055555570b895 in Perl_runops_debug () at dump.c​:2537
#6 0x00005555555ed560 in S_run_body (oldscope=1) at perl.c​:2716
#7 0x00005555555ecade in perl_run (my_perl=0x555555b4c260) at
perl.c​:2639
#8 0x00005555555a114e in main (argc=3, argv=0x7fffffffe1a8,
env=0x7fffffffe1c8) at perlmain.c​:127

This is a regression between 5.26 and 5.28, bisect points to​:

commit 50a7222 (HEAD,
refs/bisect/bad)
Author​: David Mitchell <davem@​iabyn.com>
Date​: Tue May 30 13​:45​:35 2017 +0100

Perl_sv_vcatpvfn_flags​: width/precis arg wrap

When the width or precision is specified via an argument rather than
literally, check whether the value wraps.

The problem in this case is that S_format_hexfp wasn't handling has_precis/precis
correctly.

I do think this is a security issue, though code that encounters it likely has other problems.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 16, 2019

From @tonycoz

0001-perl-134008-correct-precision-handling-in-S_format_h.patch
From 48a6726d70db547ac5edaedba32d02ed028b9e70 Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 16 Apr 2019 10:36:45 +1000
Subject: (perl #134008) correct precision handling in S_format_hexfp

A negative precision via .* is intended to be treated as if no
precision value was supplied at all, and the code in
sv_vcatpvfn_flags() handles that correctly, unfortunately
S_format_hexfp continued to use the precis value even when has_precis
is false and since precis isn't used in calculating the buffer size
when has_precis is false, this can lead to a buffer overflow.
---
 sv.c            | 4 ++--
 t/op/sprintf2.t | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/sv.c b/sv.c
index 9b659e8c16..812ed65b1a 100644
--- a/sv.c
+++ b/sv.c
@@ -11759,11 +11759,11 @@ S_format_hexfp(pTHX_ char * const buf, const STRLEN bufsize, const char c,
     else {
         *p++ = '0';
         exponent = 0;
-        zerotail = precis;
+        zerotail = has_precis ? precis : 0;
     }
 
     /* The radix is always output if precis, or if alt. */
-    if (precis > 0 || alt) {
+    if ((has_precis && precis > 0) || alt) {
       hexradix = TRUE;
     }
 
diff --git a/t/op/sprintf2.t b/t/op/sprintf2.t
index 3f4c126c68..590273e8a2 100644
--- a/t/op/sprintf2.t
+++ b/t/op/sprintf2.t
@@ -1141,4 +1141,13 @@ foreach(
     is sprintf("%.0f", $_), sprintf("%-.0f", $_), "special-case %.0f on $_";
 }
 
+{
+    # 134008
+    # the large number in the test case from the ticket might be
+    # interpreted differently on 32-bit platforms, so use a literal
+    # negative number that reproduces the problem
+    fresh_perl_is('printf "%.*a", -100000,0', '0x0p+0',
+                  {}, 'negative precision ignored by format_hexfp');
+}
+
 done_testing();
-- 
2.11.0

@p5pRT
Copy link
Author

p5pRT commented Apr 16, 2019

From @tonycoz

On Tue, 09 Apr 2019 07​:01​:25 -0700, hv wrote​:

On Tue, 09 Apr 2019 04​:47​:13 -0700, hv wrote​:

the tail code in sv_vcatpvfn_flags() after the comment​:
/* append esignbuf, filler, zeros, eptr and dotstr to sv */

It turns out the only use here is if 'zeros' is true, which is only
set in a case where has_precis is true.

So as far as I can tell, the only problem here is for hexfp, and the
payload is to overwrite a number of bytes beyond the PL_efloatbuf
buffer with '0' characters, the number being controlled by a sprintf
argument.

As such I think the risk here is low - if a user can provide the
sprintf pattern, you've probably already lost, and likelihood of a
'%.*a' pattern with user-supplied arguments seems remote. Nothing in
the test suite uses negative precision except specific sprintf{,2}
tests asserting it should be ignored (in the absence of which I'd be
tempted to make it fatal instead).

So I propose that we commit the attached belt-and-braces patch, open
the ticket, and consider this for backporting. I'd welcome other
opinions.

Somehow I didn't see Hugo's response (and his patch).

An attacker doesn't need to supply a format string, they just need to be able to supply a negative precision, and it doesn't need to be large in magnitude.

I could see a reporting tool allowing specifying width/precision for fields, though perhaps not so much supporting %a formatting.

A case could be made that such a tool is buggy if it permits very large or negative precisions, but that's irrelevant as to whether a bug in perl becomes a security issue for such code.

The behaviour for negative precision comes from the C standard​:

  A negative precision argument is taken as if the precision were omitted.

which presumably is what the current implementation is intended to emulate (especially since PerlIO_printf() uses this code.

Tony

@p5pRT
Copy link
Author

p5pRT commented Apr 16, 2019

From @hvds

On Mon, 15 Apr 2019 18​:17​:44 -0700, tonyc wrote​:

On Tue, 09 Apr 2019 07​:01​:25 -0700, hv wrote​:

On Tue, 09 Apr 2019 04​:47​:13 -0700, hv wrote​:

the tail code in sv_vcatpvfn_flags() after the comment​:
/* append esignbuf, filler, zeros, eptr and dotstr to sv */

It turns out the only use here is if 'zeros' is true, which is only
set in a case where has_precis is true.

So as far as I can tell, the only problem here is for hexfp, and the
payload is to overwrite a number of bytes beyond the PL_efloatbuf
buffer with '0' characters, the number being controlled by a sprintf
argument.

As such I think the risk here is low - if a user can provide the
sprintf pattern, you've probably already lost, and likelihood of a
'%.*a' pattern with user-supplied arguments seems remote. Nothing in
the test suite uses negative precision except specific sprintf{,2}
tests asserting it should be ignored (in the absence of which I'd be
tempted to make it fatal instead).

So I propose that we commit the attached belt-and-braces patch, open
the ticket, and consider this for backporting. I'd welcome other
opinions.

Somehow I didn't see Hugo's response (and his patch).

An attacker doesn't need to supply a format string, they just need to
be able to supply a negative precision, and it doesn't need to be
large in magnitude.

I could see a reporting tool allowing specifying width/precision for
fields, though perhaps not so much supporting %a formatting.

A case could be made that such a tool is buggy if it permits very
large or negative precisions, but that's irrelevant as to whether a
bug in perl becomes a security issue for such code.

The behaviour for negative precision comes from the C standard​:

A negative precision argument is taken as if the precision were
omitted.

which presumably is what the current implementation is intended to
emulate (especially since PerlIO_printf() uses this code.

No problem with your analysis, I did request other opinions.

I'd recommend adding the extra change in sv_vcatpvfn_flags from my patch. Your test looks like the better one though.

Hugo

@p5pRT
Copy link
Author

p5pRT commented May 1, 2019

From @iabyn

On Tue, Apr 16, 2019 at 01​:58​:32AM -0700, Hugo van der Sanden via RT wrote​:

On Mon, 15 Apr 2019 18​:17​:44 -0700, tonyc wrote​:

On Tue, 09 Apr 2019 07​:01​:25 -0700, hv wrote​:

On Tue, 09 Apr 2019 04​:47​:13 -0700, hv wrote​:

the tail code in sv_vcatpvfn_flags() after the comment​:
/* append esignbuf, filler, zeros, eptr and dotstr to sv */

It turns out the only use here is if 'zeros' is true, which is only
set in a case where has_precis is true.

So as far as I can tell, the only problem here is for hexfp, and the
payload is to overwrite a number of bytes beyond the PL_efloatbuf
buffer with '0' characters, the number being controlled by a sprintf
argument.

As such I think the risk here is low - if a user can provide the
sprintf pattern, you've probably already lost, and likelihood of a
'%.*a' pattern with user-supplied arguments seems remote. Nothing in
the test suite uses negative precision except specific sprintf{,2}
tests asserting it should be ignored (in the absence of which I'd be
tempted to make it fatal instead).

So I propose that we commit the attached belt-and-braces patch, open
the ticket, and consider this for backporting. I'd welcome other
opinions.

Somehow I didn't see Hugo's response (and his patch).

An attacker doesn't need to supply a format string, they just need to
be able to supply a negative precision, and it doesn't need to be
large in magnitude.

I could see a reporting tool allowing specifying width/precision for
fields, though perhaps not so much supporting %a formatting.

A case could be made that such a tool is buggy if it permits very
large or negative precisions, but that's irrelevant as to whether a
bug in perl becomes a security issue for such code.

The behaviour for negative precision comes from the C standard​:

A negative precision argument is taken as if the precision were
omitted.

which presumably is what the current implementation is intended to
emulate (especially since PerlIO_printf() uses this code.

No problem with your analysis, I did request other opinions.

I'd recommend adding the extra change in sv_vcatpvfn_flags from my patch. Your test looks like the better one though.

I recommend we go with the hybrid as suggested by Hugo. I think we should
add it to blead, and I don't think we need to treat it as a serious
security issue. The string '%.*a' as part of a format appears nowhere on
cpan.

--
Overhead, without any fuss, the stars were going out.
  -- Arthur C Clarke

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2019

From @tonycoz

On Wed, 01 May 2019 05​:36​:38 -0700, davem wrote​:

On Tue, Apr 16, 2019 at 01​:58​:32AM -0700, Hugo van der Sanden via RT
wrote​:

On Mon, 15 Apr 2019 18​:17​:44 -0700, tonyc wrote​:

On Tue, 09 Apr 2019 07​:01​:25 -0700, hv wrote​:

On Tue, 09 Apr 2019 04​:47​:13 -0700, hv wrote​:

the tail code in sv_vcatpvfn_flags() after the comment​:
/* append esignbuf, filler, zeros, eptr and dotstr to sv */

It turns out the only use here is if 'zeros' is true, which is
only
set in a case where has_precis is true.

So as far as I can tell, the only problem here is for hexfp, and
the
payload is to overwrite a number of bytes beyond the PL_efloatbuf
buffer with '0' characters, the number being controlled by a
sprintf
argument.

As such I think the risk here is low - if a user can provide the
sprintf pattern, you've probably already lost, and likelihood of
a
'%.*a' pattern with user-supplied arguments seems remote. Nothing
in
the test suite uses negative precision except specific
sprintf{,2}
tests asserting it should be ignored (in the absence of which I'd
be
tempted to make it fatal instead).

So I propose that we commit the attached belt-and-braces patch,
open
the ticket, and consider this for backporting. I'd welcome other
opinions.

Somehow I didn't see Hugo's response (and his patch).

An attacker doesn't need to supply a format string, they just need
to
be able to supply a negative precision, and it doesn't need to be
large in magnitude.

I could see a reporting tool allowing specifying width/precision
for
fields, though perhaps not so much supporting %a formatting.

A case could be made that such a tool is buggy if it permits very
large or negative precisions, but that's irrelevant as to whether a
bug in perl becomes a security issue for such code.

The behaviour for negative precision comes from the C standard​:

A negative precision argument is taken as if the precision were
omitted.

which presumably is what the current implementation is intended to
emulate (especially since PerlIO_printf() uses this code.

No problem with your analysis, I did request other opinions.

I'd recommend adding the extra change in sv_vcatpvfn_flags from my
patch. Your test looks like the better one though.

I recommend we go with the hybrid as suggested by Hugo. I think we
should
add it to blead, and I don't think we need to treat it as a serious
security issue. The string '%.*a' as part of a format appears nowhere
on
cpan.

Done in b0f5b1d, 9dfe0a3.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 3, 2019

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

@p5pRT p5pRT closed this as completed Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant