Re: [Patch] NetworkPkg/IpSecDxe: Generate SPI randomly and correct IKE_SPI_BASE value

["Fu, Siyuan" <siyuan.fu@intel.com>]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>



> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Tuesday, September 6, 2016 11:39 AM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [Patch] NetworkPkg/IpSecDxe: Generate SPI randomly and correct
> IKE_SPI_BASE value
> 
> This path made the following update:
> * Generate SPI randomly.
> * Correct IKE_SPI_BASE value according RFC 4302/4303.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/IpSecDxe/IkeCommon.c     | 102
> +++++++++++++++++++++++++++++++-----
>  NetworkPkg/IpSecDxe/IkeCommon.h     |  20 ++++---
>  NetworkPkg/IpSecDxe/Ikev2/Utility.c |  11 +++-
>  3 files changed, 112 insertions(+), 21 deletions(-)
> 
> diff --git a/NetworkPkg/IpSecDxe/IkeCommon.c
> b/NetworkPkg/IpSecDxe/IkeCommon.c
> index 6fc7c06..b1e4321 100644
> --- a/NetworkPkg/IpSecDxe/IkeCommon.c
> +++ b/NetworkPkg/IpSecDxe/IkeCommon.c
> @@ -1,9 +1,9 @@
>  /** @file
>    Common operation of the IKE
> 
> -  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> 
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the
> BSD License
>    which accompanies this distribution.  The full text of the license may
> be found at
>    http://opensource.org/licenses/bsd-license.php.
> @@ -16,14 +16,56 @@
>  #include "Ike.h"
>  #include "IkeCommon.h"
>  #include "IpSecConfigImpl.h"
>  #include "IpSecDebug.h"
> 
> -//
> -// Initial the SPI
> -//
> -UINT32            mNextSpi  = IKE_SPI_BASE;
> +/**
> +  Check whether the new generated Spi has existed.
> +
> +  @param[in]   IkeSaSession   Pointer to the Child SA Session.
> +  @param[in]   SpiValue       SPI Value.
> +
> +  @retval  TRUE    This SpiValue has existed in the Child SA Session
> +  @retval  FALSE   This SpiValue doesn't exist in the Child SA Session.
> +
> +**/
> +BOOLEAN
> +IkeSpiValueExisted (
> +  IN IKEV2_SA_SESSION      *IkeSaSession,
> +  IN UINT32                SpiValue
> +  )
> +{
> +  LIST_ENTRY              *Entry;
> +  LIST_ENTRY              *Next;
> +  IKEV2_CHILD_SA_SESSION  *SaSession;
> +
> +  Entry     = NULL;
> +  Next      = NULL;
> +  SaSession = NULL;
> +
> +  //
> +  // Check whether the SPI value has existed in
> ChildSaEstablishSessionList.
> +  //
> +  NET_LIST_FOR_EACH_SAFE (Entry, Next, &IkeSaSession-
> >ChildSaEstablishSessionList) {
> +    SaSession= IKEV2_CHILD_SA_SESSION_BY_IKE_SA (Entry);
> +    if (SaSession->LocalPeerSpi == SpiValue) {
> +      return TRUE;
> +    }
> +  }
> +
> +  //
> +  // Check whether the SPI value has existed in ChildSaSessionList.
> +  //
> +  NET_LIST_FOR_EACH_SAFE (Entry, Next, &IkeSaSession->ChildSaSessionList)
> {
> +    SaSession= IKEV2_CHILD_SA_SESSION_BY_IKE_SA (Entry);
> +    if (SaSession->LocalPeerSpi == SpiValue) {
> +      return TRUE;
> +    }
> +  }
> +
> +  return FALSE;
> +}
> 
>  /**
>    Call Crypto Lib to generate a random value with eight-octet length.
> 
>    @return the 64 byte vaule.
> @@ -156,23 +198,57 @@ IkePayloadFree (
>    FreePool (IkePayload);
>  }
> 
>  /**
>    Generate an new SPI.
> -
> -  @return a SPI in 4 bytes.
> +
> +  @param[in]      IkeSaSession   Pointer to IKEV2_SA_SESSION related to
> this Child SA
> +                                 Session.
> +  @param[in out]  SpiValue       Pointer to the new generated SPI value.
> +
> +  @retval EFI_SUCCESS         The operation performs successfully.
> +  @retval Otherwise           The operation is failed.
> 
>  **/
> -UINT32
> +EFI_STATUS
>  IkeGenerateSpi (
> -  VOID
> +  IN  IKEV2_SA_SESSION         *IkeSaSession,
> +  OUT UINT32                   *SpiValue
>    )
>  {
> -  //
> -  // TODO: should generate SPI randomly to avoid security issue
> -  //
> -  return mNextSpi++;
> +  EFI_STATUS   Status;
> +
> +  Status = EFI_SUCCESS;
> +
> +  while (TRUE) {
> +    //
> +    // Generate SPI randomly
> +    //
> +    Status = IpSecCryptoIoGenerateRandomBytes ((UINT8 *)SpiValue, sizeof
> (UINT32));
> +    if (EFI_ERROR (Status)) {
> +      break;
> +    }
> +
> +    //
> +    // The set of SPI values in the range 1 through 255 are reserved by
> the
> +    // Internet Assigned Numbers Authority (IANA) for future use; a
> reserved
> +    // SPI value will not normally be assigned by IANA unless the use of
> the
> +    // assigned SPI value is specified in an RFC.
> +    //
> +    if (*SpiValue < IKE_SPI_BASE) {
> +      *SpiValue += IKE_SPI_BASE;
> +    }
> +
> +    //
> +    // Check whether the new generated SPI has existed.
> +    //
> +    if (!IkeSpiValueExisted (IkeSaSession, *SpiValue)) {
> +      break;
> +    }
> +  }
> +
> +  return Status;
>  }
> 
>  /**
>    Generate a random data for IV
> 
> diff --git a/NetworkPkg/IpSecDxe/IkeCommon.h
> b/NetworkPkg/IpSecDxe/IkeCommon.h
> index 714ecaa..7f7fd4d 100644
> --- a/NetworkPkg/IpSecDxe/IkeCommon.h
> +++ b/NetworkPkg/IpSecDxe/IkeCommon.h
> @@ -1,9 +1,9 @@
>  /** @file
>    Common operation of the IKE.
> 
> -  Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> 
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the
> BSD License
>    which accompanies this distribution.  The full text of the license may
> be found at
>    http://opensource.org/licenses/bsd-license.php.
> @@ -37,11 +37,11 @@
> 
>  #define IKE_DEFAULT_PORT              500
>  #define IKE_DEFAULT_TIMEOUT_INTERVAL  10000 // 10s
>  #define IKE_NONCE_SIZE                16
>  #define IKE_MAX_RETRY                 4
> -#define IKE_SPI_BASE                  0x10000
> +#define IKE_SPI_BASE                  0x100
>  #define IKE_PAYLOAD_SIGNATURE         SIGNATURE_32('I','K','E','P')
>  #define IKE_PAYLOAD_BY_PACKET(a)
> CR(a,IKE_PAYLOAD,ByPacket,IKE_PAYLOAD_SIGNATURE)
> 
> 
>  #define IKE_PACKET_APPEND_PAYLOAD(IkePacket,IkePayload)                 \
> @@ -128,18 +128,24 @@ VOID
>  IkePayloadFree (
>    IN IKE_PAYLOAD *IkePayload
>    );
> 
>  /**
> -  Generate an unused SPI
> -
> -  @return a SPI in 4 bytes.
> +  Generate an new SPI.
> +
> +  @param[in]      IkeSaSession   Pointer to IKEV2_SA_SESSION related to
> this Child SA
> +                                 Session.
> +  @param[in out]  SpiValue       Pointer to the new generated SPI value.
> +
> +  @retval EFI_SUCCESS         The operation performs successfully.
> +  @retval Otherwise           The operation is failed.
> 
>  **/
> -UINT32
> +EFI_STATUS
>  IkeGenerateSpi (
> -  VOID
> +  IN  IKEV2_SA_SESSION         *IkeSaSession,
> +  OUT UINT32                   *SpiValue
>    );
> 
>  /**
>    Generate a random data for IV
> 
> diff --git a/NetworkPkg/IpSecDxe/Ikev2/Utility.c
> b/NetworkPkg/IpSecDxe/Ikev2/Utility.c
> index 5b26ba1..c365532 100644
> --- a/NetworkPkg/IpSecDxe/Ikev2/Utility.c
> +++ b/NetworkPkg/IpSecDxe/Ikev2/Utility.c
> @@ -523,11 +523,20 @@ Ikev2ChildSaSessionAlloc (
>    // Initialize the fields of ChildSaSession and its SessionCommon.
>    //
>    ChildSaSession->Signature          = IKEV2_CHILD_SA_SESSION_SIGNATURE;
>    ChildSaSession->IkeSaSession       = IkeSaSession;
>    ChildSaSession->MessageId          = IkeSaSession->MessageId;
> -  ChildSaSession->LocalPeerSpi       = IkeGenerateSpi ();
> +
> +  //
> +  // Generate an new SPI.
> +  //
> +  Status = IkeGenerateSpi (IkeSaSession, &(ChildSaSession->LocalPeerSpi));
> +  if (EFI_ERROR (Status)) {
> +    FreePool (ChildSaSession);
> +    return NULL;
> +  }
> +
>    ChildSaCommon                      = &ChildSaSession->SessionCommon;
>    ChildSaCommon->UdpService          = UdpService;
>    ChildSaCommon->Private             = IkeSaSession-
> >SessionCommon.Private;
>    ChildSaCommon->IkeSessionType      = IkeSessionTypeChildSa;
>    ChildSaCommon->IkeVer              = 2;
> --
> 1.9.5.msysgit.1