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>
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56630): https://edk2.groups.io/g/devel/message/56630
Mute This Topic: https://groups.io/mt/72650697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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;
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56697): https://edk2.groups.io/g/devel/message/56697
Mute This Topic: https://groups.io/mt/72650697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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;
>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56757): https://edk2.groups.io/g/devel/message/56757
Mute This Topic: https://groups.io/mt/72650697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56802): https://edk2.groups.io/g/devel/message/56802
Mute This Topic: https://groups.io/mt/72650697/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.