From patchwork Fri Jul 22 23:43:50 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 651799 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3rx6gB29MLz9t15 for ; Sat, 23 Jul 2016 09:44:06 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id CD78F22C41B; Fri, 22 Jul 2016 16:44:04 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 850EA22C3F4 for ; Fri, 22 Jul 2016 16:44:03 -0700 (PDT) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id E538C42020D for ; Fri, 22 Jul 2016 17:44:02 -0600 (MDT) X-ASG-Debug-ID: 1469231042-09eadd46481a02d0001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar5.cudamail.com with ESMTP id ZvLSjMNLdoziqY4s (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO) for ; Fri, 22 Jul 2016 17:44:02 -0600 (MDT) X-Barracuda-Envelope-From: blp@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO relay6-d.mail.gandi.net) (217.70.183.198) by mx1-pf1.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 22 Jul 2016 23:44:01 -0000 Received-SPF: pass (mx1-pf1.cudamail.com: SPF record at ovn.org designates 217.70.183.198 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.198 X-Barracuda-RBL-IP: 217.70.183.198 Received: from mfilter30-d.gandi.net (mfilter30-d.gandi.net [217.70.178.161]) by relay6-d.mail.gandi.net (Postfix) with ESMTP id 15C84FB89F; Sat, 23 Jul 2016 01:43:59 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter30-d.gandi.net Received: from relay6-d.mail.gandi.net ([IPv6:::ffff:217.70.183.198]) by mfilter30-d.gandi.net (mfilter30-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id tJHkdBHtas04; Sat, 23 Jul 2016 01:43:57 +0200 (CEST) X-Originating-IP: 208.91.2.3 Received: from sigabrt.benpfaff.org (unknown [208.91.2.3]) (Authenticated sender: blp@ovn.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 21FCAFB887; Sat, 23 Jul 2016 01:43:55 +0200 (CEST) X-CudaMail-Envelope-Sender: blp@ovn.org From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-721069406 X-CudaMail-DTE: 072216 X-CudaMail-Originating-IP: 217.70.183.198 Date: Fri, 22 Jul 2016 16:43:50 -0700 X-ASG-Orig-Subj: [##CM-E1-721069406##][PATCH] flow: Verify that tot_len >= ip_len in miniflow_extract(). Message-Id: <1469231030-30541-1-git-send-email-blp@ovn.org> X-Mailer: git-send-email 2.1.3 X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1469231042 X-Barracuda-Encrypted: ECDHE-RSA-AES256-GCM-SHA384 X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: Ben Pfaff , Bhargava Shastry , Kashyap Thimmaraju Subject: [ovs-dev] [PATCH] flow: Verify that tot_len >= ip_len in miniflow_extract(). X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" 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 Reported-by: Kashyap Thimmaraju Signed-off-by: Ben Pfaff Acked-by: Flavio Leitner --- lib/flow.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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)) {