[ovs-dev] [PATCH] datapath-windows: Add validations for IP_HEADER_LEN

[Guru Shetty]

On 15 June 2017 at 15:15, Shashank Ram <rams at vmware.com> wrote:

> Adds validations in OvsGetIp() to make sure the IHL is
> within valid bounds. If IHL is invalid, then the packet
> is dropped by the callers of this function.
>
> Signed-off-by: Shashank Ram <rams at vmware.com>
>
Applied, thanks.

> ---
>  datapath-windows/ovsext/Flow.c         | 5 +++++
>  datapath-windows/ovsext/Offload.c      | 3 +++
>  datapath-windows/ovsext/PacketParser.h | 9 ++++++++-
>  datapath-windows/ovsext/Stt.c          | 2 +-
>  datapath-windows/ovsext/User.c         | 5 +++++
>  datapath-windows/ovsext/Vxlan.c        | 3 ++-
>  6 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/
> Flow.c
> index 60f9b1c..852a22f 100644
> --- a/datapath-windows/ovsext/Flow.c
> +++ b/datapath-windows/ovsext/Flow.c
> @@ -2141,6 +2141,9 @@ OvsExtractLayers(const NET_BUFFER_LIST *packet,
>                      }
>                  }
>              }
> +        } else {
> +            /* Invalid network header */
> +            return NDIS_STATUS_INVALID_PACKET;
>          }
>      } else if (dlType == htons(ETH_TYPE_IPV6)) {
>          NDIS_STATUS status;
> @@ -2360,8 +2363,10 @@ OvsExtractFlow(const NET_BUFFER_LIST *packet,
>                  }
>              }
>          } else {
> +            /* Invalid network header */
>              ((UINT64 *)ipKey)[0] = 0;
>              ((UINT64 *)ipKey)[1] = 0;
> +            return NDIS_STATUS_INVALID_PACKET;
>          }
>      } else if (flow->l2.dlType == htons(ETH_TYPE_IPV6)) {
>          NDIS_STATUS status;
> diff --git a/datapath-windows/ovsext/Offload.c b/datapath-windows/ovsext/
> Offload.c
> index 65d3b67..0905c80 100644
> --- a/datapath-windows/ovsext/Offload.c
> +++ b/datapath-windows/ovsext/Offload.c
> @@ -563,6 +563,9 @@ OvsValidateIPChecksum(PNET_BUFFER_LIST curNbl,
>              if (checksum != hdrChecksum) {
>                  return NDIS_STATUS_FAILURE;
>              }
> +        } else {
> +            /* Invalid network header */
> +            return NDIS_STATUS_FAILURE;
>          }
>      }
>      return NDIS_STATUS_SUCCESS;
> diff --git a/datapath-windows/ovsext/PacketParser.h
> b/datapath-windows/ovsext/PacketParser.h
> index f1d7f28..0d5c0a6 100644
> --- a/datapath-windows/ovsext/PacketParser.h
> +++ b/datapath-windows/ovsext/PacketParser.h
> @@ -17,6 +17,8 @@
>  #ifndef __PACKET_PARSER_H_
>  #define __PACKET_PARSER_H_ 1
>
> +#define MIN_IPV4_HLEN 20
> +
>  #include "precomp.h"
>  #include "NetProto.h"
>
> @@ -107,7 +109,12 @@ OvsGetIp(const NET_BUFFER_LIST *packet,
>      const IPHdr *ip = OvsGetPacketBytes(packet, sizeof *ip, ofs, storage);
>      if (ip) {
>          int ipLen = ip->ihl * 4;
> -        if (ipLen >= sizeof *ip && OvsPacketLenNBL(packet) >= ofs +
> ipLen) {
> +        if (ipLen <  MIN_IPV4_HLEN ||
> +                ipLen > MAX_IPV4_HLEN ||
> +                OvsPacketLenNBL(packet) < ofs + ipLen) {
> +           /* IP header is invalid, flag it */
> +           return NULL;
> +        } else {
>              return ip;
>          }
>      }
> diff --git a/datapath-windows/ovsext/Stt.c b/datapath-windows/ovsext/Stt.c
> index 1f36835..676cf0c 100644
> --- a/datapath-windows/ovsext/Stt.c
> +++ b/datapath-windows/ovsext/Stt.c
> @@ -1019,7 +1019,7 @@ OvsDecapStt(POVS_SWITCH_CONTEXT switchContext,
>                  innerIpHdr->check = IPChecksum((UINT8 *)innerIpHdr,
>                                                  innerIpHdr->ihl * 4, 0);
>              } else {
> -                status = NDIS_STATUS_RESOURCES;
> +                status = NDIS_STATUS_INVALID_PACKET;
>                  goto dropNbl;
>              }
>          } else if (layers.isIPv6) {
> diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/
> User.c
> index 7880220..22ee7af 100644
> --- a/datapath-windows/ovsext/User.c
> +++ b/datapath-windows/ovsext/User.c
> @@ -465,6 +465,11 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
>      ndisStatus = OvsExtractFlow(pNbl, execute->inPort, &key, &layers,
>                       tempTunKey.tunKey.dst == 0 ? NULL :
> &tempTunKey.tunKey);
>
> +    if (ndisStatus != NDIS_STATUS_SUCCESS) {
> +        /* Invalid network header */
> +        goto dropit;
> +    }
> +
>      ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(pNbl);
>      ctx->mru = execute->mru;
>
> diff --git a/datapath-windows/ovsext/Vxlan.c b/datapath-windows/ovsext/
> Vxlan.c
> index 427f31c..f66a7e5 100644
> --- a/datapath-windows/ovsext/Vxlan.c
> +++ b/datapath-windows/ovsext/Vxlan.c
> @@ -489,7 +489,8 @@ OvsSlowPathDecapVxlan(const PNET_BUFFER_LIST packet,
>          if (nh) {
>              layers.l4Offset = layers.l3Offset + nh->ihl * 4;
>          } else {
> -            break;
> +           status = NDIS_STATUS_INVALID_PACKET;
> +           break;
>          }
>
>          /* make sure it's a VXLAN packet */
> --
> 2.9.3.windows.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>