CryptoPkg/TlsLib: rewrite TlsSetCipherList()

Rewrite the TlsSetCipherList() function in order to fix the following
issues:

- Any cipher identifier in CipherId that is not recognized by
  TlsGetCipherMapping() will cause the function to return EFI_UNSUPPORTED.

  This is a problem because CipherId is an ordered preference list, and a
  caller should not get EFI_UNSUPPORTED just because it has an elaborate
  CipherId preference list. Instead, we can filter out cipher identifiers
  that we don't recognize, as long as we keep the relative order intact.

- CipherString is allocated on the stack, with 500 bytes.

  While processing a large CipherId preference list, this room may not be
  enough. Although no buffer overflow is possible, CipherString exhaustion
  can lead to a failed TLS connection, because any cipher names that don't
  fit on CipherString cannot be negotiated.

  Compute CipherStringSize first, and allocate CipherString dynamically.

- Finally, the "@STRENGTH" pseudo cipher name is appended to CipherString.
  (Assuming there is enough room left in CipherString.) This causes
  OpenSSL to sort the cipher list "in order of encryption algorithm key
  length".

  This is a bad idea. The caller specifically passes an ordered preference
  list in CipherId. Therefore TlsSetCipherList() must not ask OpenSSL to
  reorder the list, for any reason. Drop "@STRENGTH".

While at it, fix and unify the documentation of the CipherId parameter.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>

4 files changed, 157 insertions, 26 deletions

