From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.7026.1581936012621446086 for ; Mon, 17 Feb 2020 02:40:12 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Z+V/wVSl; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581936011; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ArMc0v9yP5FqQpOFEUyZXY0eZA1M9+0KgWwumXX8kuI=; b=Z+V/wVSlDEL65zw6i446ANRIhabmo93Mr5cz36fKAaGgdVKbFzMksH7r3EZQ8pAZiqCxON rkhWAkreKwwkl9gp5IlifPwCTCulCZxmjCkErc80+hl4Rq9kEzkcGKsoC+lB/NbRwAa+Za FiApb3JLQ0MnHBtCd3tpKD29pJMl9yg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-32-jCgCzS2_OLC8XQ9PAqsETg-1; Mon, 17 Feb 2020 05:40:02 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 73FB2801E67; Mon, 17 Feb 2020 10:40:01 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-195.ams2.redhat.com [10.36.116.195]) by smtp.corp.redhat.com (Postfix) with ESMTP id 018155C553; Mon, 17 Feb 2020 10:39:59 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package length (CVE-2019-14559). To: devel@edk2.groups.io, jiaxin.wu@intel.com Cc: Fu Siyuan , Maciej Rabeda , nicholas.armour@intel.com References: <20200217074349.8924-1-Jiaxin.wu@intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 17 Feb 2020 11:39:59 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200217074349.8924-1-Jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: jCgCzS2_OLC8XQ9PAqsETg-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 02/17/20 08:43, Wu, Jiaxin wrote: > This patch is to check the received package length to make sure the package > has a valid length field. > > Cc: Fu Siyuan > Cc: Maciej Rabeda > Signed-off-by: Wu Jiaxin > Reviewed-by: Siyuan Fu > --- > NetworkPkg/Ip4Dxe/Ip4Input.c | 46 +++++++++++++++++++++++++++++++++++--------- > 1 file changed, 37 insertions(+), 9 deletions(-) There are two patches on the list for CVE-2019-14559: - [edk2-devel] [PATCH v1] MdeModulePkg/Ip4Dxe: Check the received package length (CVE-2019-14559). - [edk2-devel] [PATCH 1/1] NetworkPkg/ArpDxe: Recycle invalid ARP packets(CVE-2019-14559). sent by different submitters. How do they relate to each other? Also, while Nick's patch mentions TianoCore#2031, the current patch doesn't include a BZ link. Is the current patch for TianoCore#2032? (Per , both BZs share the same CVE ID.) Also, I remain confused (with comment 11 being the latest one, as of this time, in TianoCore#2032), whether the issue affects IPv4 only, IPv6 only, or both. This patch is only for IPv4, apparently. If the present patch is related to TianoCore#2032, then please add a mailing list archive link to the BZ, and move the BZ to IN_PROGRESS status. Laszlo > > diff --git a/NetworkPkg/Ip4Dxe/Ip4Input.c b/NetworkPkg/Ip4Dxe/Ip4Input.c > index fec242c71f..95fbd01d05 100644 > --- a/NetworkPkg/Ip4Dxe/Ip4Input.c > +++ b/NetworkPkg/Ip4Dxe/Ip4Input.c > @@ -1,9 +1,9 @@ > /** @file > IP4 input process. > > -Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2005 - 2020, Intel Corporation. All rights reserved.
> (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -709,14 +709,10 @@ Ip4PreProcessPacket ( > UINT16 Checksum; > > // > // Check if the IP4 header is correctly formatted. > // > - if ((*Packet)->TotalSize < IP4_MIN_HEADLEN) { > - return EFI_INVALID_PARAMETER; > - } > - > HeadLen = (Head->HeadLen << 2); > TotalLen = NTOHS (Head->TotalLen); > > // > // Mnp may deliver frame trailer sequence up, trim it off. > @@ -806,10 +802,34 @@ Ip4PreProcessPacket ( > } > > return EFI_SUCCESS; > } > > +/** > + This function checks the IPv4 packet length. > + > + @param[in] Packet Pointer to the IPv4 Packet to be checked. > + > + @retval TRUE The input IPv4 packet length is valid. > + @retval FALSE The input IPv4 packet length is invalid. > + > +**/ > +BOOLEAN > +Ip4IsValidPacketLength ( > + IN NET_BUF *Packet > + ) > +{ > + // > + // Check the IP4 packet length. > + // > + if (Packet->TotalSize < IP4_MIN_HEADLEN) { > + return FALSE; > + } > + > + return TRUE; > +} > + > /** > The IP4 input routine. It is called by the IP4_INTERFACE when a > IP4 fragment is received from MNP. > > @param[in] Ip4Instance The IP4 child that request the receive, most like > @@ -842,10 +862,14 @@ Ip4AccpetFrame ( > > if (EFI_ERROR (IoStatus) || (IpSb->State == IP4_SERVICE_DESTROY)) { > goto DROP; > } > > + if (!Ip4IsValidPacketLength (Packet)) { > + goto RESTART; > + } > + > Head = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL); > ASSERT (Head != NULL); > OptionLen = (Head->HeadLen << 2) - IP4_MIN_HEADLEN; > if (OptionLen > 0) { > Option = (UINT8 *) (Head + 1); > @@ -888,14 +912,18 @@ Ip4AccpetFrame ( > // > // If the packet is protected by tunnel mode, parse the inner Ip Packet. > // > ZeroMem (&ZeroHead, sizeof (IP4_HEAD)); > if (0 == CompareMem (Head, &ZeroHead, sizeof (IP4_HEAD))) { > - // Packet may have been changed. Head, HeadLen, TotalLen, and > - // info must be reloaded before use. The ownership of the packet > - // is transferred to the packet process logic. > - // > + // Packet may have been changed. Head, HeadLen, TotalLen, and > + // info must be reloaded before use. The ownership of the packet > + // is transferred to the packet process logic. > + // > + if (!Ip4IsValidPacketLength (Packet)) { > + goto RESTART; > + } > + > Head = (IP4_HEAD *) NetbufGetByte (Packet, 0, NULL); > ASSERT (Head != NULL); > Status = Ip4PreProcessPacket ( > IpSb, > &Packet, >