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

["Ye, Ting" <ting.ye@intel.com>]

Reviewed-by: Ye Ting <ting.ye@intel.com> 

-----Original Message-----
From: Wu, Jiaxin 
Sent: Tuesday, September 06, 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