diff mbox

Fix for heap overflow in wscanf (BZ 16618)

Message ID CALoOobOqBGEp=Jv-sncnUzi6BVzypg9txr-Oh2OTQL7BFbuwSw@mail.gmail.com
State Committed
Headers

Commit Message

Paul Pluzhnikov Feb. 2, 2015, 7:52 p.m. UTC 
  On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:

> So, how about the attached (untested) patch to vfscanf.c instead? It's
> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
> safe in glibc.

I like it. Re-tested.

Combined patch attached.

Thanks,

Comments

Paul Pluzhnikov Feb. 3, 2015, 4:06 p.m. UTC | #1 
On Mon, Feb 2, 2015 at 11:52 AM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>
>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>> safe in glibc.
>
> I like it. Re-tested.
>
> Combined patch attached.

Ping?
Paul Eggert Feb. 3, 2015, 4:12 p.m. UTC | #2 
On 02/02/2015 11:52 AM, Paul Pluzhnikov wrote:
> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>
>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>> safe in glibc.
> I like it. Re-tested.
>
> Combined patch attached.
>
> Thanks,

Thanks, this fix looks good to me.  I assume Carlos needs to ACK this, 
given that the Ottawa river is still frozen solid....
Carlos O'Donell Feb. 3, 2015, 4:50 p.m. UTC | #3 
On 02/03/2015 11:12 AM, Paul Eggert wrote:
> On 02/02/2015 11:52 AM, Paul Pluzhnikov wrote:
>> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>>
>>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>>> safe in glibc.
>> I like it. Re-tested.
>>
>> Combined patch attached.
>>
>> Thanks,
> 
> Thanks, this fix looks good to me.  I assume Carlos needs to ACK this, given that the Ottawa river is still frozen solid....

I do need to ACK it, and I'm reviewing it right now, and testing.

Cheers,
Carlos.
Carlos O'Donell Feb. 6, 2015, 5:52 a.m. UTC | #4 
On 02/02/2015 02:52 PM, Paul Pluzhnikov wrote:
> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> 
>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>> safe in glibc.
> 
> I like it. Re-tested.
> 
> Combined patch attached.
> 
> Thanks,
> 

Committed for 2.21.

commit 5bd80bfe9ca0d955bfbbc002781bc7b01b6bcb06
Author: Paul Pluzhnikov <ppluzhnikov@google.com>
Date:   Fri Feb 6 00:30:42 2015 -0500

    CVE-2015-1472: wscanf allocates too little memory
    
    BZ #16618
    
    Under certain conditions wscanf can allocate too little memory for the
    to-be-scanned arguments and overflow the allocated buffer.  The
    implementation now correctly computes the required buffer size when
    using malloc.
    
    A regression test was added to tst-sscanf.

c.
Florian Weimer Feb. 6, 2015, 1:45 p.m. UTC | #5 
On 02/06/2015 06:52 AM, Carlos O'Donell wrote:
> On 02/02/2015 02:52 PM, Paul Pluzhnikov wrote:
>> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>>
>>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>>> safe in glibc.
>>
>> I like it. Re-tested.
>>
>> Combined patch attached.
>>
>> Thanks,
>>
> 
> Committed for 2.21.
> 
> commit 5bd80bfe9ca0d955bfbbc002781bc7b01b6bcb06
> Author: Paul Pluzhnikov <ppluzhnikov@google.com>
> Date:   Fri Feb 6 00:30:42 2015 -0500
> 
>     CVE-2015-1472: wscanf allocates too little memory
>     
>     BZ #16618
>     
>     Under certain conditions wscanf can allocate too little memory for the
>     to-be-scanned arguments and overflow the allocated buffer.  The
>     implementation now correctly computes the required buffer size when
>     using malloc.
>     
>     A regression test was added to tst-sscanf.

I think this fixes as CVE-2015-1473 as well, which was assigned for the
inconsistent use of __libc_use_alloca (even though no application impact
had been demonstrated).
Carlos O'Donell Feb. 6, 2015, 2:45 p.m. UTC | #6 
On 02/06/2015 08:45 AM, Florian Weimer wrote:
> On 02/06/2015 06:52 AM, Carlos O'Donell wrote:
>> On 02/02/2015 02:52 PM, Paul Pluzhnikov wrote:
>>> On Mon, Feb 2, 2015 at 11:23 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>>>
>>>> So, how about the attached (untested) patch to vfscanf.c instead? It's
>>>> simpler.  It does rely on realloc (wp, SIZE_MAX) returning NULL, but that's
>>>> safe in glibc.
>>>
>>> I like it. Re-tested.
>>>
>>> Combined patch attached.
>>>
>>> Thanks,
>>>
>>
>> Committed for 2.21.
>>
>> commit 5bd80bfe9ca0d955bfbbc002781bc7b01b6bcb06
>> Author: Paul Pluzhnikov <ppluzhnikov@google.com>
>> Date:   Fri Feb 6 00:30:42 2015 -0500
>>
>>     CVE-2015-1472: wscanf allocates too little memory
>>     
>>     BZ #16618
>>     
>>     Under certain conditions wscanf can allocate too little memory for the
>>     to-be-scanned arguments and overflow the allocated buffer.  The
>>     implementation now correctly computes the required buffer size when
>>     using malloc.
>>     
>>     A regression test was added to tst-sscanf.
> 
> I think this fixes as CVE-2015-1473 as well, which was assigned for the
> inconsistent use of __libc_use_alloca (even though no application impact
> had been demonstrated).
> 

Could you confirm that please? I've still got a laundry list of release
announcements to make for 2.21. Then we'll adjust the NEWS and bugzilla
accordingly on release/2.21/master and master.

Cheers,
Carlos.
Paul Pluzhnikov Feb. 6, 2015, 3:19 p.m. UTC | #7 
On Fri, Feb 6, 2015 at 6:45 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 02/06/2015 08:45 AM, Florian Weimer wrote:

>> I think this fixes as CVE-2015-1473 as well,

Correct.
Carlos O'Donell Feb. 6, 2015, 3:29 p.m. UTC | #8 
On 02/06/2015 10:19 AM, Paul Pluzhnikov wrote:
> On Fri, Feb 6, 2015 at 6:45 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 02/06/2015 08:45 AM, Florian Weimer wrote:
> 
>>> I think this fixes as CVE-2015-1473 as well,
> 
> Correct.
> 

Could you expand a bit on this comment? Did you test that it fixes the issue?
Did you review that it's actually the same bug?

I trust your review, but "Correct." is not sufficiently verbose for me and
I want to make sure we're all in agreement.

Cheers,
Carlos.
Florian Weimer Feb. 6, 2015, 3:34 p.m. UTC | #9 
On 02/06/2015 04:29 PM, Carlos O'Donell wrote:
> On 02/06/2015 10:19 AM, Paul Pluzhnikov wrote:
>> On Fri, Feb 6, 2015 at 6:45 AM, Carlos O'Donell <carlos@redhat.com> wrote:
>>> On 02/06/2015 08:45 AM, Florian Weimer wrote:
>>
>>>> I think this fixes as CVE-2015-1473 as well,
>>
>> Correct.
>>
> 
> Could you expand a bit on this comment? Did you test that it fixes the issue?
> Did you review that it's actually the same bug?
> 
> I trust your review, but "Correct." is not sufficiently verbose for me and
> I want to make sure we're all in agreement.

The old code had this:

     size_t newsize = (UCHAR_MAX + 1 > 2 * wpmax                       \
                       ? UCHAR_MAX + 1 : 2 * wpmax);                   \
     if (use_malloc || !__libc_use_alloca (newsize))                   \
…
         wp = (CHAR_T *) extend_alloca (wp, s,                         \
                                        newsize * sizeof (CHAR_T));    \

Which is to say, the alloca policy check was against newsize, but the
actual allocation used newsize * sizeof (CHAR_T).

The new version computes newsize in bytes and uses it consistently,
addressing this discrepancy.
diff mbox

Patch

diff --git a/NEWS b/NEWS
index c91b9fc..85b2948 100644
--- a/NEWS
+++ b/NEWS
@@ -10,15 +10,16 @@  Version 2.21
 * The following bugs are resolved with this release:
 
   6652, 10672, 12674, 12847, 12926, 13862, 14132, 14138, 14171, 14498,
