From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [216.205.24.74]) by mx.groups.io with SMTP id smtpd.web12.9341.1585136160089785680 for ; Wed, 25 Mar 2020 04:36:00 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=A6VYE7Il; spf=pass (domain: redhat.com, ip: 216.205.24.74, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1585136159; 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=zCrLQXsSI7qoxrYrh7VTKAC5pvucD52fupU03p1qfDE=; b=A6VYE7IlbixuO+fPBvFp+Em2BnPIUexIDeGhh9N30QJLXbMuX6Zq9qWyF3mdCRP6nxX9fU 8S9qSkdxW7N8T2TgjfkGHdm/kEhfCx77ssHWo9JaHm647P85pkAGmYkO2N9U0BDstipA+G d1PkDljIhSNIJvryEplsSE3ha11s1+w= 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-175-Jcvt9x2XO_GkG27PwfiqRQ-1; Wed, 25 Mar 2020 07:35:54 -0400 X-MC-Unique: Jcvt9x2XO_GkG27PwfiqRQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id EBDE88017CC; Wed, 25 Mar 2020 11:35:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-153.ams2.redhat.com [10.36.113.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id CF86094B5B; Wed, 25 Mar 2020 11:35:51 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559) To: Jiaxin Wu , Siyuan Fu Cc: devel@edk2.groups.io, maciej.rabeda@linux.intel.com References: <20200302123852.575-1-maciej.rabeda@linux.intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 25 Mar 2020 12:35:50 +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: <20200302123852.575-1-maciej.rabeda@linux.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 Jiaxin, Siyuan, On 03/02/20 13:38, Maciej Rabeda wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2174 > > Problem has been identified with Ip6ProcessRouterAdvertise() when > Router Advertise packet contains options with malicious/invalid > 'Length' field. This can lead to platform entering infinite loop > when processing options from that packet. > > Cc: Jiaxin Wu > Cc: Siyuan Fu > Signed-off-by: Maciej Rabeda > --- > NetworkPkg/Ip6Dxe/Ip6Nd.c | 44 +++++++++------ > NetworkPkg/Ip6Dxe/Ip6Nd.h | 13 +++++ > NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++----- > 3 files changed, 83 insertions(+), 31 deletions(-) can you please review this patch? It has spent 3+ weeks on the list. Thanks, Laszlo > diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c > index 4288ef02dd46..fd7f60b2f92c 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c > +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c > @@ -1927,7 +1927,7 @@ Ip6ProcessRouterAdvertise ( > UINT32 ReachableTime; > UINT32 RetransTimer; > UINT16 RouterLifetime; > - UINT16 Offset; > + UINT32 Offset; > UINT8 Type; > UINT8 Length; > IP6_ETHER_ADDR_OPTION LinkLayerOption; > @@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise ( > // > // The only defined options that may appear are the Source > // Link-Layer Address, Prefix information and MTU options. > - // All included options have a length that is greater than zero. > + // All included options have a length that is greater than zero and > + // fit within the input packet. > // > Offset = 16; > - while (Offset < Head->PayloadLength) { > + while (Offset < (UINT32) Head->PayloadLength) { > NetbufCopy (Packet, Offset, sizeof (UINT8), &Type); > switch (Type) { > case Ip6OptionEtherSource: > @@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise ( > // Update the neighbor cache > // > NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *) &LinkLayerOption); > - if (LinkLayerOption.Length <= 0) { > - goto Exit; > - } > + > + // > + // Option size validity ensured by Ip6IsNDOptionValid(). > + // > + ASSERT (LinkLayerOption.Length != 0); > + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength); > > ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); > CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); > @@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise ( > } > } > > - Offset = (UINT16) (Offset + (UINT16) LinkLayerOption.Length * 8); > + Offset += (UINT32) LinkLayerOption.Length * 8; > break; > case Ip6OptionPrefixInfo: > NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 *) &PrefixOption); > - if (PrefixOption.Length != 4) { > - goto Exit; > - } > + > + // > + // Option size validity ensured by Ip6IsNDOptionValid(). > + // > + ASSERT (PrefixOption.Length == 4); > + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength); > + > PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); > PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime); > > @@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise ( > break; > case Ip6OptionMtu: > NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUOption); > - if (MTUOption.Length != 1) { > - goto Exit; > - } > + > + // > + // Option size validity ensured by Ip6IsNDOptionValid(). > + // > + ASSERT (MTUOption.Length == 1); > + ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength); > > // > // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order > @@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise ( > // Silently ignore unrecognized options > // > NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length); > - if (Length <= 0) { > - goto Exit; > - } > > - Offset = (UINT16) (Offset + (UINT16) Length * 8); > + ASSERT (Length != 0); > + > + Offset += (UINT32) Length * 8; > break; > } > } > diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.h b/NetworkPkg/Ip6Dxe/Ip6Nd.h > index 560dfa343782..5f1bd6fb922a 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Nd.h > +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.h > @@ -56,12 +56,21 @@ VOID > VOID *Context > ); > > +typedef struct _IP6_OPTION_HEADER { > + UINT8 Type; > + UINT8 Length; > +} IP6_OPTION_HEADER; > + > +STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) == 2, "IP6_OPTION_HEADER is expected to be exactly 2 bytes long."); > + > typedef struct _IP6_ETHE_ADDR_OPTION { > UINT8 Type; > UINT8 Length; > UINT8 EtherAddr[6]; > } IP6_ETHER_ADDR_OPTION; > > +STATIC_ASSERT (sizeof (IP6_ETHER_ADDR_OPTION) == 8, "IP6_ETHER_ADDR_OPTION is expected to be exactly 8 bytes long."); > + > typedef struct _IP6_MTU_OPTION { > UINT8 Type; > UINT8 Length; > @@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION { > UINT32 Mtu; > } IP6_MTU_OPTION; > > +STATIC_ASSERT (sizeof (IP6_MTU_OPTION) == 8, "IP6_MTU_OPTION is expected to be exactly 8 bytes long."); > + > typedef struct _IP6_PREFIX_INFO_OPTION { > UINT8 Type; > UINT8 Length; > @@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION { > EFI_IPv6_ADDRESS Prefix; > } IP6_PREFIX_INFO_OPTION; > > +STATIC_ASSERT (sizeof (IP6_PREFIX_INFO_OPTION) == 32, "IP6_PREFIX_INFO_OPTION is expected to be exactly 32 bytes long."); > + > typedef > VOID > (*IP6_DAD_CALLBACK) ( > diff --git a/NetworkPkg/Ip6Dxe/Ip6Option.c b/NetworkPkg/Ip6Dxe/Ip6Option.c > index 4d92a852dc86..6b4b029d1479 100644 > --- a/NetworkPkg/Ip6Dxe/Ip6Option.c > +++ b/NetworkPkg/Ip6Dxe/Ip6Option.c > @@ -129,45 +129,74 @@ Ip6IsNDOptionValid ( > IN UINT16 OptionLen > ) > { > - UINT16 Offset; > - UINT8 OptionType; > + UINT32 Offset; > UINT16 Length; > + IP6_OPTION_HEADER *OptionHeader; > + > + if (Option == NULL) { > + ASSERT (Option != NULL); > + return FALSE; > + } > > Offset = 0; > > - while (Offset < OptionLen) { > - OptionType = *(Option + Offset); > - Length = (UINT16) (*(Option + Offset + 1) * 8); > + // > + // RFC 4861 states that Neighbor Discovery packet can contain zero or more > + // options. Start processing the options if at least Type + Length fields > + // fit within the input buffer. > + // > + while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) { > + OptionHeader = (IP6_OPTION_HEADER*) (Option + Offset); > + Length = (UINT16) OptionHeader->Length * 8; > > - switch (OptionType) { > + switch (OptionHeader->Type) { > case Ip6OptionPrefixInfo: > if (Length != 32) { > return FALSE; > } > - > break; > > case Ip6OptionMtu: > if (Length != 8) { > return FALSE; > } > - > break; > > default: > - // > - // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, and > - // Ip6OptionRedirected here. For unrecognized options, silently ignore > - // and continue processing the message. > - // > + // RFC 4861 states that Length field cannot be 0. > if (Length == 0) { > return FALSE; > } > + break; > + } > + > + // > + // Check whether recognized options are within the input buffer's scope. > + // > + switch (OptionHeader->Type) { > + case Ip6OptionEtherSource: > + case Ip6OptionEtherTarget: > + case Ip6OptionPrefixInfo: > + case Ip6OptionRedirected: > + case Ip6OptionMtu: > + if (Offset + Length > (UINT32) OptionLen) { > + return FALSE; > + } > + break; > > + default: > + // > + // Unrecognized options can be either valid (but unused) or invalid > + // (garbage in between or right after valid options). Silently ignore. > + // > break; > } > > - Offset = (UINT16) (Offset + Length); > + // > + // Advance to the next option. > + // Length already considers option header's Type + Length. > + // > + Offset += Length; > } > > return TRUE; >