diff --git a/CryptoPkg/Include/Library/TlsLib.h b/CryptoPkg/Include/Library/TlsLib.h
index e19a38a214..e71291eaea 100644
--- a/CryptoPkg/Include/Library/TlsLib.h
+++ b/CryptoPkg/Include/Library/TlsLib.h
@@ -348,13 +348,16 @@ TlsSetConnectionEnd (
   This function sets the ciphers for use by a specified TLS object.

 

   @param[in]  Tls          Pointer to a TLS object.

-  @param[in]  CipherId     Pointer to a string that contains one or more

-                           ciphers separated by a colon.

+  @param[in]  CipherId     Array of UINT16 cipher identifiers. Each UINT16

+                           cipher identifier comes from the TLS Cipher Suite

+                           Registry of the IANA, interpreting Byte1 and Byte2

+                           in network (big endian) byte order.

   @param[in]  CipherNum    The number of cipher in the list.

 

   @retval  EFI_SUCCESS           The ciphers list was set successfully.

   @retval  EFI_INVALID_PARAMETER The parameter is invalid.

-  @retval  EFI_UNSUPPORTED       Unsupported TLS cipher in the list.

+  @retval  EFI_UNSUPPORTED       No supported TLS cipher was found in CipherId.

+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.

 

 **/

 EFI_STATUS

diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
index 3f18a461a8..b6cf9816aa 100644
--- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
+++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
@@ -19,9 +19,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #undef _WIN64

 

 #include <Library/BaseCryptLib.h>

-#include <Library/BaseLib.h>

 #include <Library/BaseMemoryLib.h>

 #include <Library/DebugLib.h>

+#include <Library/MemoryAllocationLib.h>

+#include <Library/SafeIntLib.h>

 #include <openssl/ssl.h>

 #include <openssl/bio.h>

 #include <openssl/err.h>

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index e2f819b903..9154686610 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -235,12 +235,16 @@ TlsSetConnectionEnd (
   This function sets the ciphers for use by a specified TLS object.

 

   @param[in]  Tls          Pointer to a TLS object.

-  @param[in]  CipherId     Pointer to a UINT16 cipher Id.

+  @param[in]  CipherId     Array of UINT16 cipher identifiers. Each UINT16

+                           cipher identifier comes from the TLS Cipher Suite

+                           Registry of the IANA, interpreting Byte1 and Byte2

+                           in network (big endian) byte order.

   @param[in]  CipherNum    The number of cipher in the list.

 

   @retval  EFI_SUCCESS           The ciphers list was set successfully.

   @retval  EFI_INVALID_PARAMETER The parameter is invalid.

-  @retval  EFI_UNSUPPORTED       Unsupported TLS cipher in the list.

+  @retval  EFI_UNSUPPORTED       No supported TLS cipher was found in CipherId.

+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.

 

 **/

 EFI_STATUS

@@ -252,51 +256,173 @@ TlsSetCipherList (
   )

 {

   TLS_CONNECTION           *TlsConn;

+  EFI_STATUS               Status;

+  CONST TLS_CIPHER_MAPPING **MappedCipher;

+  UINTN                    MappedCipherBytes;

+  UINTN                    MappedCipherCount;

+  UINTN                    CipherStringSize;

   UINTN                    Index;

   CONST TLS_CIPHER_MAPPING *Mapping;

-  CONST CHAR8              *MappingName;

-  CHAR8                    CipherString[500];

+  CHAR8                    *CipherString;

+  CHAR8                    *CipherStringPosition;

 

   TlsConn = (TLS_CONNECTION *) Tls;

   if (TlsConn == NULL || TlsConn->Ssl == NULL || CipherId == NULL) {

     return EFI_INVALID_PARAMETER;

   }

 

-  Mapping     = NULL;

-  MappingName = NULL;

-

-  memset (CipherString, 0, sizeof (CipherString));

+  //

+  // Allocate the MappedCipher array for recording the mappings that we find

+  // for the input IANA identifiers in CipherId.

+  //

+  Status = SafeUintnMult (CipherNum, sizeof (*MappedCipher),

+             &MappedCipherBytes);

+  if (EFI_ERROR (Status)) {

+    return EFI_OUT_OF_RESOURCES;

+  }

+  MappedCipher = AllocatePool (MappedCipherBytes);

+  if (MappedCipher == NULL) {

+    return EFI_OUT_OF_RESOURCES;

+  }

 

+  //

+  // Map the cipher IDs, and count the number of bytes for the full

+  // CipherString.

+  //

+  MappedCipherCount = 0;

+  CipherStringSize = 0;

   for (Index = 0; Index < CipherNum; Index++) {

     //

-    // Handling OpenSSL / RFC Cipher name mapping.

+    // Look up the IANA-to-OpenSSL mapping.

     //

-    Mapping = TlsGetCipherMapping (*(CipherId + Index));

+    Mapping = TlsGetCipherMapping (CipherId[Index]);

     if (Mapping == NULL) {

-      return EFI_UNSUPPORTED;

-    }

-    MappingName = Mapping->OpensslCipher;

-

-    if (Index != 0) {

+      DEBUG ((DEBUG_VERBOSE, "%a:%a: skipping CipherId=0x%04x\n",

+        gEfiCallerBaseName, __FUNCTION__, CipherId[Index]));

       //

-      // The ciphers were separated by a colon.

+      // Skipping the cipher is valid because CipherId is an ordered

+      // preference list of ciphers, thus we can filter it as long as we

+      // don't change the relative order of elements on it.

       //

-      AsciiStrCatS (CipherString, sizeof (CipherString), ":");

+      continue;

     }

+    //

+    // Accumulate Mapping->OpensslCipherLength into CipherStringSize. If this

+    // is not the first successful mapping, account for a colon (":") prefix

+    // too.

+    //

+    if (MappedCipherCount > 0) {

+      Status = SafeUintnAdd (CipherStringSize, 1, &CipherStringSize);

+      if (EFI_ERROR (Status)) {

+        Status = EFI_OUT_OF_RESOURCES;

+        goto FreeMappedCipher;

+      }

+    }

+    Status = SafeUintnAdd (CipherStringSize, Mapping->OpensslCipherLength,

+               &CipherStringSize);

+    if (EFI_ERROR (Status)) {

+      Status = EFI_OUT_OF_RESOURCES;

+      goto FreeMappedCipher;

+    }

+    //

+    // Record the mapping.

+    //

+    MappedCipher[MappedCipherCount++] = Mapping;

+  }

+

+  //

+  // Verify that at least one IANA cipher ID could be mapped; account for the

+  // terminating NUL character in CipherStringSize; allocate CipherString.

+  //

+  if (MappedCipherCount == 0) {

+    DEBUG ((DEBUG_ERROR, "%a:%a: no CipherId could be mapped\n",

+      gEfiCallerBaseName, __FUNCTION__));

+    Status = EFI_UNSUPPORTED;

+    goto FreeMappedCipher;

+  }

+  Status = SafeUintnAdd (CipherStringSize, 1, &CipherStringSize);

+  if (EFI_ERROR (Status)) {

+    Status = EFI_OUT_OF_RESOURCES;

+    goto FreeMappedCipher;

+  }

+  CipherString = AllocatePool (CipherStringSize);

+  if (CipherString == NULL) {

+    Status = EFI_OUT_OF_RESOURCES;

+    goto FreeMappedCipher;

+  }

 

-    AsciiStrCatS (CipherString, sizeof (CipherString), MappingName);

+  //

+  // Go over the collected mappings and populate CipherString.

+  //

+  CipherStringPosition = CipherString;

+  for (Index = 0; Index < MappedCipherCount; Index++) {

+    Mapping = MappedCipher[Index];

+    //

+    // Append the colon (":") prefix except for the first mapping, then append

+    // Mapping->OpensslCipher.

+    //

+    if (Index > 0) {

+      *(CipherStringPosition++) = ':';

+    }

+    CopyMem (CipherStringPosition, Mapping->OpensslCipher,

+      Mapping->OpensslCipherLength);

+    CipherStringPosition += Mapping->OpensslCipherLength;

   }

 

-  AsciiStrCatS (CipherString, sizeof (CipherString), ":@STRENGTH");

+  //

+  // NUL-terminate CipherString.

+  //

+  *(CipherStringPosition++) = '\0';

+  ASSERT (CipherStringPosition == CipherString + CipherStringSize);

+

+  //

+  // Log CipherString for debugging. CipherString can be very long if the

+  // caller provided a large CipherId array, so log CipherString in segments of

+  // 79 non-newline characters. (MAX_DEBUG_MESSAGE_LENGTH is usually 0x100 in

+  // DebugLib instances.)

+  //

+  DEBUG_CODE (

+    UINTN FullLength;

+    UINTN SegmentLength;

+

+    FullLength = CipherStringSize - 1;

+    DEBUG ((DEBUG_VERBOSE, "%a:%a: CipherString={\n", gEfiCallerBaseName,

+      __FUNCTION__));

+    for (CipherStringPosition = CipherString;

+         CipherStringPosition < CipherString + FullLength;

+         CipherStringPosition += SegmentLength) {

+      SegmentLength = FullLength - (CipherStringPosition - CipherString);

+      if (SegmentLength > 79) {

+        SegmentLength = 79;

+      }

+      DEBUG ((DEBUG_VERBOSE, "%.*a\n", SegmentLength, CipherStringPosition));

+    }

+    DEBUG ((DEBUG_VERBOSE, "}\n"));

+    //

+    // Restore the pre-debug value of CipherStringPosition by skipping over the

+    // trailing NUL.

+    //

+    CipherStringPosition++;

+    ASSERT (CipherStringPosition == CipherString + CipherStringSize);

+  );

 

   //

   // Sets the ciphers for use by the Tls object.

   //

   if (SSL_set_cipher_list (TlsConn->Ssl, CipherString) <= 0) {

-    return EFI_UNSUPPORTED;

+    Status = EFI_UNSUPPORTED;

+    goto FreeCipherString;

   }

 

-  return EFI_SUCCESS;

+  Status = EFI_SUCCESS;

+

+FreeCipherString:

+  FreePool (CipherString);

+

+FreeMappedCipher:

+  FreePool (MappedCipher);

+

+  return Status;

 }

 

 /**

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index ae17a7d874..4dacb2fab0 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -40,11 +40,12 @@
 

 [LibraryClasses]

   BaseCryptLib

-  BaseLib

   BaseMemoryLib

   DebugLib

   IntrinsicLib

+  MemoryAllocationLib

   OpensslLib

+  SafeIntLib

 

 [BuildOptions]

   #