diff mbox

[ovs-dev,1/3] flow: Fix remote DoS for crafted MPLS packets with debug logging enabled.

Message ID 1458651757-102018-1-git-send-email-jpettit@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit March 22, 2016, 1:02 p.m. UTC 
From: Ben Pfaff <blp@ovn.org>

A crafted MPLS packet yields a zero 'count' in this excerpt from
miniflow_extract():

        count = parse_mpls(&data, &size);
        miniflow_push_words_32(mf, mpls_lse, mpls, count);

In turn, miniflow_push_words_32() updated mf.map as follows:

    MF.map |= ((UINT64_MAX >> (64 - DIV_ROUND_UP(N_WORDS, 2))) << ofs64);

which expanded to:

    mf.map |= (UINT64_MAX >> 64) << ofs64;

Unforunately, C renders shifting a 64-bit constant by 64 bits undefined.
On common x86 platforms, 'n << 64' is equal to 'n', so this behaves as:

    mf.map |= UINT64_MAX << ofs64;

In this particular case, ofs64 is 15, so this sets the most-significant 48
bits of mf.map (a 63-bit bit-field) to 1.  Only the least-significant 28
bits of mf.map should ever be set to 1, so this sets 35 bits to 1 that
should never be.  Because of the structure of the data structure that
mf.map is embedded within, this makes it possible later to overwrite 8*35
== 280 bytes of data in the stack.  However, there is no obvious way to
control the data used in the overwrite--it is memcpy'd from one place to
another but the source data does not come from the network.  In the bug
reporter's testing, this overwrite caused a userspace crash if debug
logging was enabled, but not otherwise.

This commit fixes the problem by avoiding the out-of-range shift.

Vulnerability: CVE-2016-2074
Reported-by: Kashyap Thimmaraju <kashyap.thimmaraju@sec.t-labs.tu-berlin.de>
Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
Signed-off-by: Ben Pfaff <blp@ovn.org>
Acked-by: Jesse Gross <jesse@kernel.org>
---
 lib/flow.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ben Pfaff March 29, 2016, 12:26 a.m. UTC | #1 
On Tue, Mar 22, 2016 at 06:02:35AM -0700, Justin Pettit wrote:
> From: Ben Pfaff <blp@ovn.org>
> 
> A crafted MPLS packet yields a zero 'count' in this excerpt from
> miniflow_extract():
> 
>         count = parse_mpls(&data, &size);
>         miniflow_push_words_32(mf, mpls_lse, mpls, count);
> 
> In turn, miniflow_push_words_32() updated mf.map as follows:
> 
>     MF.map |= ((UINT64_MAX >> (64 - DIV_ROUND_UP(N_WORDS, 2))) << ofs64);
> 
> which expanded to:
> 
>     mf.map |= (UINT64_MAX >> 64) << ofs64;
> 
> Unforunately, C renders shifting a 64-bit constant by 64 bits undefined.
> On common x86 platforms, 'n << 64' is equal to 'n', so this behaves as:
> 
>     mf.map |= UINT64_MAX << ofs64;
> 
> In this particular case, ofs64 is 15, so this sets the most-significant 48
> bits of mf.map (a 63-bit bit-field) to 1.  Only the least-significant 28
> bits of mf.map should ever be set to 1, so this sets 35 bits to 1 that
> should never be.  Because of the structure of the data structure that
> mf.map is embedded within, this makes it possible later to overwrite 8*35
> == 280 bytes of data in the stack.  However, there is no obvious way to
> control the data used in the overwrite--it is memcpy'd from one place to
> another but the source data does not come from the network.  In the bug
> reporter's testing, this overwrite caused a userspace crash if debug
> logging was enabled, but not otherwise.
> 
> This commit fixes the problem by avoiding the out-of-range shift.
> 
> Vulnerability: CVE-2016-2074
> Reported-by: Kashyap Thimmaraju <kashyap.thimmaraju@sec.t-labs.tu-berlin.de>
> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> Acked-by: Jesse Gross <jesse@kernel.org>

Jesse acked this one, privately.  It's my patch so I can't ack it ;-)
Justin Pettit March 29, 2016, 8:14 p.m. UTC | #2 
> On Mar 28, 2016, at 5:26 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
>> Vulnerability: CVE-2016-2074
>> Reported-by: Kashyap Thimmaraju <kashyap.thimmaraju@sec.t-labs.tu-berlin.de>
>> Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de>
>> Signed-off-by: Ben Pfaff <blp@ovn.org>
>> Acked-by: Jesse Gross <jesse@kernel.org>
> 
> Jesse acked this one, privately.  It's my patch so I can't ack it ;-)

Yep.  I just wanted to get it out on the mailing list so there wouldn't be any gaps.

Thanks for the reviews.

--Justin
diff mbox

Patch

diff --git a/lib/flow.c b/lib/flow.c
index 5df23a9..03c175a 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1,5 +1,5 @@ 
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -197,7 +197,7 @@  BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime "
 
 /* Data at 'valuep' may be unaligned. */
 #define miniflow_push_words_(MF, OFS, VALUEP, N_WORDS)          \
-{                                                               \
+if (N_WORDS) {                                                  \
     int ofs64 = (OFS) / 8;                                      \
                                                                         \
     MINIFLOW_ASSERT(MF.data + (N_WORDS) <= MF.end && (OFS) % 8 == 0     \
@@ -210,7 +210,7 @@  BUILD_MESSAGE("FLOW_WC_SEQ changed: miniflow_extract() will have runtime "
 
 /* Push 32-bit words padded to 64-bits. */
 #define miniflow_push_words_32_(MF, OFS, VALUEP, N_WORDS)               \
-{                                                                       \
+if (N_WORDS) {                                                          \
     int ofs64 = (OFS) / 8;                                              \
                                                                         \
     MINIFLOW_ASSERT(MF.data + DIV_ROUND_UP(N_WORDS, 2) <= MF.end        \