From nobody Fri Nov 1 03:44:54 2024 Delivered-To: importer@patchew.org Authentication-Results: mx.zohomail.com; spf=none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) smtp.mailfrom=edk2-devel-bounces@lists.01.org Return-Path: Received: from ml01.01.org (ml01.01.org [198.145.21.10]) by mx.zohomail.com with SMTPS id 1522767169911863.3625014849789; Tue, 3 Apr 2018 07:52:49 -0700 (PDT) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id 90D0E226C7C39; Tue, 3 Apr 2018 07:52:21 -0700 (PDT) Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9CEFE226C7C21 for ; Tue, 3 Apr 2018 07:52:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E8D24406E8A4; Tue, 3 Apr 2018 14:52:19 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id 783CC2026E0E; Tue, 3 Apr 2018 14:52:18 +0000 (UTC) X-Original-To: edk2-devel@lists.01.org Received-SPF: none (zoho.com: 198.145.21.10 is neither permitted nor denied by domain of lists.01.org) client-ip=198.145.21.10; envelope-from=edk2-devel-bounces@lists.01.org; helo=ml01.01.org; Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org From: Laszlo Ersek To: edk2-devel-01 Date: Tue, 3 Apr 2018 16:51:49 +0200 Message-Id: <20180403145149.8925-14-lersek@redhat.com> In-Reply-To: <20180403145149.8925-1-lersek@redhat.com> References: <20180403145149.8925-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 03 Apr 2018 14:52:19 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 03 Apr 2018 14:52:19 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: [edk2] [PATCH 13/13] CryptoPkg/TlsLib: rewrite TlsSetCipherList() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Ting Ye , Siyuan Fu , Jiaxin Wu , Qin Long MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Errors-To: edk2-devel-bounces@lists.01.org Sender: "edk2-devel" X-ZohoMail: RSF_4 Z_629925259 SPT_0 Content-Type: text/plain; charset="utf-8" Rewrite the TlsSetCipherList() function in order to fix the following issues: - Any cipher identifier in CipherId that is not recognized by TlsGetCipherMapping() will cause the function to return EFI_UNSUPPORTED. This is a problem because CipherId is an ordered preference list, and a caller should not get EFI_UNSUPPORTED just because it has an elaborate CipherId preference list. Instead, we can filter out cipher identifiers that we don't recognize, as long as we keep the relative order intact. - CipherString is allocated on the stack, with 500 bytes. While processing a large CipherId preference list, this room may not be enough. Although no buffer overflow is possible, CipherString exhaustion can lead to a failed TLS connection, because any cipher names that don't fit on CipherString cannot be negotiated. Compute CipherStringSize first, and allocate CipherString dynamically. - Finally, the "@STRENGTH" pseudo cipher name is appended to CipherString. (Assuming there is enough room left in CipherString.) This causes OpenSSL to sort the cipher list "in order of encryption algorithm key length". This is a bad idea. The caller specifically passes an ordered preference list in CipherId. Therefore TlsSetCipherList() must not ask OpenSSL to reorder the list, for any reason. Drop "@STRENGTH". Cc: Jiaxin Wu Cc: Qin Long Cc: Siyuan Fu Cc: Ting Ye Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D915 Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek --- CryptoPkg/Library/TlsLib/TlsLib.inf | 3 +- CryptoPkg/Include/Library/TlsLib.h | 3 +- CryptoPkg/Library/TlsLib/InternalTlsLib.h | 3 +- CryptoPkg/Library/TlsLib/TlsConfig.c | 163 +++++++++++++++++--- 4 files changed, 149 insertions(+), 23 deletions(-) diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib= /TlsLib.inf index dbb737b2a147..9b44c9cdab3a 100644 --- a/CryptoPkg/Library/TlsLib/TlsLib.inf +++ b/CryptoPkg/Library/TlsLib/TlsLib.inf @@ -37,17 +37,18 @@ [Sources] [Packages] MdePkg/MdePkg.dec CryptoPkg/CryptoPkg.dec =20 [LibraryClasses] BaseCryptLib - BaseLib BaseMemoryLib DebugLib IntrinsicLib + MemoryAllocationLib OpensslLib + SafeIntLib =20 [BuildOptions] # # suppress the following warnings so we do not break the build with warn= ings-as-errors: # C4090: 'function' : different 'const' qualifiers # diff --git a/CryptoPkg/Include/Library/TlsLib.h b/CryptoPkg/Include/Library= /TlsLib.h index 0ffbcb2b7c2a..e71291eaea45 100644 --- a/CryptoPkg/Include/Library/TlsLib.h +++ b/CryptoPkg/Include/Library/TlsLib.h @@ -353,13 +353,14 @@ TlsSetConnectionEnd ( Registry of the IANA, interpreting Byte1 and By= te2 in network (big endian) byte order. @param[in] CipherNum The number of cipher in the list. =20 @retval EFI_SUCCESS The ciphers list was set successfully. @retval EFI_INVALID_PARAMETER The parameter is invalid. - @retval EFI_UNSUPPORTED Unsupported TLS cipher in the list. + @retval EFI_UNSUPPORTED No supported TLS cipher was found in Ciph= erId. + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. =20 **/ EFI_STATUS EFIAPI TlsSetCipherList ( IN VOID *Tls, diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Library/= TlsLib/InternalTlsLib.h index 3f18a461a8d1..b6cf9816aa38 100644 --- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h +++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h @@ -16,15 +16,16 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITH= ER EXPRESS OR IMPLIED. #define __INTERNAL_TLS_LIB_H__ =20 #undef _WIN32 #undef _WIN64 =20 #include -#include #include #include +#include +#include #include #include #include =20 typedef struct { // diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLi= b/TlsConfig.c index ab786fc23849..c7d643fd81f7 100644 --- a/CryptoPkg/Library/TlsLib/TlsConfig.c +++ b/CryptoPkg/Library/TlsLib/TlsConfig.c @@ -409,69 +409,192 @@ TlsSetConnectionEnd ( Registry of the IANA, interpreting Byte1 and By= te2 in network (big endian) byte order. @param[in] CipherNum The number of cipher in the list. =20 @retval EFI_SUCCESS The ciphers list was set successfully. @retval EFI_INVALID_PARAMETER The parameter is invalid. - @retval EFI_UNSUPPORTED Unsupported TLS cipher in the list. + @retval EFI_UNSUPPORTED No supported TLS cipher was found in Ciph= erId. + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. =20 **/ EFI_STATUS EFIAPI TlsSetCipherList ( IN VOID *Tls, IN UINT16 *CipherId, 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; - CONST CHAR8 *MappingName; - CHAR8 CipherString[500]; + CHAR8 *CipherString; + CHAR8 *CipherStringPosition; =20 TlsConn =3D (TLS_CONNECTION *) Tls; if (TlsConn =3D=3D NULL || TlsConn->Ssl =3D=3D NULL || CipherId =3D=3D N= ULL) { return EFI_INVALID_PARAMETER; } =20 - Mapping =3D NULL; - MappingName =3D NULL; - - memset (CipherString, 0, sizeof (CipherString)); + // + // Allocate the MappedCipher array for recording the mappings that we fi= nd + // for the input IANA identifiers in CipherId. + // + Status =3D SafeUintnMult (CipherNum, sizeof (*MappedCipher), + &MappedCipherBytes); + if (EFI_ERROR (Status)) { + return EFI_OUT_OF_RESOURCES; + } + MappedCipher =3D AllocatePool (MappedCipherBytes); + if (MappedCipher =3D=3D NULL) { + return EFI_OUT_OF_RESOURCES; + } =20 + // + // Map the cipher IDs, and count the number of bytes for the full + // CipherString. + // + MappedCipherCount =3D 0; + CipherStringSize =3D 0; for (Index =3D 0; Index < CipherNum; Index++) { // - // Handling OpenSSL / RFC Cipher name mapping. + // Look up the IANA-to-OpenSSL mapping. // - Mapping =3D TlsGetCipherMapping (*(CipherId + Index)); + Mapping =3D TlsGetCipherMapping (CipherId[Index]); if (Mapping =3D=3D NULL) { - return EFI_UNSUPPORTED; - } - MappingName =3D Mapping->OpensslCipher; - - if (Index !=3D 0) { + DEBUG ((DEBUG_VERBOSE, "%a:%a: skipping CipherId=3D0x%04x\n", + gEfiCallerBaseName, __FUNCTION__, CipherId[Index])); // - // The ciphers were separated by a colon. + // Skipping the cipher is valid because CipherId is an ordered + // preference list of ciphers, thus we can filter it as long as we + // don't change the relative order of elements on it. // - AsciiStrCatS (CipherString, sizeof (CipherString), ":"); + continue; + } + // + // Accumulate Mapping->OpensslCipherLength into CipherStringSize. If t= his + // is not the first successful mapping, account for a colon (":") pref= ix + // too. + // + if (MappedCipherCount > 0) { + Status =3D SafeUintnAdd (CipherStringSize, 1, &CipherStringSize); + if (EFI_ERROR (Status)) { + Status =3D EFI_OUT_OF_RESOURCES; + goto FreeMappedCipher; + } + } + Status =3D SafeUintnAdd (CipherStringSize, Mapping->OpensslCipherLengt= h, + &CipherStringSize); + if (EFI_ERROR (Status)) { + Status =3D EFI_OUT_OF_RESOURCES; + goto FreeMappedCipher; } + // + // Record the mapping. + // + MappedCipher[MappedCipherCount++] =3D Mapping; + } =20 - AsciiStrCatS (CipherString, sizeof (CipherString), MappingName); + // + // Verify that at least one IANA cipher ID could be mapped; account for = the + // terminating NUL character in CipherStringSize; allocate CipherString. + // + if (MappedCipherCount =3D=3D 0) { + DEBUG ((DEBUG_ERROR, "%a:%a: no CipherId could be mapped\n", + gEfiCallerBaseName, __FUNCTION__)); + Status =3D EFI_UNSUPPORTED; + goto FreeMappedCipher; + } + Status =3D SafeUintnAdd (CipherStringSize, 1, &CipherStringSize); + if (EFI_ERROR (Status)) { + Status =3D EFI_OUT_OF_RESOURCES; + goto FreeMappedCipher; + } + CipherString =3D AllocatePool (CipherStringSize); + if (CipherString =3D=3D NULL) { + Status =3D EFI_OUT_OF_RESOURCES; + goto FreeMappedCipher; } =20 - AsciiStrCatS (CipherString, sizeof (CipherString), ":@STRENGTH"); + // + // Go over the collected mappings and populate CipherString. + // + CipherStringPosition =3D CipherString; + for (Index =3D 0; Index < MappedCipherCount; Index++) { + Mapping =3D MappedCipher[Index]; + // + // Append the colon (":") prefix except for the first mapping, then ap= pend + // Mapping->OpensslCipher. + // + if (Index > 0) { + *(CipherStringPosition++) =3D ':'; + } + CopyMem (CipherStringPosition, Mapping->OpensslCipher, + Mapping->OpensslCipherLength); + CipherStringPosition +=3D Mapping->OpensslCipherLength; + } + + // + // NUL-terminate CipherString. + // + *(CipherStringPosition++) =3D '\0'; + ASSERT (CipherStringPosition =3D=3D CipherString + CipherStringSize); + + // + // Log CipherString for debugging. CipherString can be very long if the + // caller provided a large CipherId array, so log CipherString in segmen= ts of + // 79 non-newline characters. (MAX_DEBUG_MESSAGE_LENGTH is usually 0x100= in + // DebugLib instances.) + // + DEBUG_CODE ( + UINTN FullLength; + UINTN SegmentLength; + + FullLength =3D CipherStringSize - 1; + DEBUG ((DEBUG_VERBOSE, "%a:%a: CipherString=3D{\n", gEfiCallerBaseName, + __FUNCTION__)); + for (CipherStringPosition =3D CipherString; + CipherStringPosition < CipherString + FullLength; + CipherStringPosition +=3D SegmentLength) { + SegmentLength =3D FullLength - (CipherStringPosition - CipherString); + if (SegmentLength > 79) { + SegmentLength =3D 79; + } + DEBUG ((DEBUG_VERBOSE, "%.*a\n", SegmentLength, CipherStringPosition= )); + } + DEBUG ((DEBUG_VERBOSE, "}\n")); + // + // Restore the pre-debug value of CipherStringPosition by skipping ove= r the + // trailing NUL. + // + CipherStringPosition++; + ASSERT (CipherStringPosition =3D=3D CipherString + CipherStringSize); + ); =20 // // Sets the ciphers for use by the Tls object. // if (SSL_set_cipher_list (TlsConn->Ssl, CipherString) <=3D 0) { - return EFI_UNSUPPORTED; + Status =3D EFI_UNSUPPORTED; + goto FreeCipherString; } =20 - return EFI_SUCCESS; + Status =3D EFI_SUCCESS; + +FreeCipherString: + FreePool (CipherString); + +FreeMappedCipher: + FreePool (MappedCipher); + + return Status; } =20 /** Set the compression method for TLS/SSL operations. =20 This function handles TLS/SSL integrated compression methods. --=20 2.14.1.3.gb7cf6e02401b _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel