unknown-crash in S_format_hexfp #16942
Comments
From @dur-randirCreated by @dur-randirWhile fuzzing perl v5.29.9-63-g2496d8f3f7 built with afl and run 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>..., This is a regression between 5.26 and 5.28, bisect points to: commit 50a7222 (HEAD, refs/bisect/bad) Perl_sv_vcatpvfn_flags: width/precis arg wrap When the width or precision is specified via an argument rather than Perl Info |
From @hvdsOn Mon, 08 Apr 2019 23:40:01 -0700, randir wrote:
We don't need wraparound, it's enough to provide the negative precision directly, with various outcomes: 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: 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 Patchdiff --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 { |
|
The RT System itself - Status changed from 'new' to 'open' |
From @hvdsOn Tue, 09 Apr 2019 04:47:13 -0700, hv wrote:
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 |
From @hvds0001-134008-More-carefully-ignore-negative-precision-in-s.patchFrom 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
|
From @tonycozOn Mon, 08 Apr 2019 23:40:01 -0700, randir wrote:
The problem in this case is that S_format_hexfp wasn't handling has_precis/precis I do think this is a security issue, though code that encounters it likely has other problems. Tony |
From @tonycoz0001-perl-134008-correct-precision-handling-in-S_format_h.patchFrom 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
|
From @tonycozOn Tue, 09 Apr 2019 07:01:25 -0700, hv wrote:
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 |
From @hvdsOn Mon, 15 Apr 2019 18:17:44 -0700, tonyc wrote:
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 |
From @iabynOn Tue, Apr 16, 2019 at 01:58:32AM -0700, Hugo van der Sanden via RT wrote:
I recommend we go with the hybrid as suggested by Hugo. I think we should -- |
From @tonycozOn Wed, 01 May 2019 05:36:38 -0700, davem wrote:
Tony |
|
@tonycoz - Status changed from 'open' to 'pending release' |
Migrated from rt.perl.org#134008 (status was 'pending release')
Searchable as RT134008$
The text was updated successfully, but these errors were encountered: