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...@intel.com>
> Cc: Siyuan Fu <siyuan...@intel.com>
> Signed-off-by: Maciej Rabeda <maciej.rab...@linux.intel.com>
> Reviewed-by: Siyuan Fu <siyuan...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
