From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mx.groups.io with SMTP id smtpd.web11.7497.1585657358136657300 for ; Tue, 31 Mar 2020 05:22:38 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 192.55.52.136, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: ciw/Um7A6pRXrGXHuGd/mWEXWmwYDYlMgRNcfRNUPd4C4SJMQYOqMSK8WkYfAXfKKPsWOkXZcr LOTef12m/8QQ== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 05:22:37 -0700 IronPort-SDR: LwrdTdDH58yS30bQcoR73yKWMf62O2S71gOd1jhtULSiI+6cO5F1FrYqhFQ0F14UV34pHuIukx mk4MQcaVPwXQ== X-IronPort-AV: E=Sophos;i="5.72,327,1580803200"; d="scan'208";a="395473717" Received: from mrabeda-mobl.ger.corp.intel.com (HELO [10.213.8.245]) ([10.213.8.245]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Mar 2020 05:22:36 -0700 Subject: Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. To: devel@edk2.groups.io, lersek@redhat.com Cc: Jiaxin Wu , Siyuan Fu References: <20200330122351.1229-1-maciej.rabeda@linux.intel.com> <19b35911-8d69-2a32-df59-a9fb05e88457@redhat.com> From: "Maciej Rabeda" Message-ID: <1a4f9e2f-c0a3-4102-63a2-7e3e13d7f3b9@linux.intel.com> Date: Tue, 31 Mar 2020 14:22:33 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.6.0 MIME-Version: 1.0 In-Reply-To: <19b35911-8d69-2a32-df59-a9fb05e88457@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Content-Language: pl Hi Laszlo, Thanks for trying this out! The condition in the ASSERTs is reversed, consequently for the ASSERTs added in this function. I have added them to fire up when Ip6IsNDOptionValid() fails to properly react to invalid packet (return with an error and do not proceed with processing options). In this case, fire assert when current position within the packet (Offset) moved past the size of the option (Option.Length) * 8 (it's in 8-byte units) is outside the packet (further than first byte outside the packet). Offset one byte past the packet == last option ended at the end of the packet (packet size is Head->PayloadLength). Can you try swapping the >= to <= and rerun your test? This should apply to lines 2114, 2167 and 2337. I will submit a patch for that. Thanks, Maciej On 31-Mar-20 02:35, Laszlo Ersek wrote: > Hi Maciej, > > On 03/30/20 14:23, 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 >> Reviewed-by: Siyuan Fu >> --- >> NetworkPkg/Ip6Dxe/Ip6Nd.c | 44 +++++++++------ >> NetworkPkg/Ip6Dxe/Ip6Nd.h | 13 +++++ >> NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++----- >> 3 files changed, 83 insertions(+), 31 deletions(-) >> >> 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); > I've hit this ASSERT(): > > ASSERT NetworkPkg/Ip6Dxe/Ip6Nd.c(2167): Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength > > Here's the packet that triggers it: > > 02:30:25.350096 52:54:00:18:64:e7 > 33:33:00:00:00:01, ethertype IPv6 (0x86dd), length 142: (class 0xc0, hlim 255, next-header ICMPv6 (58) payload length: 88) fe80::5054:ff:fe18:64e7 > ff02::1: [icmp6 sum ok] ICMP6, router advertisement, length 88 > hop limit 64, Flags [none], pref medium, router lifetime 1800s, reachable time 0ms, retrans time 0ms > prefix info option (3), length 32 (4): fd33:eb1b:9b36::/64, Flags [onlink, auto], valid time 3600s, pref. time 3600s > mtu option (5), length 8 (1): 1500 > source link-address option (1), length 8 (1): 52:54:00:18:64:e7 > rdnss option (25), length 24 (3): lifetime 3600s, addr: fe80::5054:ff:fe18:64e7 > 0x0000: 6c00 0000 0058 3aff fe80 0000 0000 0000 l....X:......... > 0x0010: 5054 00ff fe18 64e7 ff02 0000 0000 0000 PT....d......... > 0x0020: 0000 0000 0000 0001 8600 0008 4000 0708 ............@... > 0x0030: 0000 0000 0000 0000 0304 40c0 0000 0e10 ..........@..... > 0x0040: 0000 0e10 0000 0000 fd33 eb1b 9b36 0000 .........3...6.. > 0x0050: 0000 0000 0000 0000 0501 0000 0000 05dc ................ > 0x0060: 0101 5254 0018 64e7 1903 0000 0000 0e10 ..RT..d......... > 0x0070: fe80 0000 0000 0000 5054 00ff fe18 64e7 ........PT....d. > > Thanks, > Laszlo > >> + >> 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; >> > > >