From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id A6632D80298 for ; Sat, 30 Sep 2023 23:20:15 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=yH5OuF0YjntKoLv9RZ1pwbae1skSddn4UsJWjRgC+Jw=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696116014; v=1; b=QpGMvPKZ8STUr84NNcfYQzLX3vV4nvrTrwk/Cq9SAyZOBhck7jo79bMlzsSvJqnyjB8ozUG3 0OaxbrNIc9MOJ+GQV4xl4t7X6bQ0HhYukYJ1sVvCIXwCghuGZn4sdrWxgsojfK3BAa7a3Wezamf ywMDRDBOcvW/RRyVwzyW89VM= X-Received: by 127.0.0.2 with SMTP id ozSQYY7687511xo0ieWjUvuk; Sat, 30 Sep 2023 16:20:14 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.51081.1696116013414034651 for ; Sat, 30 Sep 2023 16:20:13 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-677-fqDA5A1iNAi4x4Cw5y-7wA-1; Sat, 30 Sep 2023 19:20:10 -0400 X-MC-Unique: fqDA5A1iNAi4x4Cw5y-7wA-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 37A378007A4; Sat, 30 Sep 2023 23:20:10 +0000 (UTC) X-Received: from [10.39.192.54] (unknown [10.39.192.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 99F0040C6EBF; Sat, 30 Sep 2023 23:20:08 +0000 (UTC) Message-ID: <028c50b9-dc23-781b-ce1a-c817a81f9e6e@redhat.com> Date: Sun, 1 Oct 2023 01:20:07 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration To: devel@edk2.groups.io, kraxel@redhat.com Cc: Yi Li , Guomin Jiang , Xiaoyu Lu , Jiewen Yao , Oliver Steffen References: <20230929132318.2901308-1-kraxel@redhat.com> From: "Laszlo Ersek" In-Reply-To: <20230929132318.2901308-1-kraxel@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: oYyngeD6CswnDgctraf10hWDx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=QpGMvPKZ; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 9/29/23 15:23, Gerd Hoffmann wrote: > 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. >=20 > 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. >=20 > Signed-off-by: Gerd Hoffmann > --- > CryptoPkg/Library/TlsLib/TlsConfig.c | 158 ++++++--------------------- > 1 file changed, 33 insertions(+), 125 deletions(-) >=20 > diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/Tls= Lib/TlsConfig.c > index f9333165a913..0ca6994b6586 100644 > --- a/CryptoPkg/Library/TlsLib/TlsConfig.c > +++ b/CryptoPkg/Library/TlsLib/TlsConfig.c > @@ -9,65 +9,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > =20 > #include "InternalTlsLib.h" > =20 > -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 OpensslCi= pher so > -// that OpensslCipherLength is filled in automatically. IanaCipher must = be an > -// integer constant expression, and OpensslCipher must be a string liter= al. > -// > -#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[] =3D { > - MAP (0x0001, "NULL-MD5"), /// TLS_RSA_WITH_NUL= L_MD5 > - MAP (0x0002, "NULL-SHA"), /// TLS_RSA_WITH_NUL= L_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_3DE= S_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_NUL= L_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_WI= TH_AES_256_GCM_SHA384 > -}; > - > typedef struct { > // > // TLS Algorithm > @@ -96,54 +37,6 @@ STATIC CONST TLS_ALGO_TO_NAME TlsSignatureAlgoToName[= ] =3D { > { TlsSignatureAlgoEcdsa, "ECDSA" }, > }; > =20 > -/** > - 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 Translat= ion > - // > - Left =3D 0; > - Right =3D ARRAY_SIZE (TlsCipherMappingTable) - 1; > - > - while (Right >=3D Left) { > - Middle =3D (Left + Right) / 2; > - > - if (CipherId =3D=3D TlsCipherMappingTable[Middle].IanaCipher) { > - // > - // Translate IANA cipher suite ID to OpenSSL name. > - // > - return &TlsCipherMappingTable[Middle]; > - } > - > - if (CipherId < TlsCipherMappingTable[Middle].IanaCipher) { > - Right =3D Middle - 1; > - } else { > - Left =3D Middle + 1; > - } > - } > - > - // > - // No Cipher Mapping found, return NULL. > - // > - return NULL; > -} > - > /** > Set a new TLS/SSL method for a particular TLS object. > =20 > @@ -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; Surprisingly, this does pass uncrustify. :) OK. > + CONST SSL_CIPHER *OpensslCipher; > + CONST CHAR8 *OpensslCipherName; > + UINTN OpensslCipherNameSize; > =20 > TlsConn =3D (TLS_CONNECTION *)Tls; > if ((TlsConn =3D=3D NULL) || (TlsConn->Ssl =3D=3D NULL) || (CipherId = =3D=3D NULL)) { > @@ -315,6 +213,8 @@ TlsSetCipherList ( > return EFI_OUT_OF_RESOURCES; > } > =20 > + OpensslCipherStack =3D SSL_get_ciphers (TlsConn->Ssl); > + > // > // Map the cipher IDs, and count the number of bytes for the full > // CipherString. Per documentation, this does not need freeing, indeed. OK. > @@ -325,8 +225,14 @@ TlsSetCipherList ( > // > // Look up the IANA-to-OpenSSL mapping. > // > - Mapping =3D TlsGetCipherMapping (CipherId[Index]); > - if (Mapping =3D=3D NULL) { > + for (StackIdx =3D 0; StackIdx < sk_SSL_CIPHER_num (OpensslCipherStac= k); StackIdx++) { Surprisingly, StackIdx really does have to be INT32 type here; through multiple macro definitions, sk_SSL_CIPHER_num() seems to expand to OPENSSL_sk_num(), and that returns a plain "int". OK. (1) Problem: if SSL_get_ciphers() fails above ("no ciphers are available"), then OpensslCipherStack is NULL here. For a NULL argument, sk_SSL_CIPHER_num() returns -- I think! -- the value (-1). See OPENSSL_sk_num(). Then this loop will never run (which is fine), but *after* the loop, our condition for "not found" will not hold, because StackIdx will be zero, but sk_SSL_CIPHER_num() will return (-1). > + OpensslCipher =3D sk_SSL_CIPHER_value (OpensslCipherStack, StackId= x); Using "apps/ciphers.c" as an example, this is good, and needs no freeing (pointer to const). OK. > + if (CipherId[Index] =3D=3D SSL_CIPHER_get_protocol_id (OpensslCiph= er)) { > + break; > + } > + } > + > + if (StackIdx =3D=3D sk_SSL_CIPHER_num (OpensslCipherStack)) { > DEBUG (( > DEBUG_VERBOSE, > "%a:%a: skipping CipherId=3D0x%04x\n", (2) We could salvage this by writing (OpensslCipherStack =3D=3D NULL) || (StackIdx =3D=3D sk_SSL_CIPHER_num (OpensslCipherStack)) but it's probably better to just skip the whole outer loop if "OpensslCipherStack" is NULL (as no lookup will succeed). > @@ -357,7 +263,7 @@ TlsSetCipherList ( (3) The comment (not shown in the context) Accumulate Mapping->OpensslCipherLength into CipherStringSize has not been updated. There is no "Mapping" anymore. > =20 > Status =3D SafeUintnAdd ( > CipherStringSize, > - Mapping->OpensslCipherLength, > + AsciiStrnLenS (SSL_CIPHER_get_name (OpensslCipher), MAX_S= TRING_SIZE), SSL_CIPHER_get_name() returns pointer-to-const, so it needs no freeing. OK. (4) Why AsciiStrnLenS(), and not AsciiStrLen()? > &CipherStringSize > ); > if (EFI_ERROR (Status)) { > @@ -368,7 +274,7 @@ TlsSetCipherList ( > // > // Record the mapping. > // > - MappedCipher[MappedCipherCount++] =3D Mapping; > + MappedCipher[MappedCipherCount++] =3D OpensslCipher; > } > =20 > // > @@ -403,7 +309,9 @@ TlsSetCipherList ( > // > CipherStringPosition =3D CipherString; > for (Index =3D 0; Index < MappedCipherCount; Index++) { > - Mapping =3D MappedCipher[Index]; > + OpensslCipher =3D MappedCipher[Index]; > + OpensslCipherName =3D SSL_CIPHER_get_name (OpensslCipher); > + OpensslCipherNameSize =3D AsciiStrnLenS (OpensslCipherName, MAX_STRI= NG_SIZE); (5) Better call this "OpensslCipherNameLength" ("Size" suggests including the terminating NUL). (6) Same as (4) -- should be AsciiStrLen() IMO. > // > // Append the colon (":") prefix except for the first mapping, then = append > // Mapping->OpensslCipher. (7) Stale comment, there's no "Mapping" anymore. Should say "append OpensslCipherName". > @@ -414,10 +322,10 @@ TlsSetCipherList ( > =20 > CopyMem ( > CipherStringPosition, > - Mapping->OpensslCipher, > - Mapping->OpensslCipherLength > + OpensslCipherName, > + OpensslCipherNameSize > ); > - CipherStringPosition +=3D Mapping->OpensslCipherLength; > + CipherStringPosition +=3D OpensslCipherNameSize; > } > =20 > // (8) Can you - assign to yoursel= f, - link this patch into a comment on that BZ, - and reference the BZ URL in the commit message? Thanks! Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109237): https://edk2.groups.io/g/devel/message/109237 Mute This Topic: https://groups.io/mt/101657203/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-