| Message ID | 1469231030-30541-1-git-send-email-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Jul 22, 2016 at 04:43:50PM -0700, Ben Pfaff wrote: > miniflow_extract() uses the following quantities when it examines an IPv4 > header: > > size, the number of bytes from the start of the IPv4 header onward > ip_len, the number of bytes in the IPv4 header (from the IHL field) > tot_len, same as size but taken from IPv4 header Total Length field > > Until now, the code in miniflow_extract() verified these invariants: > > size >= 20 (minimum IP header length) > ip_len >= 20 (ditto) > ip_len <= size (to avoid reading past end of packet) > tot_len <= size (ditto) > size - tot_len <= 255 (because this is stored in a 1-byte variable > internally and wouldn't normally be big) > > It failed to verify the following, which is not implied by the conjunction > of the above: > > ip_len <= tot_len (e.g. that the IP header fits in the packet) > > This means that the code was willing to read past the end of an IP > packet's declared length, up to the actual end of the packet including any > L2 padding. For example, given: > > size = 44 > ip_len = 44 > tot_len = 40 > > miniflow_extract() would successfully verify all the constraints, then: > > * Trim off 4 bytes of tail padding (size - tot_len), reducing size to > 40 to match tot_len. > * Pull 44 (ip_len) bytes of IP header, even though there are only 40 > bytes left. This causes 'size' to wrap around to SIZE_MAX-4. > > Given an IP protocol that OVS understands (such as TCP or UDP), this > integer wraparound could cause OVS to read past the end of the packet. > In turn, this could cause OVS to extract invalid port numbers, TCP flags, > or ICMPv4 or ICMPv6 or IGMP type and code from arbitrary heap data > past the end of a packet. > > This bug has common hallmarks of a security vulnerability, but we do not > know of a way to exploit this bug to cause an Open vSwitch crash, or to > extract sensitive data from Open vSwitch address space to an attacker's > benefit. > > We do not have a specific example, but it is reasonable to suspect that > this bug could allow an attacker in some circumstances to bypass ACLs > implemented via Open vSwitch flow tables. However, any IP packet that > triggers this bug is invalid and should be rejected in an early stage of a > receiver's IP stack. For the same reason, any IP packet that triggers this > bug will also be dropped by any IP router, so an attacker would have to > share the same L2 segment as the victim. In conjunction with an IP stack > that has a similar bug, of course, this could cause some damage, but we do > not know of an IP stack with such a bug; neither Linux nor the OVS > userspace tunnel implementation appear to have such a bug. > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > Reported-by: Kashyap Thimmaraju <Kashyap.Thimmaraju@sec.t-labs.tu-berlin.de> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- Acked-by: Flavio Leitner <fbl@sysclose.org>
On Sat, Jul 23, 2016 at 02:29:35AM -0300, Flavio Leitner wrote: > On Fri, Jul 22, 2016 at 04:43:50PM -0700, Ben Pfaff wrote: > > miniflow_extract() uses the following quantities when it examines an IPv4 > > header: > > > > size, the number of bytes from the start of the IPv4 header onward > > ip_len, the number of bytes in the IPv4 header (from the IHL field) > > tot_len, same as size but taken from IPv4 header Total Length field > > > > Until now, the code in miniflow_extract() verified these invariants: > > > > size >= 20 (minimum IP header length) > > ip_len >= 20 (ditto) > > ip_len <= size (to avoid reading past end of packet) > > tot_len <= size (ditto) > > size - tot_len <= 255 (because this is stored in a 1-byte variable > > internally and wouldn't normally be big) > > > > It failed to verify the following, which is not implied by the conjunction > > of the above: > > > > ip_len <= tot_len (e.g. that the IP header fits in the packet) > > > > This means that the code was willing to read past the end of an IP > > packet's declared length, up to the actual end of the packet including any > > L2 padding. For example, given: > > > > size = 44 > > ip_len = 44 > > tot_len = 40 > > > > miniflow_extract() would successfully verify all the constraints, then: > > > > * Trim off 4 bytes of tail padding (size - tot_len), reducing size to > > 40 to match tot_len. > > * Pull 44 (ip_len) bytes of IP header, even though there are only 40 > > bytes left. This causes 'size' to wrap around to SIZE_MAX-4. > > > > Given an IP protocol that OVS understands (such as TCP or UDP), this > > integer wraparound could cause OVS to read past the end of the packet. > > In turn, this could cause OVS to extract invalid port numbers, TCP flags, > > or ICMPv4 or ICMPv6 or IGMP type and code from arbitrary heap data > > past the end of a packet. > > > > This bug has common hallmarks of a security vulnerability, but we do not > > know of a way to exploit this bug to cause an Open vSwitch crash, or to > > extract sensitive data from Open vSwitch address space to an attacker's > > benefit. > > > > We do not have a specific example, but it is reasonable to suspect that > > this bug could allow an attacker in some circumstances to bypass ACLs > > implemented via Open vSwitch flow tables. However, any IP packet that > > triggers this bug is invalid and should be rejected in an early stage of a > > receiver's IP stack. For the same reason, any IP packet that triggers this > > bug will also be dropped by any IP router, so an attacker would have to > > share the same L2 segment as the victim. In conjunction with an IP stack > > that has a similar bug, of course, this could cause some damage, but we do > > not know of an IP stack with such a bug; neither Linux nor the OVS > > userspace tunnel implementation appear to have such a bug. > > > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > > Reported-by: Kashyap Thimmaraju <Kashyap.Thimmaraju@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Acked-by: Flavio Leitner <fbl@sysclose.org> Thank you. I'll plan to give this until Monday to collect more acks (well, more scrutiny really, but acks are usually the result of that), if possible.
On Sat, Jul 23, 2016 at 02:29:35AM -0300, Flavio Leitner wrote: > On Fri, Jul 22, 2016 at 04:43:50PM -0700, Ben Pfaff wrote: > > miniflow_extract() uses the following quantities when it examines an IPv4 > > header: > > > > size, the number of bytes from the start of the IPv4 header onward > > ip_len, the number of bytes in the IPv4 header (from the IHL field) > > tot_len, same as size but taken from IPv4 header Total Length field > > > > Until now, the code in miniflow_extract() verified these invariants: > > > > size >= 20 (minimum IP header length) > > ip_len >= 20 (ditto) > > ip_len <= size (to avoid reading past end of packet) > > tot_len <= size (ditto) > > size - tot_len <= 255 (because this is stored in a 1-byte variable > > internally and wouldn't normally be big) > > > > It failed to verify the following, which is not implied by the conjunction > > of the above: > > > > ip_len <= tot_len (e.g. that the IP header fits in the packet) > > > > This means that the code was willing to read past the end of an IP > > packet's declared length, up to the actual end of the packet including any > > L2 padding. For example, given: > > > > size = 44 > > ip_len = 44 > > tot_len = 40 > > > > miniflow_extract() would successfully verify all the constraints, then: > > > > * Trim off 4 bytes of tail padding (size - tot_len), reducing size to > > 40 to match tot_len. > > * Pull 44 (ip_len) bytes of IP header, even though there are only 40 > > bytes left. This causes 'size' to wrap around to SIZE_MAX-4. > > > > Given an IP protocol that OVS understands (such as TCP or UDP), this > > integer wraparound could cause OVS to read past the end of the packet. > > In turn, this could cause OVS to extract invalid port numbers, TCP flags, > > or ICMPv4 or ICMPv6 or IGMP type and code from arbitrary heap data > > past the end of a packet. > > > > This bug has common hallmarks of a security vulnerability, but we do not > > know of a way to exploit this bug to cause an Open vSwitch crash, or to > > extract sensitive data from Open vSwitch address space to an attacker's > > benefit. > > > > We do not have a specific example, but it is reasonable to suspect that > > this bug could allow an attacker in some circumstances to bypass ACLs > > implemented via Open vSwitch flow tables. However, any IP packet that > > triggers this bug is invalid and should be rejected in an early stage of a > > receiver's IP stack. For the same reason, any IP packet that triggers this > > bug will also be dropped by any IP router, so an attacker would have to > > share the same L2 segment as the victim. In conjunction with an IP stack > > that has a similar bug, of course, this could cause some damage, but we do > > not know of an IP stack with such a bug; neither Linux nor the OVS > > userspace tunnel implementation appear to have such a bug. > > > > Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> > > Reported-by: Kashyap Thimmaraju <Kashyap.Thimmaraju@sec.t-labs.tu-berlin.de> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > Acked-by: Flavio Leitner <fbl@sysclose.org> Thanks, I applied this to master, branch-2.5, and branch-2.4.
diff --git a/lib/flow.c b/lib/flow.c index 8842e8d..e2b121d 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -581,7 +581,7 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) goto out; } tot_len = ntohs(nh->ip_tot_len); - if (OVS_UNLIKELY(tot_len > size)) { + if (OVS_UNLIKELY(tot_len > size || ip_len > tot_len)) { goto out; } if (OVS_UNLIKELY(size - tot_len > UINT8_MAX)) {
miniflow_extract() uses the following quantities when it examines an IPv4 header: size, the number of bytes from the start of the IPv4 header onward ip_len, the number of bytes in the IPv4 header (from the IHL field) tot_len, same as size but taken from IPv4 header Total Length field Until now, the code in miniflow_extract() verified these invariants: size >= 20 (minimum IP header length) ip_len >= 20 (ditto) ip_len <= size (to avoid reading past end of packet) tot_len <= size (ditto) size - tot_len <= 255 (because this is stored in a 1-byte variable internally and wouldn't normally be big) It failed to verify the following, which is not implied by the conjunction of the above: ip_len <= tot_len (e.g. that the IP header fits in the packet) This means that the code was willing to read past the end of an IP packet's declared length, up to the actual end of the packet including any L2 padding. For example, given: size = 44 ip_len = 44 tot_len = 40 miniflow_extract() would successfully verify all the constraints, then: * Trim off 4 bytes of tail padding (size - tot_len), reducing size to 40 to match tot_len. * Pull 44 (ip_len) bytes of IP header, even though there are only 40 bytes left. This causes 'size' to wrap around to SIZE_MAX-4. Given an IP protocol that OVS understands (such as TCP or UDP), this integer wraparound could cause OVS to read past the end of the packet. In turn, this could cause OVS to extract invalid port numbers, TCP flags, or ICMPv4 or ICMPv6 or IGMP type and code from arbitrary heap data past the end of a packet. This bug has common hallmarks of a security vulnerability, but we do not know of a way to exploit this bug to cause an Open vSwitch crash, or to extract sensitive data from Open vSwitch address space to an attacker's benefit. We do not have a specific example, but it is reasonable to suspect that this bug could allow an attacker in some circumstances to bypass ACLs implemented via Open vSwitch flow tables. However, any IP packet that triggers this bug is invalid and should be rejected in an early stage of a receiver's IP stack. For the same reason, any IP packet that triggers this bug will also be dropped by any IP router, so an attacker would have to share the same L2 segment as the victim. In conjunction with an IP stack that has a similar bug, of course, this could cause some damage, but we do not know of an IP stack with such a bug; neither Linux nor the OVS userspace tunnel implementation appear to have such a bug. Reported-by: Bhargava Shastry <bshastry@sec.t-labs.tu-berlin.de> Reported-by: Kashyap Thimmaraju <Kashyap.Thimmaraju@sec.t-labs.tu-berlin.de> Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/flow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)