-  15215, 15378, 15884, 16009, 16418, 16191, 16469, 16576, 16617, 16619,
-  16657, 16740, 16857, 17192, 17266, 17273, 17344, 17363, 17370, 17371,
-  17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555, 17570,
-  17571, 17572, 17573, 17574, 17582, 17583, 17584, 17585, 17589, 17594,
-  17601, 17608, 17616, 17625, 17630, 17633, 17634, 17635, 17647, 17653,
-  17657, 17658, 17664, 17665, 17668, 17682, 17702, 17717, 17719, 17722,
-  17723, 17724, 17725, 17732, 17733, 17744, 17745, 17746, 17747, 17748,
-  17775, 17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797, 17801,
-  17803, 17806, 17834, 17844, 17848, 17868, 17869, 17870, 17885, 17892.
+  15215, 15378, 15884, 16009, 16418, 16191, 16469, 16576, 16617, 16618,
+  16619, 16657, 16740, 16857, 17192, 17266, 17273, 17344, 17363, 17370,
+  17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555,
+  17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584, 17585, 17589,
+  17594, 17601, 17608, 17616, 17625, 17630, 17633, 17634, 17635, 17647,
+  17653, 17657, 17658, 17664, 17665, 17668, 17682, 17702, 17717, 17719,
+  17722, 17723, 17724, 17725, 17732, 17733, 17744, 17745, 17746, 17747,
+  17748, 17775, 17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797,
+  17801, 17803, 17806, 17834, 17844, 17848, 17868, 17869, 17870, 17885,
+  17892.
 
 * A new semaphore algorithm has been implemented in generic C code for all
   machines. Previous custom assembly implementations of semaphore were
diff --git a/stdio-common/tst-sscanf.c b/stdio-common/tst-sscanf.c
index aece3f2..a12333c 100644
--- a/stdio-common/tst-sscanf.c
+++ b/stdio-common/tst-sscanf.c
@@ -233,5 +233,23 @@  main (void)
 	}
     }
 
+  /* BZ 16618 */
+  {
+#define SIZE 131072
+    CHAR *s = malloc ((SIZE + 1) * sizeof (*s));
+    if (s == NULL)
+      abort ();
+    for (size_t i = 0; i < SIZE; i++)
+      s[i] = L('0');
+    s[SIZE] = L('\0');
+    int i = 42;
+    if (SSCANF (s, L("%d"), &i) != 1)
+      result = 1;
+    if (i != 0)
+      result = 1;
+    free (s);
+#undef SIZE
+  }
+
   return result;
 }
diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
index cd129a8..0e204e7 100644
--- a/stdio-common/vfscanf.c
+++ b/stdio-common/vfscanf.c
@@ -272,9 +272,10 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
       if (__glibc_unlikely (wpsize == wpmax))				      \
 	{								    \
 	  CHAR_T *old = wp;						    \
-	  size_t newsize = (UCHAR_MAX + 1 > 2 * wpmax			    \
-			    ? UCHAR_MAX + 1 : 2 * wpmax);		    \
-	  if (use_malloc || !__libc_use_alloca (newsize))		    \
+	  bool fits = __glibc_likely (wpmax <= SIZE_MAX / sizeof (CHAR_T) / 2); \
+	  size_t wpneed = MAX (UCHAR_MAX + 1, 2 * wpmax);		    \
+	  size_t newsize = fits ? wpneed * sizeof (CHAR_T) : SIZE_MAX;	    \
+	  if (!__libc_use_alloca (newsize))				    \
 	    {								    \
 	      wp = realloc (use_malloc ? wp : NULL, newsize);		    \
 	      if (wp == NULL)						    \
@@ -286,14 +287,13 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 		}							    \
 	      if (! use_malloc)						    \
 		MEMCPY (wp, old, wpsize);				    \
-	      wpmax = newsize;						    \
+	      wpmax = wpneed;						    \
 	      use_malloc = true;					    \
 	    }								    \
 	  else								    \
 	    {								    \
 	      size_t s = wpmax * sizeof (CHAR_T);			    \
-	      wp = (CHAR_T *) extend_alloca (wp, s,			    \
-					     newsize * sizeof (CHAR_T));    \
+	      wp = (CHAR_T *) extend_alloca (wp, s, newsize);		    \
 	      wpmax = s / sizeof (CHAR_T);				    \
 	      if (old != NULL)						    \
 		MEMCPY (wp, old, wpsize);				    \