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

["Laszlo Ersek" <lersek@redhat.com>]

On 10/7/23 16:32, Li, Yi wrote:
> Sorry for delayed response due to PRC holiday.
> This is a pretty good solution, I also ran some basic HTTPSBOOT and EAP-TLS test cases, and all passed.
> 
> Reviewed-by: Yi Li <yi1.li@intel.com>

Merged as commit 4ddd8ac3a29d via
<https://github.com/tianocore/edk2/pull/4898>.

Laszlo

> 
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com> 
> Sent: Wednesday, October 4, 2023 5:20 PM
> To: devel@edk2.groups.io
> Cc: Li, Yi1 <yi1.li@intel.com>; Pawel Polawski <ppolawsk@redhat.com>; László Érsek <lersek@redhat.com>; Jiang, Guomin <guomin.jiang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Oliver Steffen <osteffen@redhat.com>; Lu, Xiaoyu1 <xiaoyu1.lu@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v2 1/1] CryptoPkg/TlsLib: fix tls cipher configuration
> 
> 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 (#109390): https://edk2.groups.io/g/devel/message/109390
Mute This Topic: https://groups.io/mt/101751673/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-