From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mx.groups.io with SMTP id smtpd.web12.48646.1585571038721725891 for ; Mon, 30 Mar 2020 05:23:58 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: linux.intel.com, ip: 134.134.136.24, mailfrom: maciej.rabeda@linux.intel.com) IronPort-SDR: hKWqGxH11mBiX0BCfc6sshAHkFHkSJOteS/W1cwIss3F2T1opr24xSzD8zFChit/caTZvalX2V 4Zl3njuqVJlA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Mar 2020 05:23:58 -0700 IronPort-SDR: A3dCKMDqJfBc54Q0BfD2xOHH/rYQGPF5srNZeiCgy2VCqYqbo1D95pggRLdSCt+vTvNr38nbxY O6h68TNFRJQA== X-IronPort-AV: E=Sophos;i="5.72,324,1580803200"; d="scan'208";a="421910150" Received: from mrabeda-mobl.ger.corp.intel.com ([10.213.6.151]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Mar 2020 05:23:56 -0700 From: "Maciej Rabeda" To: devel@edk2.groups.io Cc: Jiaxin Wu , Siyuan Fu Subject: [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. Date: Mon, 30 Mar 2020 14:23:51 +0200 Message-Id: <20200330122351.1229-1-maciej.rabeda@linux.intel.com> X-Mailer: git-send-email 2.24.0.windows.2 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2174 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;=0D UINT32 RetransTimer;=0D UINT16 RouterLifetime;=0D - UINT16 Offset;=0D + UINT32 Offset;=0D UINT8 Type;=0D UINT8 Length;=0D IP6_ETHER_ADDR_OPTION LinkLayerOption;=0D @@ -2094,10 +2094,11 @@ Ip6ProcessRouterAdvertise ( //=0D // The only defined options that may appear are the Source=0D // Link-Layer Address, Prefix information and MTU options.=0D - // All included options have a length that is greater than zero.=0D + // All included options have a length that is greater than zero and=0D + // fit within the input packet.=0D //=0D Offset =3D 16;=0D - while (Offset < Head->PayloadLength) {=0D + while (Offset < (UINT32) Head->PayloadLength) {=0D NetbufCopy (Packet, Offset, sizeof (UINT8), &Type);=0D switch (Type) {=0D case Ip6OptionEtherSource:=0D @@ -2105,9 +2106,12 @@ Ip6ProcessRouterAdvertise ( // Update the neighbor cache=0D //=0D NetbufCopy (Packet, Offset, sizeof (IP6_ETHER_ADDR_OPTION), (UINT8 *= ) &LinkLayerOption);=0D - if (LinkLayerOption.Length <=3D 0) {=0D - goto Exit;=0D - }=0D +=0D + //=0D + // Option size validity ensured by Ip6IsNDOptionValid().=0D + //=0D + ASSERT (LinkLayerOption.Length !=3D 0);=0D + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >=3D (UINT32) H= ead->PayloadLength);=0D =0D ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS));=0D CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6);=0D @@ -2151,13 +2155,17 @@ Ip6ProcessRouterAdvertise ( }=0D }=0D =0D - Offset =3D (UINT16) (Offset + (UINT16) LinkLayerOption.Length * 8);= =0D + Offset +=3D (UINT32) LinkLayerOption.Length * 8;=0D break;=0D case Ip6OptionPrefixInfo:=0D NetbufCopy (Packet, Offset, sizeof (IP6_PREFIX_INFO_OPTION), (UINT8 = *) &PrefixOption);=0D - if (PrefixOption.Length !=3D 4) {=0D - goto Exit;=0D - }=0D +=0D + //=0D + // Option size validity ensured by Ip6IsNDOptionValid().=0D + //=0D + ASSERT (PrefixOption.Length =3D=3D 4);=0D + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >=3D (UINT32) Head= ->PayloadLength);=0D +=0D PrefixOption.ValidLifetime =3D NTOHL (PrefixOption.ValidLifetime= );=0D PrefixOption.PreferredLifetime =3D NTOHL (PrefixOption.PreferredLife= time);=0D =0D @@ -2321,9 +2329,12 @@ Ip6ProcessRouterAdvertise ( break;=0D case Ip6OptionMtu:=0D NetbufCopy (Packet, Offset, sizeof (IP6_MTU_OPTION), (UINT8 *) &MTUO= ption);=0D - if (MTUOption.Length !=3D 1) {=0D - goto Exit;=0D - }=0D +=0D + //=0D + // Option size validity ensured by Ip6IsNDOptionValid().=0D + //=0D + ASSERT (MTUOption.Length =3D=3D 1);=0D + ASSERT (Offset + (UINT32) MTUOption.Length * 8 >=3D (UINT32) Head->P= ayloadLength);=0D =0D //=0D // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size i= n order=0D @@ -2338,11 +2349,10 @@ Ip6ProcessRouterAdvertise ( // Silently ignore unrecognized options=0D //=0D NetbufCopy (Packet, Offset + sizeof (UINT8), sizeof (UINT8), &Length= );=0D - if (Length <=3D 0) {=0D - goto Exit;=0D - }=0D =0D - Offset =3D (UINT16) (Offset + (UINT16) Length * 8);=0D + ASSERT (Length !=3D 0);=0D +=0D + Offset +=3D (UINT32) Length * 8;=0D break;=0D }=0D }=0D 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=0D );=0D =0D +typedef struct _IP6_OPTION_HEADER {=0D + UINT8 Type;=0D + UINT8 Length;=0D +} IP6_OPTION_HEADER;=0D +=0D +STATIC_ASSERT (sizeof (IP6_OPTION_HEADER) =3D=3D 2, "IP6_OPTION_HEADER is = expected to be exactly 2 bytes long.");=0D +=0D typedef struct _IP6_ETHE_ADDR_OPTION {=0D UINT8 Type;=0D UINT8 Length;=0D UINT8 EtherAddr[6];=0D } IP6_ETHER_ADDR_OPTION;=0D =0D +STATIC_ASSERT (sizeof (IP6_ETHER_ADDR_OPTION) =3D=3D 8, "IP6_ETHER_ADDR_OP= TION is expected to be exactly 8 bytes long.");=0D +=0D typedef struct _IP6_MTU_OPTION {=0D UINT8 Type;=0D UINT8 Length;=0D @@ -69,6 +78,8 @@ typedef struct _IP6_MTU_OPTION { UINT32 Mtu;=0D } IP6_MTU_OPTION;=0D =0D +STATIC_ASSERT (sizeof (IP6_MTU_OPTION) =3D=3D 8, "IP6_MTU_OPTION is expect= ed to be exactly 8 bytes long.");=0D +=0D typedef struct _IP6_PREFIX_INFO_OPTION {=0D UINT8 Type;=0D UINT8 Length;=0D @@ -80,6 +91,8 @@ typedef struct _IP6_PREFIX_INFO_OPTION { EFI_IPv6_ADDRESS Prefix;=0D } IP6_PREFIX_INFO_OPTION;=0D =0D +STATIC_ASSERT (sizeof (IP6_PREFIX_INFO_OPTION) =3D=3D 32, "IP6_PREFIX_INFO= _OPTION is expected to be exactly 32 bytes long.");=0D +=0D typedef=0D VOID=0D (*IP6_DAD_CALLBACK) (=0D 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=0D )=0D {=0D - UINT16 Offset;=0D - UINT8 OptionType;=0D + UINT32 Offset;=0D UINT16 Length;=0D + IP6_OPTION_HEADER *OptionHeader;=0D +=0D + if (Option =3D=3D NULL) {=0D + ASSERT (Option !=3D NULL);=0D + return FALSE;=0D + }=0D =0D Offset =3D 0;=0D =0D - while (Offset < OptionLen) {=0D - OptionType =3D *(Option + Offset);=0D - Length =3D (UINT16) (*(Option + Offset + 1) * 8);=0D + //=0D + // RFC 4861 states that Neighbor Discovery packet can contain zero or mo= re=0D + // options. Start processing the options if at least Type + Length field= s=0D + // fit within the input buffer.=0D + //=0D + while (Offset + sizeof (IP6_OPTION_HEADER) - 1 < OptionLen) {=0D + OptionHeader =3D (IP6_OPTION_HEADER*) (Option + Offset);=0D + Length =3D (UINT16) OptionHeader->Length * 8;=0D =0D - switch (OptionType) {=0D + switch (OptionHeader->Type) {=0D case Ip6OptionPrefixInfo:=0D if (Length !=3D 32) {=0D return FALSE;=0D }=0D -=0D break;=0D =0D case Ip6OptionMtu:=0D if (Length !=3D 8) {=0D return FALSE;=0D }=0D -=0D break;=0D =0D default:=0D - //=0D - // Check the length of Ip6OptionEtherSource, Ip6OptionEtherTarget, a= nd=0D - // Ip6OptionRedirected here. For unrecognized options, silently igno= re=0D - // and continue processing the message.=0D - //=0D + // RFC 4861 states that Length field cannot be 0.=0D if (Length =3D=3D 0) {=0D return FALSE;=0D }=0D + break;=0D + }=0D +=0D + //=0D + // Check whether recognized options are within the input buffer's scop= e.=0D + //=0D + switch (OptionHeader->Type) {=0D + case Ip6OptionEtherSource:=0D + case Ip6OptionEtherTarget:=0D + case Ip6OptionPrefixInfo:=0D + case Ip6OptionRedirected:=0D + case Ip6OptionMtu:=0D + if (Offset + Length > (UINT32) OptionLen) {=0D + return FALSE;=0D + }=0D + break;=0D =0D + default:=0D + //=0D + // Unrecognized options can be either valid (but unused) or invalid= =0D + // (garbage in between or right after valid options). Silently ignor= e.=0D + //=0D break;=0D }=0D =0D - Offset =3D (UINT16) (Offset + Length);=0D + //=0D + // Advance to the next option.=0D + // Length already considers option header's Type + Length.=0D + //=0D + Offset +=3D Length;=0D }=0D =0D return TRUE;=0D --=20 2.24.0.windows.2