Re: [edk2-devel] setting TLS ciphers is broken (openssl 3?)

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

  Hi,

> > (2) TlsCipherMappingTable [CryptoPkg/Library/TlsLib/TlsConfig.c]

> That seems to be the case.  Maybe (2) needs an update to enable newer
> ciphers, when logging the mappings I see there are quite a few IDs which
> don't get mapped:

Turns out we don't need TlsCipherMappingTable at all,
we can just query the openssl library instead.

take care,
  Gerd

--------------------------- cut here ---------------------------
From 47f94a52d8e7a21ec3f0ff05ac7d146beff5be3b Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Fri, 29 Sep 2023 12:03:18 +0200
Subject: [PATCH 1/2] CryptoPkg: fix tls config

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 157 ++++++---------------------
 1 file changed, 32 insertions(+), 125 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index f9333165a913..2e5210193c6b 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,20 @@ 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, StackIdx;
+  CHAR8             *CipherString;
+  CHAR8             *CipherStringPosition;
+
+  STACK_OF (SSL_CIPHER)      *OpensslCipherStack;
+  CONST SSL_CIPHER  *OpensslCipher;
+  CONST CHAR8       *OpensslCipherName;
+  UINTN             OpensslCipherNameSize;
 
   TlsConn = (TLS_CONNECTION *)Tls;
   if ((TlsConn == NULL) || (TlsConn->Ssl == NULL) || (CipherId == NULL)) {
@@ -315,6 +212,8 @@ 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.
@@ -325,8 +224,14 @@ TlsSetCipherList (
     //
     // 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",
@@ -357,7 +262,7 @@ TlsSetCipherList (
 
     Status = SafeUintnAdd (
                CipherStringSize,
-               Mapping->OpensslCipherLength,
+               AsciiStrnLenS (SSL_CIPHER_get_name (OpensslCipher), MAX_STRING_SIZE),
                &CipherStringSize
                );
     if (EFI_ERROR (Status)) {
@@ -368,7 +273,7 @@ TlsSetCipherList (
     //
     // Record the mapping.
     //
-    MappedCipher[MappedCipherCount++] = Mapping;
+    MappedCipher[MappedCipherCount++] = OpensslCipher;
   }
 
   //
@@ -403,7 +308,9 @@ TlsSetCipherList (
   //
   CipherStringPosition = CipherString;
   for (Index = 0; Index < MappedCipherCount; Index++) {
-    Mapping = MappedCipher[Index];
+    OpensslCipher         = MappedCipher[Index];
+    OpensslCipherName     = SSL_CIPHER_get_name (OpensslCipher);
+    OpensslCipherNameSize = AsciiStrnLenS (OpensslCipherName, MAX_STRING_SIZE);
     //
     // Append the colon (":") prefix except for the first mapping, then append
     // Mapping->OpensslCipher.
@@ -414,10 +321,10 @@ TlsSetCipherList (
 
     CopyMem (
       CipherStringPosition,
-      Mapping->OpensslCipher,
-      Mapping->OpensslCipherLength
+      OpensslCipherName,
+      OpensslCipherNameSize
       );
-    CipherStringPosition += Mapping->OpensslCipherLength;
+    CipherStringPosition += OpensslCipherNameSize;
   }
 
   //
-- 
2.41.0



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