[edk2-devel] [PATCH v2 1/1] CryptoPkg/TlsLib: fix tls cipher configuration

["Gerd Hoffmann" <kraxel@redhat.com>]

Trying to configure the TLS ciphers can lead to TLS handshake failures
because TlsCipherMappingTable is not in line with the ciphers actually
supported by OpensslLib.

Fix that by removing TlsCipherMappingTable altogether.  Use
SSL_get_ciphers() instead to get the stack of ciphers supported by
openssl.  Name and ID of the ciphers can be queried using the
SSL_CIPHER_get_name() and SSL_CIPHER_get_protocol_id() functions,
which allows us to map IDs to names without a hard-code table.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=2541
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 164 ++++++---------------------
 1 file changed, 36 insertions(+), 128 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index f9333165a913..29d24abdca0f 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -9,65 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "InternalTlsLib.h"
 
-typedef struct {
-  //
-  // IANA/IETF defined Cipher Suite ID
-  //
-  UINT16         IanaCipher;
-  //
-  // OpenSSL-used Cipher Suite String
-  //
-  CONST CHAR8    *OpensslCipher;
-  //
-  // Length of OpensslCipher
-  //
-  UINTN          OpensslCipherLength;
-} TLS_CIPHER_MAPPING;
-
-//
-// Create a TLS_CIPHER_MAPPING initializer from IanaCipher and OpensslCipher so
-// that OpensslCipherLength is filled in automatically. IanaCipher must be an
-// integer constant expression, and OpensslCipher must be a string literal.
-//
-#define MAP(IanaCipher, OpensslCipher) \
-  { (IanaCipher), (OpensslCipher), sizeof (OpensslCipher) - 1 }
-
-//
-// The mapping table between IANA/IETF Cipher Suite definitions and
-// OpenSSL-used Cipher Suite name.
-//
-// Keep the table uniquely sorted by the IanaCipher field, in increasing order.
-//
-STATIC CONST TLS_CIPHER_MAPPING  TlsCipherMappingTable[] = {
-  MAP (0x0001, "NULL-MD5"),                         /// TLS_RSA_WITH_NULL_MD5
-  MAP (0x0002, "NULL-SHA"),                         /// TLS_RSA_WITH_NULL_SHA
-  MAP (0x0004, "RC4-MD5"),                          /// TLS_RSA_WITH_RC4_128_MD5
-  MAP (0x0005, "RC4-SHA"),                          /// TLS_RSA_WITH_RC4_128_SHA
-  MAP (0x000A, "DES-CBC3-SHA"),                     /// TLS_RSA_WITH_3DES_EDE_CBC_SHA, mandatory TLS 1.1
-  MAP (0x0016, "DHE-RSA-DES-CBC3-SHA"),             /// TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
-  MAP (0x002F, "AES128-SHA"),                       /// TLS_RSA_WITH_AES_128_CBC_SHA, mandatory TLS 1.2
-  MAP (0x0030, "DH-DSS-AES128-SHA"),                /// TLS_DH_DSS_WITH_AES_128_CBC_SHA
-  MAP (0x0031, "DH-RSA-AES128-SHA"),                /// TLS_DH_RSA_WITH_AES_128_CBC_SHA
-  MAP (0x0033, "DHE-RSA-AES128-SHA"),               /// TLS_DHE_RSA_WITH_AES_128_CBC_SHA
-  MAP (0x0035, "AES256-SHA"),                       /// TLS_RSA_WITH_AES_256_CBC_SHA
-  MAP (0x0036, "DH-DSS-AES256-SHA"),                /// TLS_DH_DSS_WITH_AES_256_CBC_SHA
-  MAP (0x0037, "DH-RSA-AES256-SHA"),                /// TLS_DH_RSA_WITH_AES_256_CBC_SHA
-  MAP (0x0039, "DHE-RSA-AES256-SHA"),               /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA
-  MAP (0x003B, "NULL-SHA256"),                      /// TLS_RSA_WITH_NULL_SHA256
-  MAP (0x003C, "AES128-SHA256"),                    /// TLS_RSA_WITH_AES_128_CBC_SHA256
-  MAP (0x003D, "AES256-SHA256"),                    /// TLS_RSA_WITH_AES_256_CBC_SHA256
-  MAP (0x003E, "DH-DSS-AES128-SHA256"),             /// TLS_DH_DSS_WITH_AES_128_CBC_SHA256
-  MAP (0x003F, "DH-RSA-AES128-SHA256"),             /// TLS_DH_RSA_WITH_AES_128_CBC_SHA256
-  MAP (0x0067, "DHE-RSA-AES128-SHA256"),            /// TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
-  MAP (0x0068, "DH-DSS-AES256-SHA256"),             /// TLS_DH_DSS_WITH_AES_256_CBC_SHA256
-  MAP (0x0069, "DH-RSA-AES256-SHA256"),             /// TLS_DH_RSA_WITH_AES_256_CBC_SHA256
-  MAP (0x006B, "DHE-RSA-AES256-SHA256"),            /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
-  MAP (0x009F, "DHE-RSA-AES256-GCM-SHA384"),        /// TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
-  MAP (0xC02B, "ECDHE-ECDSA-AES128-GCM-SHA256"),    /// TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
-  MAP (0xC02C, "ECDHE-ECDSA-AES256-GCM-SHA384"),    /// TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
-  MAP (0xC030, "ECDHE-RSA-AES256-GCM-SHA384"),      /// TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
-};
-
 typedef struct {
   //
   // TLS Algorithm
@@ -96,54 +37,6 @@ STATIC CONST TLS_ALGO_TO_NAME  TlsSignatureAlgoToName[] = {
   { TlsSignatureAlgoEcdsa,     "ECDSA" },
 };
 
-/**
-  Gets the OpenSSL cipher suite mapping for the supplied IANA TLS cipher suite.
-
-  @param[in]  CipherId    The supplied IANA TLS cipher suite ID.
-
-  @return  The corresponding OpenSSL cipher suite mapping if found,
-           NULL otherwise.
-
-**/
-STATIC
-CONST TLS_CIPHER_MAPPING *
-TlsGetCipherMapping (
-  IN     UINT16  CipherId
-  )
-{
-  INTN  Left;
-  INTN  Right;
-  INTN  Middle;
-
-  //
-  // Binary Search Cipher Mapping Table for IANA-OpenSSL Cipher Translation
-  //
-  Left  = 0;
-  Right = ARRAY_SIZE (TlsCipherMappingTable) - 1;
-
-  while (Right >= Left) {
-    Middle = (Left + Right) / 2;
-
-    if (CipherId == TlsCipherMappingTable[Middle].IanaCipher) {
-      //
-      // Translate IANA cipher suite ID to OpenSSL name.
-      //
-      return &TlsCipherMappingTable[Middle];
-    }
-
-    if (CipherId < TlsCipherMappingTable[Middle].IanaCipher) {
-      Right = Middle - 1;
-    } else {
-      Left = Middle + 1;
-    }
-  }
-
-  //
-  // No Cipher Mapping found, return NULL.
-  //
-  return NULL;
-}
-
 /**
   Set a new TLS/SSL method for a particular TLS object.
 
@@ -281,16 +174,21 @@ TlsSetCipherList (
   IN     UINTN   CipherNum
   )
 {
-  TLS_CONNECTION            *TlsConn;
-  EFI_STATUS                Status;
-  CONST TLS_CIPHER_MAPPING  **MappedCipher;
-  UINTN                     MappedCipherBytes;
-  UINTN                     MappedCipherCount;
-  UINTN                     CipherStringSize;
-  UINTN                     Index;
-  CONST TLS_CIPHER_MAPPING  *Mapping;
-  CHAR8                     *CipherString;
-  CHAR8                     *CipherStringPosition;
+  TLS_CONNECTION    *TlsConn;
+  EFI_STATUS        Status;
+  CONST SSL_CIPHER  **MappedCipher;
+  UINTN             MappedCipherBytes;
+  UINTN             MappedCipherCount;
+  UINTN             CipherStringSize;
+  UINTN             Index;
+  INT32             StackIdx;
+  CHAR8             *CipherString;
+  CHAR8             *CipherStringPosition;
+
+  STACK_OF (SSL_CIPHER)      *OpensslCipherStack;
+  CONST SSL_CIPHER  *OpensslCipher;
+  CONST CHAR8       *OpensslCipherName;
+  UINTN             OpensslCipherNameLength;
 
   TlsConn = (TLS_CONNECTION *)Tls;
   if ((TlsConn == NULL) || (TlsConn->Ssl == NULL) || (CipherId == NULL)) {
@@ -315,18 +213,26 @@ TlsSetCipherList (
     return EFI_OUT_OF_RESOURCES;
   }
 
+  OpensslCipherStack = SSL_get_ciphers (TlsConn->Ssl);
+
   //
   // Map the cipher IDs, and count the number of bytes for the full
   // CipherString.
   //
   MappedCipherCount = 0;
   CipherStringSize  = 0;
-  for (Index = 0; Index < CipherNum; Index++) {
+  for (Index = 0; OpensslCipherStack != NULL && Index < CipherNum; Index++) {
     //
     // Look up the IANA-to-OpenSSL mapping.
     //
-    Mapping = TlsGetCipherMapping (CipherId[Index]);
-    if (Mapping == NULL) {
+    for (StackIdx = 0; StackIdx < sk_SSL_CIPHER_num (OpensslCipherStack); StackIdx++) {
+      OpensslCipher = sk_SSL_CIPHER_value (OpensslCipherStack, StackIdx);
+      if (CipherId[Index] == SSL_CIPHER_get_protocol_id (OpensslCipher)) {
+        break;
+      }
+    }
+
+    if (StackIdx == sk_SSL_CIPHER_num (OpensslCipherStack)) {
       DEBUG ((
         DEBUG_VERBOSE,
         "%a:%a: skipping CipherId=0x%04x\n",
@@ -343,7 +249,7 @@ TlsSetCipherList (
     }
 
     //
-    // Accumulate Mapping->OpensslCipherLength into CipherStringSize. If this
+    // Accumulate cipher name string length into CipherStringSize. If this
     // is not the first successful mapping, account for a colon (":") prefix
     // too.
     //
@@ -357,7 +263,7 @@ TlsSetCipherList (
 
     Status = SafeUintnAdd (
                CipherStringSize,
-               Mapping->OpensslCipherLength,
+               AsciiStrLen (SSL_CIPHER_get_name (OpensslCipher)),
                &CipherStringSize
                );
     if (EFI_ERROR (Status)) {
@@ -368,7 +274,7 @@ TlsSetCipherList (
     //
     // Record the mapping.
     //
-    MappedCipher[MappedCipherCount++] = Mapping;
+    MappedCipher[MappedCipherCount++] = OpensslCipher;
   }
 
   //
@@ -403,10 +309,12 @@ TlsSetCipherList (
   //
   CipherStringPosition = CipherString;
   for (Index = 0; Index < MappedCipherCount; Index++) {
-    Mapping = MappedCipher[Index];
+    OpensslCipher           = MappedCipher[Index];
+    OpensslCipherName       = SSL_CIPHER_get_name (OpensslCipher);
+    OpensslCipherNameLength = AsciiStrLen (OpensslCipherName);
     //
     // Append the colon (":") prefix except for the first mapping, then append
-    // Mapping->OpensslCipher.
+    // OpensslCipherName.
     //
     if (Index > 0) {
       *(CipherStringPosition++) = ':';
@@ -414,10 +322,10 @@ TlsSetCipherList (
 
     CopyMem (
       CipherStringPosition,
-      Mapping->OpensslCipher,
-      Mapping->OpensslCipherLength
+      OpensslCipherName,
+      OpensslCipherNameLength
       );
-    CipherStringPosition += Mapping->OpensslCipherLength;
+    CipherStringPosition += OpensslCipherNameLength;
   }
 
   //
-- 
2.41.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109312): https://edk2.groups.io/g/devel/message/109312
Mute This Topic: https://groups.io/mt/101751673/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-