* [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration
@ 2023-09-29 13:23 Gerd Hoffmann
2023-09-30 23:20 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Gerd Hoffmann @ 2023-09-29 13:23 UTC (permalink / raw)
To: devel
Cc: László Érsek, Yi Li, Guomin Jiang, Xiaoyu Lu,
Jiewen Yao, Oliver Steffen, Gerd Hoffmann
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.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
CryptoPkg/Library/TlsLib/TlsConfig.c | 158 ++++++---------------------
1 file changed, 33 insertions(+), 125 deletions(-)
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/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
#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 OpensslCipherNameSize;
TlsConn = (TLS_CONNECTION *)Tls;
if ((TlsConn == NULL) || (TlsConn->Ssl == NULL) || (CipherId == NULL)) {
@@ -315,6 +213,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 +225,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 +263,7 @@ TlsSetCipherList (
Status = SafeUintnAdd (
CipherStringSize,
- Mapping->OpensslCipherLength,
+ AsciiStrnLenS (SSL_CIPHER_get_name (OpensslCipher), MAX_STRING_SIZE),
&CipherStringSize
);
if (EFI_ERROR (Status)) {
@@ -368,7 +274,7 @@ TlsSetCipherList (
//
// Record the mapping.
//
- MappedCipher[MappedCipherCount++] = Mapping;
+ MappedCipher[MappedCipherCount++] = OpensslCipher;
}
//
@@ -403,7 +309,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 +322,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 (#109198): https://edk2.groups.io/g/devel/message/109198
Mute This Topic: https://groups.io/mt/101657203/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration
2023-09-29 13:23 [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration Gerd Hoffmann
@ 2023-09-30 23:20 ` Laszlo Ersek
2023-10-04 9:19 ` Gerd Hoffmann
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2023-09-30 23:20 UTC (permalink / raw)
To: devel, kraxel; +Cc: Yi Li, Guomin Jiang, Xiaoyu Lu, Jiewen Yao, Oliver Steffen
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.
>
> 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.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> CryptoPkg/Library/TlsLib/TlsConfig.c | 158 ++++++---------------------
> 1 file changed, 33 insertions(+), 125 deletions(-)
>
> diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/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
>
> #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;
Surprisingly, this does pass uncrustify. :) OK.
> + CONST SSL_CIPHER *OpensslCipher;
> + CONST CHAR8 *OpensslCipherName;
> + UINTN OpensslCipherNameSize;
>
> TlsConn = (TLS_CONNECTION *)Tls;
> if ((TlsConn == NULL) || (TlsConn->Ssl == NULL) || (CipherId == NULL)) {
> @@ -315,6 +213,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.
Per documentation, this does not need freeing, indeed. OK.
> @@ -325,8 +225,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++) {
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 = sk_SSL_CIPHER_value (OpensslCipherStack, StackIdx);
Using "apps/ciphers.c" as an example, this is good, and needs no freeing
(pointer to const). OK.
> + 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",
(2) We could salvage this by writing
(OpensslCipherStack == NULL) ||
(StackIdx == 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.
>
> Status = SafeUintnAdd (
> CipherStringSize,
> - Mapping->OpensslCipherLength,
> + AsciiStrnLenS (SSL_CIPHER_get_name (OpensslCipher), MAX_STRING_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++] = Mapping;
> + MappedCipher[MappedCipherCount++] = OpensslCipher;
> }
>
> //
> @@ -403,7 +309,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);
(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 (
>
> CopyMem (
> CipherStringPosition,
> - Mapping->OpensslCipher,
> - Mapping->OpensslCipherLength
> + OpensslCipherName,
> + OpensslCipherNameSize
> );
> - CipherStringPosition += Mapping->OpensslCipherLength;
> + CipherStringPosition += OpensslCipherNameSize;
> }
>
> //
(8) Can you
- assign <https://bugzilla.tianocore.org/show_bug.cgi?id=2541> to yourself,
- link this patch into a comment on that BZ,
- and reference the BZ URL in the commit message?
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration
2023-09-30 23:20 ` Laszlo Ersek
@ 2023-10-04 9:19 ` Gerd Hoffmann
0 siblings, 0 replies; 3+ messages in thread
From: Gerd Hoffmann @ 2023-10-04 9:19 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Yi Li, Guomin Jiang, Xiaoyu Lu, Jiewen Yao, Oliver Steffen
Hi,
> > + CHAR8 *CipherString;
> > + CHAR8 *CipherStringPosition;
> > +
> > + STACK_OF (SSL_CIPHER) *OpensslCipherStack;
>
> Surprisingly, this does pass uncrustify. :) OK.
Wasn't my idea, uncrustify actually formatted it that way for me ;)
> > - Mapping = TlsGetCipherMapping (CipherId[Index]);
> > - if (Mapping == NULL) {
> > + for (StackIdx = 0; StackIdx < sk_SSL_CIPHER_num (OpensslCipherStack); 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.
Yep. Initially I had UINTN, but that caused CI errors due to the type
mismatch ...
> 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).
Good point, missed that.
> but it's probably better to just skip the whole outer loop if
> "OpensslCipherStack" is NULL (as no lookup will succeed).
Agree, will do it that way.
> (4) Why AsciiStrnLenS(), and not AsciiStrLen()?
No good reason. I just looked up the edk2 name of strlen using
'git grep strlen CryptoPkg/Library/Include', and this is the
result I got ;)
> > - Mapping = MappedCipher[Index];
> > + OpensslCipher = MappedCipher[Index];
> > + OpensslCipherName = SSL_CIPHER_get_name (OpensslCipher);
> > + OpensslCipherNameSize = AsciiStrnLenS (OpensslCipherName, MAX_STRING_SIZE);
>
> (5) Better call this "OpensslCipherNameLength" ("Size" suggests
> including the terminating NUL).
OK.
> (8) Can you
>
> - assign <https://bugzilla.tianocore.org/show_bug.cgi?id=2541> to yourself,
>
> - link this patch into a comment on that BZ,
>
> - and reference the BZ URL in the commit message?
Will do.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109311): https://edk2.groups.io/g/devel/message/109311
Mute This Topic: https://groups.io/mt/101657203/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-04 9:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-29 13:23 [edk2-devel] [PATCH 1/1] CryptoPkg/TlsLib: fix tls cipher configuration Gerd Hoffmann
2023-09-30 23:20 ` Laszlo Ersek
2023-10-04 9:19 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox