Re: Call for advice regarding curl CVE-2016-9586
Hi,
2016-12-28 11:59 GMT+01:00 Ola Lundqvist <ola@inguza.com>:
> Thank you.
>
> It was added to dla-needed.txt one or two days ago.
I'm in the process of uploading the fixed packaga.
For the record curl_mprintf() is formatting floating point values in a
buggy way in
Wheezy's version and I have adapted the added tests to that.
The buggy formatting however did not prevent the stack-corruption from
occuring thus the fix was needed. :-)
Cheers,
Balint
>
> / Ola
>
> Sent from a phone
>
> Den 27 dec 2016 22:37 skrev "Antoine Beaupré" <anarcat@orangeseeds.org>:
>>
>> On 2016-12-23 17:54:11, Ola Lundqvist wrote:
>> > Hi
>> >
>> > I have looked into CVE-2016-9586 affecting curl.
>> > What I'm trying to figure out is whether it is worth the effort to fix
>> > it or not.
>> >
>> > More info here:
>> > https://curl.haxx.se/docs/adv_20161221A.html
>> >
>> > 1) There are no known exploits -> minor issue (?)
>>
>> "No known exploits" is mostly irrelevant, the severity of the issue
>> is. In this case, a buffer overflow is severe enough to warrant action,
>> in my opinion.
>>
>> > 2) The functions have been documented as deprecated for a long time
>>
>> Considering how old the software in wheezy is, this may mean we still
>> have some of those tools. :)
>>
>> > 3) The problem only occur on applications without proper input
>> > sanitizing (and using curl_mprintf) so one could even argue that this
>> > is not really a fault in curl at all.
>>
>> This I am more convinced by: it's the format string, not the argument,
>> so it's less likely to be an attack vector. But as guido said, we can't
>> review all the instances and we should fix this anyways.
>>
>> A.
>> --
>> A man is none the less a slave because he is allowed to choose a new
>> master once in a term of years.
>> - Lysander Spooner
diff -Nru curl-7.26.0/debian/changelog curl-7.26.0/debian/changelog
--- curl-7.26.0/debian/changelog 2016-11-10 17:50:13.000000000 +0100
+++ curl-7.26.0/debian/changelog 2016-12-28 23:19:19.000000000 +0100
@@ -1,3 +1,17 @@
+curl (7.26.0-1+wheezy18) wheezy-security; urgency=medium
+
+ * Non-maintainer upload by the LTS Team.
+ * CVE-2016-9586
+ libcurl's implementation of the printf() functions triggers a buffer
+ overflow when doing a large floating point output. The bug occurs
+ when the conversion outputs more than 255 bytes.
+ If there are any application that accepts a format string from the outside
+ without necessary input filtering, it could allow remote attacks.
+ This flaw does not exist in the command line tool.
+ (Closes: #848958)
+
+ -- Balint Reczey <balint@balintreczey.hu> Wed, 28 Dec 2016 17:23:47 +0100
+
curl (7.26.0-1+wheezy17) wheezy-security; urgency=high
* Non-maintainer upload by the LTS Team.
diff -Nru curl-7.26.0/debian/patches/CVE-2016-9586.patch curl-7.26.0/debian/patches/CVE-2016-9586.patch
--- curl-7.26.0/debian/patches/CVE-2016-9586.patch 1970-01-01 01:00:00.000000000 +0100
+++ curl-7.26.0/debian/patches/CVE-2016-9586.patch 2016-12-29 01:47:22.000000000 +0100
@@ -0,0 +1,247 @@
+From 5b988c081e6224b2202f114a01742f76dea27c42 Mon Sep 17 00:00:00 2001
+From: Daniel Stenberg <daniel@haxx.se>
+Date: Tue, 8 Nov 2016 15:32:37 +0100
+Subject: [PATCH] printf: fix floating point buffer overflow issues
+
+... and add a bunch of floating point printf tests
+
+Back-ported to curl 7.26 by Balint Reczey <balint@balintreczey.hu>
+
+Conflicts:
+ tests/data/test557
+ tests/libtest/lib557.c
+---
+ lib/mprintf.c | 20 ++++++-
+ tests/data/test557 | 1 +
+ tests/libtest/lib557.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
+ 3 files changed, 159 insertions(+), 2 deletions(-)
+
+--- a/lib/mprintf.c
++++ b/lib/mprintf.c
+@@ -84,7 +84,8 @@
+ # define mp_uintmax_t unsigned long
+ #endif
+
+-#define BUFFSIZE 256 /* buffer for long-to-str and float-to-str calcs */
++#define BUFFSIZE 326 /* buffer for long-to-str and float-to-str calcs, should
++ fit negative DBL_MAX (317 letters) */
+ #define MAX_PARAMETERS 128 /* lame static limit */
+
+ #ifdef __AMIGA__
+@@ -947,12 +948,25 @@
+ fptr=&formatbuf[strlen(formatbuf)];
+
+ if(width >= 0) {
++ if(width >= (long)sizeof(work))
++ width = sizeof(work)-1;
+ /* RECURSIVE USAGE */
+ len = curl_msnprintf(fptr, left, "%ld", width);
+ fptr += len;
+ left -= len;
+ }
+ if(prec >= 0) {
++ /* for each digit in the integer part, we can have one less
++ precision */
++ size_t maxprec = sizeof(work) - 2;
++ double val = p->data.dnum;
++ while(val >= 10.0) {
++ val /= 10;
++ maxprec--;
++ }
++
++ if(prec > (long)maxprec)
++ prec = maxprec-1;
+ /* RECURSIVE USAGE */
+ len = curl_msnprintf(fptr, left, ".%ld", prec);
+ fptr += len;
+@@ -972,7 +986,9 @@
+ /* NOTE NOTE NOTE!! Not all sprintf() implementations returns number
+ of output characters */
+ (sprintf)(work, formatbuf, p->data.dnum);
+-
++#ifdef CURLDEBUG
++ assert(strlen(work) <= sizeof(work));
++#endif
+ for(fptr=work; *fptr; fptr++)
+ OUTCHAR(*fptr);
+ }
+--- a/tests/data/test557
++++ b/tests/data/test557
+@@ -33,6 +33,7 @@
+ All curl_mprintf() unsigned long tests OK!
+ All curl_mprintf() signed long tests OK!
+ All curl_mprintf() curl_off_t tests OK!
++All float strings tests OK!
+ </stdout>
+ </verify>
+
+--- a/tests/libtest/lib557.c
++++ b/tests/libtest/lib557.c
+@@ -1374,6 +1374,158 @@
+ return failed;
+ }
+
++static int _string_check(int linenumber, char *buf, const char *buf2)
++{
++ if(strcmp(buf, buf2)) {
++ /* they shouldn't differ */
++ printf("sprintf line %d failed:\nwe '%s'\nsystem: '%s'\n",
++ linenumber, buf, buf2);
++ return 1;
++ }
++ return 0;
++}
++#define string_check(x,y) _string_check(__LINE__, x, y)
++
++static int _strlen_check(int linenumber, char *buf, size_t len)
++{
++ size_t buflen = strlen(buf);
++ if(len != buflen) {
++ /* they shouldn't differ */
++ printf("sprintf strlen:%d failed:\nwe '%d'\nsystem: '%d'\n",
++ linenumber, buflen, len);
++ return 1;
++ }
++ return 0;
++}
++
++#define strlen_check(x,y) _strlen_check(__LINE__, x, y)
++
++/* DBL_MAX value from Linux */
++#define MAXIMIZE -179769313486231570814527423731704356798070567525844996598917476803157260780028538760589558632766878171540458953514382464234321326889464182768467546703537516986049910576551282076245490090389328944075868508455133942304583236903222948165808559332123348274797826204144723168738177180919299881250404026184124858368.000000
++
++static int test_float_formatting(void)
++{
++ int errors = 0;
++ char buf[512]; /* larger than max float size */
++ curl_msnprintf(buf, sizeof(buf), "%f", 9.0);
++ errors += string_check(buf, "9.000000");
++
++ curl_msnprintf(buf, sizeof(buf), "%.1f", 9.1);
++ errors += string_check(buf, "9.1");
++
++ curl_msnprintf(buf, sizeof(buf), "%.2f", 9.1);
++ errors += string_check(buf, "9.10");
++
++ curl_msnprintf(buf, sizeof(buf), "%.0f", 9.1);
++ errors += string_check(buf, "9");
++
++ curl_msnprintf(buf, sizeof(buf), "%0f", 9.1);
++ errors += string_check(buf, "9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%10f", 9.1);
++ errors += string_check(buf, " 9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%10.3f", 9.1);
++ errors += string_check(buf, " 9.100");
++
++ curl_msnprintf(buf, sizeof(buf), "%-10.3f", 9.1);
++ errors += string_check(buf, "9.100 ");
++
++ curl_msnprintf(buf, sizeof(buf), "%-10.3f", 9.123456);
++ errors += string_check(buf, "9.123 ");
++
++ curl_msnprintf(buf, sizeof(buf), "%.-2f", 9.1);
++ errors += string_check(buf, "9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%*f", 10, 9.1);
++ errors += string_check(buf, " 9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%*f", 3, 9.1);
++ errors += string_check(buf, "9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%*f", 6, 9.2987654);
++ errors += string_check(buf, "9.298765");
++
++ curl_msnprintf(buf, sizeof(buf), "%*f", 6, 9.298765);
++ errors += string_check(buf, "9.298765");
++
++ curl_msnprintf(buf, sizeof(buf), "%*f", 6, 9.29876);
++ errors += string_check(buf, "9.298760");
++
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 6, 9.2987654);
++ // buggy in 7.26
++ // errors += string_check(buf, "9.298765");
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 5, 9.2987654);
++ // buggy in 7.26
++ // errors += string_check(buf, "9.29877");
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 4, 9.2987654);
++ // buggy in 7.26
++ // errors += string_check(buf, "9.2988");
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 3, 9.2987654);
++ // buggy in 7.26
++ // errors += string_check(buf, "9.299");
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 2, 9.2987654);
++ // buggy in 7.26
++ // errors += string_check(buf, "9.30");
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 1, 9.2987654);
++ errors += string_check(buf, "9.3");
++ curl_msnprintf(buf, sizeof(buf), "%.*f", 0, 9.2987654);
++ // buggy in 7.26
++ //errors += string_check(buf, "9");
++ errors += string_check(buf, "9.3");
++
++ /* very large precisions easily turn into system specific outputs so we only
++ check the output buffer length here as we know the internal limit */
++
++ curl_msnprintf(buf, sizeof(buf), "%.*f", (1<<30), 9.2987654);
++ // buggy in 7.26
++ // errors += strlen_check(buf, 325);
++ errors += strlen_check(buf, 3);
++
++ curl_msnprintf(buf, sizeof(buf), "%10000.10000f", 9.2987654);
++ errors += strlen_check(buf, 325);
++
++ curl_msnprintf(buf, sizeof(buf), "%240.10000f",
++ 123456789123456789123456789.2987654);
++ errors += strlen_check(buf, 325);
++
++ /* 1<<31 turns negative (-2147483648) when used signed */
++ curl_msnprintf(buf, sizeof(buf), "%*f", (1<<31), 9.1);
++ errors += string_check(buf, "9.100000");
++
++ /* curl_msnprintf() limits a single float output to 325 bytes maximum
++ width */
++ curl_msnprintf(buf, sizeof(buf), "%*f", (1<<30), 9.1);
++ errors += string_check(buf, " 9.100000");
++ curl_msnprintf(buf, sizeof(buf), "%100000f", 9.1);
++ errors += string_check(buf, " 9.100000");
++
++ curl_msnprintf(buf, sizeof(buf), "%f", MAXIMIZE);
++ errors += strlen_check(buf, 317);
++
++ curl_msnprintf(buf, 2, "%f", MAXIMIZE);
++ errors += strlen_check(buf, 1);
++ curl_msnprintf(buf, 3, "%f", MAXIMIZE);
++ errors += strlen_check(buf, 2);
++ curl_msnprintf(buf, 4, "%f", MAXIMIZE);
++ errors += strlen_check(buf, 3);
++ curl_msnprintf(buf, 5, "%f", MAXIMIZE);
++ errors += strlen_check(buf, 4);
++ curl_msnprintf(buf, 6, "%f", MAXIMIZE);
++ errors += strlen_check(buf, 5);
++
++ if(!errors)
++ printf("All float strings tests OK!\n");
++ else
++ printf("test_float_formatting Failed!\n");
++
++ return errors;
++}
+
+ int test(char *URL)
+ {
+@@ -1394,6 +1546,8 @@
+
+ errors += test_curl_off_t_formatting();
+
++ errors += test_float_formatting();
++
+ if(errors)
+ return TEST_ERR_MAJOR_BAD;
+ else
diff -Nru curl-7.26.0/debian/patches/series curl-7.26.0/debian/patches/series
--- curl-7.26.0/debian/patches/series 2016-11-10 17:50:13.000000000 +0100
+++ curl-7.26.0/debian/patches/series 2016-12-28 22:14:36.000000000 +0100
@@ -36,6 +36,7 @@
CVE-2016-8623.patch
CVE-2016-8624.patch
#don't change behaviour in Wheezy: CVE-2016-8625.patch
+CVE-2016-9586.patch
# Add new patches before the ones below this line
90_gnutls.patch
Reply to: