* [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. @ 2020-03-30 12:23 Maciej Rabeda 2020-03-31 0:35 ` [edk2-devel] " Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Maciej Rabeda @ 2020-03-30 12:23 UTC (permalink / raw) To: devel; +Cc: Jiaxin Wu, Siyuan Fu 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 <jiaxin.wu@intel.com> Cc: Siyuan Fu <siyuan.fu@intel.com> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> --- 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); + 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; -- 2.24.0.windows.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. 2020-03-30 12:23 [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation Maciej Rabeda @ 2020-03-31 0:35 ` Laszlo Ersek 2020-03-31 12:22 ` Maciej Rabeda 0 siblings, 1 reply; 4+ messages in thread From: Laszlo Ersek @ 2020-03-31 0:35 UTC (permalink / raw) To: devel, maciej.rabeda; +Cc: Jiaxin Wu, Siyuan Fu 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 <jiaxin.wu@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> > --- > 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; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. 2020-03-31 0:35 ` [edk2-devel] " Laszlo Ersek @ 2020-03-31 12:22 ` Maciej Rabeda 2020-04-01 0:27 ` Laszlo Ersek 0 siblings, 1 reply; 4+ messages in thread From: Maciej Rabeda @ 2020-03-31 12:22 UTC (permalink / raw) To: devel, lersek; +Cc: Jiaxin Wu, Siyuan Fu 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 <jiaxin.wu@intel.com> >> Cc: Siyuan Fu <siyuan.fu@intel.com> >> Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> >> Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> >> --- >> 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; >> > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation. 2020-03-31 12:22 ` Maciej Rabeda @ 2020-04-01 0:27 ` Laszlo Ersek 0 siblings, 0 replies; 4+ messages in thread From: Laszlo Ersek @ 2020-04-01 0:27 UTC (permalink / raw) To: devel, maciej.rabeda; +Cc: Jiaxin Wu, Siyuan Fu On 03/31/20 14:22, Maciej Rabeda wrote: > 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). I understand. In your testing, both sides were probably equal, so the inverted relops didn't cause problems. > 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. The following patch works OK for me: diff --git a/NetworkPkg/Ip6Dxe/Ip6Nd.c b/NetworkPkg/Ip6Dxe/Ip6Nd.c index fd7f60b2f92c..0780a98cb325 100644 --- a/NetworkPkg/Ip6Dxe/Ip6Nd.c +++ b/NetworkPkg/Ip6Dxe/Ip6Nd.c @@ -2111,7 +2111,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (LinkLayerOption.Length != 0); - ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) LinkLayerOption.Length * 8 <= (UINT32) Head->PayloadLength); ZeroMem (&LinkLayerAddress, sizeof (EFI_MAC_ADDRESS)); CopyMem (&LinkLayerAddress, LinkLayerOption.EtherAddr, 6); @@ -2164,7 +2164,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (PrefixOption.Length == 4); - ASSERT (Offset + (UINT32) PrefixOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) PrefixOption.Length * 8 <= (UINT32) Head->PayloadLength); PrefixOption.ValidLifetime = NTOHL (PrefixOption.ValidLifetime); PrefixOption.PreferredLifetime = NTOHL (PrefixOption.PreferredLifetime); @@ -2334,7 +2334,7 @@ Ip6ProcessRouterAdvertise ( // Option size validity ensured by Ip6IsNDOptionValid(). // ASSERT (MTUOption.Length == 1); - ASSERT (Offset + (UINT32) MTUOption.Length * 8 >= (UINT32) Head->PayloadLength); + ASSERT (Offset + (UINT32) MTUOption.Length * 8 <= (UINT32) Head->PayloadLength); // // Use IPv6 minimum link MTU 1280 bytes as the maximum packet size in order If your posted patch will be identical to the above code changes, then you can add at once: Tested-by: Laszlo Ersek <lersek@redhat.com> Thanks! Laszlo ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-01 0:27 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-30 12:23 [PATCH v2] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation Maciej Rabeda 2020-03-31 0:35 ` [edk2-devel] " Laszlo Ersek 2020-03-31 12:22 ` Maciej Rabeda 2020-04-01 0:27 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox