* [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
@ 2020-03-02 12:38 Maciej Rabeda
2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Maciej Rabeda @ 2020-03-02 12:38 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>
---
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] 3+ messages in thread
* Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
2020-03-02 12:38 [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559) Maciej Rabeda
@ 2020-03-25 11:35 ` Laszlo Ersek
2020-03-26 0:01 ` Siyuan, Fu
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2020-03-25 11:35 UTC (permalink / raw)
To: Jiaxin Wu, Siyuan Fu; +Cc: devel, maciej.rabeda
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;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559)
2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
@ 2020-03-26 0:01 ` Siyuan, Fu
0 siblings, 0 replies; 3+ messages in thread
From: Siyuan, Fu @ 2020-03-26 0:01 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Wu, Jiaxin
Cc: maciej.rabeda@linux.intel.com
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;
> >
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-26 0:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-02 12:38 [PATCH v1] NetworkPkg/Ip6Dxe: Improve Neightbor Discovery message validation (CVE-2019-14559) Maciej Rabeda
2020-03-25 11:35 ` [edk2-devel] " Laszlo Ersek
2020-03-26 0:01 ` Siyuan, Fu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox