NetworkPkg/Ip6Dxe/Ip6Nd.c | 44 +++++++++------ NetworkPkg/Ip6Dxe/Ip6Nd.h | 13 +++++ NetworkPkg/Ip6Dxe/Ip6Option.c | 57 +++++++++++++++----- 3 files changed, 83 insertions(+), 31 deletions(-)
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>
---
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#55170): https://edk2.groups.io/g/devel/message/55170
Mute This Topic: https://groups.io/mt/71673126/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <jiaxin.wu@intel.com> > Cc: Siyuan Fu <siyuan.fu@intel.com> > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > --- > 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; > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56285): https://edk2.groups.io/g/devel/message/56285 Mute This Topic: https://groups.io/mt/71673126/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Maciej/Laszlo, Sorry I missed this patch. It looks good with me. Reviewed-by: Siyuan Fu <siyuan.fu@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek > Sent: 2020年3月25日 19:36 > To: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com> > Cc: devel@edk2.groups.io; maciej.rabeda@linux.intel.com > Subject: Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor > Discovery message validation (CVE-2019-14559) > > 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 <jiaxin.wu@intel.com> > > Cc: Siyuan Fu <siyuan.fu@intel.com> > > Signed-off-by: Maciej Rabeda <maciej.rabeda@linux.intel.com> > > --- > > 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; > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#56347): https://edk2.groups.io/g/devel/message/56347 Mute This Topic: https://groups.io/mt/71673126/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.