[PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: tls_ciphers

Earlier I posted two patch sets for better platform control of the CA
certificates used in HTTPS booting (and for putting that control to use
in OVMF):

  [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates

  [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list
                     for HTTPS boot

These series have been committed; thank you everyone that helped with
review and testing.

My next goal is better platform control of the TLS cipher suites that
are used in HTTPS booting (and similarly, putting that control to use in
OVMF). That's what this series is about.

You'll see references to TianoCore BZ#915 in the commit messages. The BZ
is not public just yet, because I originally thought that I found
security issues. It turns out that's not the case, so the BZ should be
opened up soon. Either way, the commit messages contain enough
information about the code changes.

I'm aware some of my reviewers are currently traveling for business --
please take your time and feel free to review the patches whenever it
best suits you.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>

Thanks!
Laszlo

Laszlo Ersek (13):
  OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS
    boot
  MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
  NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList
  NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList
  CryptoPkg/TlsLib: replace TlsGetCipherString() with
    TlsGetCipherMapping()
  CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()
    function
  CryptoPkg/TlsLib: pre-compute OpensslCipherLength in
    TlsCipherMappingTable
  CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script
  CryptoPkg/TlsLib: extend "TlsCipherMappingTable"
  CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file
  CryptoPkg/TlsLib: sanitize lib classes in internal header and INF
  CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()
  CryptoPkg/TlsLib: rewrite TlsSetCipherList()

 CryptoPkg/Include/Library/TlsLib.h                    |   9 +-
 CryptoPkg/Library/TlsLib/InternalTlsLib.h             |   4 +
 CryptoPkg/Library/TlsLib/TlsConfig.c                  | 448 +++++++++++++++++---
 CryptoPkg/Library/TlsLib/TlsLib.inf                   |  11 +-
 CryptoPkg/Library/TlsLib/TlsMappingTable.sh           | 140 ++++++
 MdePkg/Include/Protocol/Tls.h                         |  10 +
 NetworkPkg/TlsDxe/TlsProtocol.c                       |  17 +-
 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c   |  98 +++++
 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf |   3 +-
 9 files changed, 664 insertions(+), 76 deletions(-)
 create mode 100644 CryptoPkg/Library/TlsLib/TlsMappingTable.sh

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 01/13] OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS boot

[Laszlo Ersek]

Read the list of trusted cipher suites from fw_cfg and to store it to
EFI_TLS_CA_CERTIFICATE_VARIABLE.

The fw_cfg file is formatted by the "update-crypto-policies" utility on
the host side, so that the host settings take effect in guest HTTPS boot
as well. QEMU forwards the file intact to the firmware. The contents are
forwarded by NetworkPkg/HttpDxe (in TlsConfigCipherList()) to
NetworkPkg/TlsDxe (TlsSetSessionData()) and TlsLib (TlsSetCipherList()).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf |  3 +-
 OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c   | 98 ++++++++++++++++++++
 2 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
index 5f83582a8313..40754ea5a2f3 100644
--- a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
+++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf
@@ -46,10 +46,11 @@ [LibraryClasses]
   DebugLib
   MemoryAllocationLib
   QemuFwCfgLib
   UefiRuntimeServicesTableLib
 
 [Guids]
-  gEfiTlsCaCertificateGuid ## PRODUCES ## Variable:L"TlsCaCertificate"
+  gEdkiiHttpTlsCipherListGuid ## PRODUCES ## Variable:L"HttpTlsCipherList"
+  gEfiTlsCaCertificateGuid    ## PRODUCES ## Variable:L"TlsCaCertificate"
 
 [Depex]
   gEfiVariableWriteArchProtocolGuid
diff --git a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
index b5b33bc4fc69..74c393e5462f 100644
--- a/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
+++ b/OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c
@@ -17,12 +17,13 @@
 
 **/
 
 #include <Uefi/UefiBaseType.h>
 #include <Uefi/UefiSpec.h>
 
+#include <Guid/HttpTlsCipherList.h>
 #include <Guid/TlsAuthentication.h>
 
 #include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/QemuFwCfgLib.h>
@@ -118,16 +119,113 @@ SetCaCerts (
     gEfiCallerBaseName, __FUNCTION__, (UINT64)HttpsCaCertsSize));
 
 FreeHttpsCaCerts:
   FreePool (HttpsCaCerts);
 }
 
+/**
+  Read the list of trusted cipher suites from the fw_cfg file
+  "etc/edk2/https/ciphers", and store it to
+  gEdkiiHttpTlsCipherListGuid:EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE.
+
+  The contents are propagated by NetworkPkg/HttpDxe to NetworkPkg/TlsDxe; the
+  list is processed by the latter.
+**/
+STATIC
+VOID
+SetCipherSuites (
+  VOID
+  )
+{
+  EFI_STATUS           Status;
+  FIRMWARE_CONFIG_ITEM HttpsCiphersItem;
+  UINTN                HttpsCiphersSize;
+  VOID                 *HttpsCiphers;
+
+  Status = QemuFwCfgFindFile ("etc/edk2/https/ciphers", &HttpsCiphersItem,
+             &HttpsCiphersSize);
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: not touching cipher suites\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return;
+  }
+  //
+  // From this point on, any failure is fatal. An ordered cipher preference
+  // list is available from QEMU, thus we cannot let the firmware attempt HTTPS
+  // boot with either pre-existent or non-existent preferences. An empty set of
+  // cipher suites does not fail HTTPS boot automatically; the default cipher
+  // suite preferences would take effect, and we must prevent that.
+  //
+  // Delete the current EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE if it exists. If
+  // the variable exists with EFI_VARIABLE_NON_VOLATILE attribute, we cannot
+  // make it volatile without deleting it first.
+  //
+  Status = gRT->SetVariable (
+                  EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE, // VariableName
+                  &gEdkiiHttpTlsCipherListGuid,        // VendorGuid
+                  0,                                   // Attributes
+                  0,                                   // DataSize
+                  NULL                                 // Data
+                  );
+  if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: failed to delete %g:\"%s\"\n",
+      gEfiCallerBaseName, __FUNCTION__, &gEdkiiHttpTlsCipherListGuid,
+      EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE));
+    goto Done;
+  }
+
+  if (HttpsCiphersSize == 0) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: list of cipher suites must not be empty\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    Status = EFI_INVALID_PARAMETER;
+    goto Done;
+  }
+
+  HttpsCiphers = AllocatePool (HttpsCiphersSize);
+  if (HttpsCiphers == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate HttpsCiphers\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    Status = EFI_OUT_OF_RESOURCES;
+    goto Done;
+  }
+
+  QemuFwCfgSelectItem (HttpsCiphersItem);
+  QemuFwCfgReadBytes (HttpsCiphersSize, HttpsCiphers);
+
+  Status = gRT->SetVariable (
+                  EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE, // VariableName
+                  &gEdkiiHttpTlsCipherListGuid,        // VendorGuid
+                  EFI_VARIABLE_BOOTSERVICE_ACCESS,     // Attributes
+                  HttpsCiphersSize,                    // DataSize
+                  HttpsCiphers                         // Data
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: failed to set %g:\"%s\"\n",
+      gEfiCallerBaseName, __FUNCTION__, &gEdkiiHttpTlsCipherListGuid,
+      EDKII_HTTP_TLS_CIPHER_LIST_VARIABLE));
+    goto FreeHttpsCiphers;
+  }
+
+  DEBUG ((DEBUG_VERBOSE, "%a:%a: stored list of cipher suites (%Lu byte(s))\n",
+    gEfiCallerBaseName, __FUNCTION__, (UINT64)HttpsCiphersSize));
+
+FreeHttpsCiphers:
+  FreePool (HttpsCiphers);
+
+Done:
+  if (EFI_ERROR (Status)) {
+    ASSERT_EFI_ERROR (Status);
+    CpuDeadLoop ();
+  }
+}
+
 RETURN_STATUS
 EFIAPI
 TlsAuthConfigInit (
   VOID
   )
 {
   SetCaCerts ();
+  SetCipherSuites ();
 
   return RETURN_SUCCESS;
 }
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC

[Laszlo Ersek]

The structures defined in RFC 5246 are not to have any padding between
fields or at the end; use the "pack" pragma as necessary.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Include/Protocol/Tls.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h
index 2119f33c0f5b..dafaabcd2a8b 100644
--- a/MdePkg/Include/Protocol/Tls.h
+++ b/MdePkg/Include/Protocol/Tls.h
@@ -138,33 +138,37 @@ typedef enum {
 ///
 /// EFI_TLS_CIPHER
 /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, A.4.1.
 ///       Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher
 ///       Suite Registry of IANA.
 ///
+#pragma pack (1)
 typedef struct {
   UINT8                         Data1;
   UINT8                         Data2;
 } EFI_TLS_CIPHER;
+#pragma pack ()
 
 ///
 /// EFI_TLS_COMPRESSION
 /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749".
 ///
 typedef UINT8 EFI_TLS_COMPRESSION;
 
 ///
 /// EFI_TLS_EXTENSION
 /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1.
 ///       Hello Messages".
 ///
+#pragma pack (1)
 typedef struct {
   UINT16                        ExtensionType;
   UINT16                        Length;
   UINT8                         Data[1];
 } EFI_TLS_EXTENSION;
+#pragma pack ()
 
 ///
 /// EFI_TLS_VERIFY
 /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two options
 /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired.
 ///
@@ -191,35 +195,41 @@ typedef UINT32  EFI_TLS_VERIFY;
 
 ///
 /// EFI_TLS_RANDOM
 /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1.
 ///       Hello Messages".
 ///
+#pragma pack (1)
 typedef struct {
   UINT32                        GmtUnixTime;
   UINT8                         RandomBytes[28];
 } EFI_TLS_RANDOM;
+#pragma pack ()
 
 ///
 /// EFI_TLS_MASTER_SECRET
 /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1.
 ///       Computing the Master Secret".
 ///
+#pragma pack (1)
 typedef struct {
   UINT8                         Data[48];
 } EFI_TLS_MASTER_SECRET;
+#pragma pack ()
 
 ///
 /// EFI_TLS_SESSION_ID
 /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. Hello Messages".
 ///
 #define MAX_TLS_SESSION_ID_LENGTH  32
+#pragma pack (1)
 typedef struct {
   UINT16                        Length;
   UINT8                         Data[MAX_TLS_SESSION_ID_LENGTH];
 } EFI_TLS_SESSION_ID;
+#pragma pack ()
 
 ///
 /// EFI_TLS_SESSION_STATE
 ///
 typedef enum {
   ///
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 03/13] NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList

[Laszlo Ersek]

TlsSetSessionData() shouldn't just ignore an incomplete EFI_TLS_CIPHER
element at the end of "Data":

- Generally speaking, malformed input for a security API is best rejected
  explicitly.

- Specifically speaking, the size of EFI_TLS_CIPHER is 2 bytes. If
  DataSize is 1 on input, then the initial check for (DataSize == 0) will
  fail, but then TlsSetCipherList() will be called with CipherNum=0.

Return EFI_INVALID_PARAMETER from TlsSetSessionData() if "Data" doesn't
contain a whole number of EFI_TLS_CIPHER elements. While at it, introduce
the dedicated variable CipherCount.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/TlsDxe/TlsProtocol.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c b/NetworkPkg/TlsDxe/TlsProtocol.c
index ad4c922c60bd..a5f95a098345 100644
--- a/NetworkPkg/TlsDxe/TlsProtocol.c
+++ b/NetworkPkg/TlsDxe/TlsProtocol.c
@@ -35,12 +35,13 @@ EFI_TLS_PROTOCOL  mTlsProtocol = {
 
   @retval EFI_SUCCESS             The TLS session data is set successfully.
   @retval EFI_INVALID_PARAMETER   One or more of the following conditions is TRUE:
                                   This is NULL.
                                   Data is NULL.
                                   DataSize is 0.
+                                  DataSize is invalid for DataType.
   @retval EFI_UNSUPPORTED         The DataType is unsupported.
   @retval EFI_ACCESS_DENIED       If the DataType is one of below:
                                   EfiTlsClientRandom
                                   EfiTlsServerRandom
                                   EfiTlsKeyMaterial
   @retval EFI_NOT_READY           Current TLS session state is NOT
@@ -56,12 +57,13 @@ TlsSetSessionData (
   IN     UINTN                         DataSize
   )
 {
   EFI_STATUS                Status;
   TLS_INSTANCE              *Instance;
   UINT16                    *CipherId;
+  UINTN                     CipherCount;
   UINTN                     Index;
 
   EFI_TPL                   OldTpl;
 
   Status = EFI_SUCCESS;
   CipherId = NULL;
@@ -97,23 +99,29 @@ TlsSetSessionData (
       goto ON_EXIT;
     }
 
     Status = TlsSetConnectionEnd (Instance->TlsConn, *((EFI_TLS_CONNECTION_END *) Data));
     break;
   case EfiTlsCipherList:
+    if (DataSize % sizeof (EFI_TLS_CIPHER) != 0) {
+      Status = EFI_INVALID_PARAMETER;
+      goto ON_EXIT;
+    }
+
     CipherId = AllocatePool (DataSize);
     if (CipherId == NULL) {
       Status = EFI_OUT_OF_RESOURCES;
       goto ON_EXIT;
     }
 
-    for (Index = 0; Index < DataSize / sizeof (EFI_TLS_CIPHER); Index++) {
+    CipherCount = DataSize / sizeof (EFI_TLS_CIPHER);
+    for (Index = 0; Index < CipherCount; Index++) {
       *(CipherId +Index) = HTONS (*(((UINT16 *) Data) + Index));
     }
 
-    Status = TlsSetCipherList (Instance->TlsConn, CipherId, DataSize / sizeof (EFI_TLS_CIPHER));
+    Status = TlsSetCipherList (Instance->TlsConn, CipherId, CipherCount);
 
     FreePool (CipherId);
     break;
   case EfiTlsCompressionMethod:
     //
     // TLS seems only define one CompressionMethod.null, which specifies that data exchanged via the
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 04/13] NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList

[Laszlo Ersek]

Fix the following style issues:

- "Data" is accessed through a pointer to UINT16 rather than to a pointer
  to EFI_TLS_CIPHER. While technically correct, UINT16 is harder to
  interpret against the UEFI spec.

- Array subscripting is written with weird *(Pointer + Offset)
  expressions, rather than with Pointer[Offset].

- The byte order is converted with HTONS(), while it should be NTOHS().
  Either way, use the Data1 and Data2 fields of EFI_TLS_CIPHER instead.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/TlsDxe/TlsProtocol.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c b/NetworkPkg/TlsDxe/TlsProtocol.c
index a5f95a098345..298ffdd659a2 100644
--- a/NetworkPkg/TlsDxe/TlsProtocol.c
+++ b/NetworkPkg/TlsDxe/TlsProtocol.c
@@ -57,12 +57,13 @@ TlsSetSessionData (
   IN     UINTN                         DataSize
   )
 {
   EFI_STATUS                Status;
   TLS_INSTANCE              *Instance;
   UINT16                    *CipherId;
+  CONST EFI_TLS_CIPHER      *TlsCipherList;
   UINTN                     CipherCount;
   UINTN                     Index;
 
   EFI_TPL                   OldTpl;
 
   Status = EFI_SUCCESS;
@@ -110,15 +111,17 @@ TlsSetSessionData (
     CipherId = AllocatePool (DataSize);
     if (CipherId == NULL) {
       Status = EFI_OUT_OF_RESOURCES;
       goto ON_EXIT;
     }
 
+    TlsCipherList = (CONST EFI_TLS_CIPHER *) Data;
     CipherCount = DataSize / sizeof (EFI_TLS_CIPHER);
     for (Index = 0; Index < CipherCount; Index++) {
-      *(CipherId +Index) = HTONS (*(((UINT16 *) Data) + Index));
+      CipherId[Index] = ((TlsCipherList[Index].Data1 << 8) |
+                         TlsCipherList[Index].Data2);
     }
 
     Status = TlsSetCipherList (Instance->TlsConn, CipherId, CipherCount);
 
     FreePool (CipherId);
     break;
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 05/13] CryptoPkg/TlsLib: replace TlsGetCipherString() with TlsGetCipherMapping()

[Laszlo Ersek]

In the following patches it will be useful if the IANA CipherId lookup
returns a pointer to the whole matching IANA-to-OpenSSL mapping structure,
not just the OpenSSL cipher suite name. Rename TLS_CIPHER_PAIR and
TlsGetCipherString() to TLS_CIPHER_MAPPING and TlsGetCipherMapping()
respectively, and make the function return a pointer to
TLS_CIPHER_MAPPING.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 37 +++++++++++---------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 2ffe58ad29a2..507489386b8e 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -21,19 +21,19 @@ typedef struct {
   //
   UINT16                          IanaCipher;
   //
   // OpenSSL-used Cipher Suite String
   //
   CONST CHAR8                     *OpensslCipher;
-} TLS_CIPHER_PAIR;
+} TLS_CIPHER_MAPPING;
 
 //
 // The mapping table between IANA/IETF Cipher Suite definitions and
 // OpenSSL-used Cipher Suite name.
 //
-STATIC CONST TLS_CIPHER_PAIR TlsCipherMappingTable[] = {
+STATIC CONST TLS_CIPHER_MAPPING TlsCipherMappingTable[] = {
   { 0x0001, "NULL-MD5" },                 /// TLS_RSA_WITH_NULL_MD5
   { 0x0002, "NULL-SHA" },                 /// TLS_RSA_WITH_NULL_SHA
   { 0x0004, "RC4-MD5" },                  /// TLS_RSA_WITH_RC4_128_MD5
   { 0x0005, "RC4-SHA" },                  /// TLS_RSA_WITH_RC4_128_SHA
   { 0x000A, "DES-CBC3-SHA" },             /// TLS_RSA_WITH_3DES_EDE_CBC_SHA, mandatory TLS 1.1
   { 0x0016, "DHE-RSA-DES-CBC3-SHA" },     /// TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
@@ -54,42 +54,42 @@ STATIC CONST TLS_CIPHER_PAIR TlsCipherMappingTable[] = {
   { 0x0068, "DH-DSS-AES256-SHA256" },     /// TLS_DH_DSS_WITH_AES_256_CBC_SHA256
   { 0x0069, "DH-RSA-AES256-SHA256" },     /// TLS_DH_RSA_WITH_AES_256_CBC_SHA256
   { 0x006B, "DHE-RSA-AES256-SHA256" }     /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
 };
 
 /**
-  Gets the OpenSSL cipher suite string for the supplied IANA TLS cipher suite.
+  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 string if found,
+  @return  The corresponding OpenSSL cipher suite mapping if found,
            NULL otherwise.
 
 **/
 STATIC
-CONST CHAR8 *
-TlsGetCipherString (
+CONST TLS_CIPHER_MAPPING *
+TlsGetCipherMapping (
   IN     UINT16                   CipherId
   )
 {
-  CONST TLS_CIPHER_PAIR  *CipherEntry;
-  UINTN                  TableSize;
-  UINTN                  Index;
+  CONST TLS_CIPHER_MAPPING  *CipherEntry;
+  UINTN                     TableSize;
+  UINTN                     Index;
 
   CipherEntry = TlsCipherMappingTable;
-  TableSize = sizeof (TlsCipherMappingTable) / sizeof (TLS_CIPHER_PAIR);
+  TableSize = sizeof (TlsCipherMappingTable) / sizeof (TLS_CIPHER_MAPPING);
 
   //
   // Search Cipher Mapping Table for IANA-OpenSSL Cipher Translation
   //
   for (Index = 0; Index < TableSize; Index++, CipherEntry++) {
     //
     // Translate IANA cipher suite name to OpenSSL name.
     //
     if (CipherEntry->IanaCipher == CipherId) {
-      return CipherEntry->OpensslCipher;
+      return CipherEntry;
     }
   }
 
   //
   // No Cipher Mapping found, return NULL.
   //
@@ -226,34 +226,37 @@ EFIAPI
 TlsSetCipherList (
   IN     VOID                     *Tls,
   IN     UINT16                   *CipherId,
   IN     UINTN                    CipherNum
   )
 {
-  TLS_CONNECTION  *TlsConn;
-  UINTN           Index;
-  CONST CHAR8     *MappingName;
-  CHAR8           CipherString[500];
+  TLS_CONNECTION           *TlsConn;
+  UINTN                    Index;
+  CONST TLS_CIPHER_MAPPING *Mapping;
+  CONST CHAR8              *MappingName;
+  CHAR8                    CipherString[500];
 
   TlsConn = (TLS_CONNECTION *) Tls;
   if (TlsConn == NULL || TlsConn->Ssl == NULL || CipherId == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
+  Mapping     = NULL;
   MappingName = NULL;
 
   memset (CipherString, 0, sizeof (CipherString));
 
   for (Index = 0; Index < CipherNum; Index++) {
     //
     // Handling OpenSSL / RFC Cipher name mapping.
     //
-    MappingName = TlsGetCipherString (*(CipherId + Index));
-    if (MappingName == NULL) {
+    Mapping = TlsGetCipherMapping (*(CipherId + Index));
+    if (Mapping == NULL) {
       return EFI_UNSUPPORTED;
     }
+    MappingName = Mapping->OpensslCipher;
 
     if (Index != 0) {
       //
       // The ciphers were separated by a colon.
       //
       AsciiStrCatS (CipherString, sizeof (CipherString), ":");
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 06/13] CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping() function

[Laszlo Ersek]

Improve the performance of the TlsGetCipherMapping() function by adopting
the binary search from DhcpFindOptionFormat()
[MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Option.c].

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 36 +++++++++++++-------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 507489386b8e..c1d91a599482 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -27,12 +27,14 @@ typedef struct {
 } TLS_CIPHER_MAPPING;
 
 //
 // 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[] = {
   { 0x0001, "NULL-MD5" },                 /// TLS_RSA_WITH_NULL_MD5
   { 0x0002, "NULL-SHA" },                 /// TLS_RSA_WITH_NULL_SHA
   { 0x0004, "RC4-MD5" },                  /// TLS_RSA_WITH_RC4_128_MD5
   { 0x0005, "RC4-SHA" },                  /// TLS_RSA_WITH_RC4_128_SHA
   { 0x000A, "DES-CBC3-SHA" },             /// TLS_RSA_WITH_3DES_EDE_CBC_SHA, mandatory TLS 1.1
@@ -68,28 +70,36 @@ STATIC CONST TLS_CIPHER_MAPPING TlsCipherMappingTable[] = {
 STATIC
 CONST TLS_CIPHER_MAPPING *
 TlsGetCipherMapping (
   IN     UINT16                   CipherId
   )
 {
-  CONST TLS_CIPHER_MAPPING  *CipherEntry;
-  UINTN                     TableSize;
-  UINTN                     Index;
-
-  CipherEntry = TlsCipherMappingTable;
-  TableSize = sizeof (TlsCipherMappingTable) / sizeof (TLS_CIPHER_MAPPING);
+  INTN                      Left;
+  INTN                      Right;
+  INTN                      Middle;
 
   //
-  // Search Cipher Mapping Table for IANA-OpenSSL Cipher Translation
+  // Binary Search Cipher Mapping Table for IANA-OpenSSL Cipher Translation
   //
-  for (Index = 0; Index < TableSize; Index++, CipherEntry++) {
-    //
-    // Translate IANA cipher suite name to OpenSSL name.
-    //
-    if (CipherEntry->IanaCipher == CipherId) {
-      return CipherEntry;
+  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.
   //
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 07/13] CryptoPkg/TlsLib: pre-compute OpensslCipherLength in TlsCipherMappingTable

[Laszlo Ersek]

In the next patches, we'll need the lengths of the
TLS_CIPHER_MAPPING.OpensslCipher string fields. These lengths can be
computed at build time; add the new field "OpensslCipherLength", and
introduce the MAP() macro for populating it.

While at it, add some horizontal whitespace to "TlsCipherMappingTable",
and add a comma after the last element. This will come handy in a later
patch.

(The patch does not change the first two columns of
"TlsCipherMappingTable", which can be easily verified with "git show
--word-diff".)

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 58 ++++++++++++--------
 1 file changed, 35 insertions(+), 23 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index c1d91a599482..e2f819b9035f 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -21,44 +21,56 @@ typedef struct {
   //
   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[] = {
-  { 0x0001, "NULL-MD5" },                 /// TLS_RSA_WITH_NULL_MD5
-  { 0x0002, "NULL-SHA" },                 /// TLS_RSA_WITH_NULL_SHA
-  { 0x0004, "RC4-MD5" },                  /// TLS_RSA_WITH_RC4_128_MD5
-  { 0x0005, "RC4-SHA" },                  /// TLS_RSA_WITH_RC4_128_SHA
-  { 0x000A, "DES-CBC3-SHA" },             /// TLS_RSA_WITH_3DES_EDE_CBC_SHA, mandatory TLS 1.1
-  { 0x0016, "DHE-RSA-DES-CBC3-SHA" },     /// TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
-  { 0x002F, "AES128-SHA" },               /// TLS_RSA_WITH_AES_128_CBC_SHA, mandatory TLS 1.2
-  { 0x0030, "DH-DSS-AES128-SHA" },        /// TLS_DH_DSS_WITH_AES_128_CBC_SHA
-  { 0x0031, "DH-RSA-AES128-SHA" },        /// TLS_DH_RSA_WITH_AES_128_CBC_SHA
-  { 0x0033, "DHE-RSA-AES128-SHA" },       /// TLS_DHE_RSA_WITH_AES_128_CBC_SHA
-  { 0x0035, "AES256-SHA" },               /// TLS_RSA_WITH_AES_256_CBC_SHA
-  { 0x0036, "DH-DSS-AES256-SHA" },        /// TLS_DH_DSS_WITH_AES_256_CBC_SHA
-  { 0x0037, "DH-RSA-AES256-SHA" },        /// TLS_DH_RSA_WITH_AES_256_CBC_SHA
-  { 0x0039, "DHE-RSA-AES256-SHA" },       /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA
-  { 0x003B, "NULL-SHA256" },              /// TLS_RSA_WITH_NULL_SHA256
-  { 0x003C, "AES128-SHA256" },            /// TLS_RSA_WITH_AES_128_CBC_SHA256
-  { 0x003D, "AES256-SHA256" },            /// TLS_RSA_WITH_AES_256_CBC_SHA256
-  { 0x003E, "DH-DSS-AES128-SHA256" },     /// TLS_DH_DSS_WITH_AES_128_CBC_SHA256
-  { 0x003F, "DH-RSA-AES128-SHA256" },     /// TLS_DH_RSA_WITH_AES_128_CBC_SHA256
-  { 0x0067, "DHE-RSA-AES128-SHA256" },    /// TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
-  { 0x0068, "DH-DSS-AES256-SHA256" },     /// TLS_DH_DSS_WITH_AES_256_CBC_SHA256
-  { 0x0069, "DH-RSA-AES256-SHA256" },     /// TLS_DH_RSA_WITH_AES_256_CBC_SHA256
-  { 0x006B, "DHE-RSA-AES256-SHA256" }     /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
+  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
 };
 
 /**
   Gets the OpenSSL cipher suite mapping for the supplied IANA TLS cipher suite.
 
   @param[in]  CipherId    The supplied IANA TLS cipher suite ID.
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 08/13] CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script

[Laszlo Ersek]

Add a shell script that will help us keep "TlsCipherMappingTable" in
"TlsConfig.c" up-to-date.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsLib.inf         |   2 +
 CryptoPkg/Library/TlsLib/TlsMappingTable.sh | 140 ++++++++++++++++++++
 2 files changed, 142 insertions(+)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index a3f93e7165cb..dc7f3a5dbd23 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -52,6 +52,8 @@ [BuildOptions]
   #
   # suppress the following warnings so we do not break the build with warnings-as-errors:
   # C4090: 'function' : different 'const' qualifiers
   #
   MSFT:*_*_*_CC_FLAGS = /wd4090
 
+[UserExtensions.TianoCore."ExtraFiles"]
+  TlsMappingTable.sh
diff --git a/CryptoPkg/Library/TlsLib/TlsMappingTable.sh b/CryptoPkg/Library/TlsLib/TlsMappingTable.sh
new file mode 100644
index 000000000000..0cb4a4faa597
--- /dev/null
+++ b/CryptoPkg/Library/TlsLib/TlsMappingTable.sh
@@ -0,0 +1,140 @@
+## @file
+#
+# POSIX shell script to refresh "TlsCipherMappingTable" in "TlsConfig.c".
+#
+# Note: the output of this script is not meant to blindly replace the current
+# contents of "TlsCipherMappingTable". It only helps with the composition and
+# formatting of candidate lines.
+#
+# Copyright (C) 2018, Red Hat, Inc.
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License which accompanies this
+# distribution.  The full text of the license may be found at
+# http://opensource.org/licenses/bsd-license.php
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+##
+
+# Exit on error, treat unset variables as errors, don't overwrite existing
+# files with the ">" output redirection operator.
+set -e -u -C
+
+# This script uses a few utilities that are not defined by POSIX. Check if they
+# are available.
+if ( ! command -v mktemp  ||
+     ! command -v openssl ||
+     ! command -v curl    ||
+     ! command -v column ) >/dev/null
+then
+  BASENAME=$(basename -- "$0")
+  {
+    printf -- '%s: please install the following utilities first:\n' "$BASENAME"
+    printf -- '%s:   mktemp openssl curl column\n' "$BASENAME"
+  } >&2
+  exit 1
+fi
+
+# Create a temporary file for saving the OpenSSL output.
+OPENSSL_LIST=$(mktemp)
+trap 'rm -f -- "$OPENSSL_LIST"' EXIT
+
+# Create a temporary file for saving the IANA TLS Cipher Suite Registry.
+IANA_LIST=$(mktemp)
+trap 'rm -f -- "$OPENSSL_LIST" "$IANA_LIST"' EXIT
+
+# Sorting, and range expressions in regular expressions, depend on the locale.
+# Use a well-defined locale.
+LC_ALL=C
+export LC_ALL
+
+# Get OPENSSL_LIST.
+{
+  # List cipher suite codes and names from OpenSSL.
+  openssl ciphers -V ALL
+
+  # This produces a line format like:
+  # 0xC0,0x30 - ECDHE-RSA-AES256-GCM-SHA384 TLSv1.2 Kx=ECDH Au=RSA Enc=AESGCM(256) Mac=AEAD
+  # (sequences of space characters squeezed for brevity).
+} |
+{
+  # Project the list to UINT16 hex codes (network byte order interpretation)
+  # and OpenSSL cipher suite names.
+  sed -r -n -e 's/^ *0x([0-9A-F]{2}),0x([0-9A-F]{2}) - ([^ ]+) .*$/\1\2 \3/p'
+
+  # This produces a line format like:
+  # C030 ECDHE-RSA-AES256-GCM-SHA384
+} |
+{
+  # Sort the output so we can later join it on the UINT16 hex code as key.
+  sort
+} >>"$OPENSSL_LIST"
+
+# Get IANA_LIST.
+{
+  # Download the CSV file from the IANA website.
+  curl -s https://www.iana.org/assignments/tls-parameters/tls-parameters-4.csv
+
+  # This produces a line format like:
+  # Value,Description,DTLS-OK,Reference
+  # "0x00,0x00",TLS_NULL_WITH_NULL_NULL,Y,[RFC5246]
+} |
+{
+  # Project the list to UINT16 hex codes (network byte order interpretation)
+  # and Descriptions (TLS_xxx macros).
+  sed -r -n \
+    -e 's/^"0x([0-9A-F]{2}),0x([0-9A-F]{2})",([A-Za-z0-9_]+).*$/\1\2 \3/p'
+
+  # This produces a line format like:
+  # 0000 TLS_NULL_WITH_NULL_NULL
+} |
+{
+  # Sort the output so we can later join it on the UINT16 hex code as key.
+  sort
+} >>"$IANA_LIST"
+
+# Produce the C source code on stdout.
+{
+  # Join the two lists first. Elements that are in exactly one input file are
+  # dropped.
+  join -- "$OPENSSL_LIST" "$IANA_LIST"
+
+  # This produces a line format like:
+  # 0004 RC4-MD5 TLS_RSA_WITH_RC4_128_MD5
+  # And the output remains sorted by UINT16 hex key.
+} |
+{
+  # Produce a valid C language line. Be careful that only one space character
+  # is preserved, for the next step.
+  sed -r -n -e 's!^([0-9A-F]{4}) ([^ ]+) ([^ ]+)$!{0x\1,"\2"}, ///\3!p'
+
+  # This produces a line format like:
+  # {0x0004,"RC4-MD5"}, ///TLS_RSA_WITH_RC4_128_MD5
+} |
+{
+  # Align the rightmost column nicely (the TLS_xxx macros). The "column"
+  # command will expand the space character as necessary.
+  column -t
+
+  # This produces a line format like:
+  # {0x0004,"RC4-MD5"},                        ///TLS_RSA_WITH_RC4_128_MD5
+} |
+{
+  # Final touches:
+  # - replace the opening brace "{" with "  MAP ( ",
+  # - insert one space character after the first comma ","
+  # - replace the closing brace "}" with " )",
+  # - remove one space character after the second comma "," (because the
+  #   "column" utility pads space characters to at least two),
+  # - insert one space character after the comment marker "///"
+  sed \
+    -e 's/^{/  MAP ( /' \
+    -e 's/,/, /' \
+    -e 's/}, / ),/' \
+    -e 's!///!/// !'
+
+  # This produces a line format like:
+  #   MAP ( 0x0004, "RC4-MD5" ),                       /// TLS_RSA_WITH_RC4_128_MD5
+}
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 09/13] CryptoPkg/TlsLib: extend "TlsCipherMappingTable"

[Laszlo Ersek]

Add mapping entries printed by "TlsMappingTable.sh" to
"TlsCipherMappingTable". This allows HTTPS / TLS clients to get a good
match for their EFI_TLS_CIPHER lists.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsConfig.c | 169 ++++++++++++++++++++
 1 file changed, 169 insertions(+)

diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index e2f819b9035f..9d21e6c1466d 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -46,31 +46,200 @@ typedef struct {
 //
 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 ( 0x0007, "IDEA-CBC-SHA" ),                   /// TLS_RSA_WITH_IDEA_CBC_SHA
   MAP ( 0x000A, "DES-CBC3-SHA" ),                   /// TLS_RSA_WITH_3DES_EDE_CBC_SHA, mandatory TLS 1.1
+  MAP ( 0x000D, "DH-DSS-DES-CBC3-SHA" ),            /// TLS_DH_DSS_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0010, "DH-RSA-DES-CBC3-SHA" ),            /// TLS_DH_RSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0013, "DHE-DSS-DES-CBC3-SHA" ),           /// TLS_DHE_DSS_WITH_3DES_EDE_CBC_SHA
   MAP ( 0x0016, "DHE-RSA-DES-CBC3-SHA" ),           /// TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0018, "ADH-RC4-MD5" ),                    /// TLS_DH_anon_WITH_RC4_128_MD5
+  MAP ( 0x001B, "ADH-DES-CBC3-SHA" ),               /// TLS_DH_anon_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x001F, "KRB5-DES-CBC3-SHA" ),              /// TLS_KRB5_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0020, "KRB5-RC4-SHA" ),                   /// TLS_KRB5_WITH_RC4_128_SHA
+  MAP ( 0x0021, "KRB5-IDEA-CBC-SHA" ),              /// TLS_KRB5_WITH_IDEA_CBC_SHA
+  MAP ( 0x0023, "KRB5-DES-CBC3-MD5" ),              /// TLS_KRB5_WITH_3DES_EDE_CBC_MD5
+  MAP ( 0x0024, "KRB5-RC4-MD5" ),                   /// TLS_KRB5_WITH_RC4_128_MD5
+  MAP ( 0x0025, "KRB5-IDEA-CBC-MD5" ),              /// TLS_KRB5_WITH_IDEA_CBC_MD5
   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 ( 0x0032, "DHE-DSS-AES128-SHA" ),             /// TLS_DHE_DSS_WITH_AES_128_CBC_SHA
   MAP ( 0x0033, "DHE-RSA-AES128-SHA" ),             /// TLS_DHE_RSA_WITH_AES_128_CBC_SHA
+  MAP ( 0x0034, "ADH-AES128-SHA" ),                 /// TLS_DH_anon_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 ( 0x0038, "DHE-DSS-AES256-SHA" ),             /// TLS_DHE_DSS_WITH_AES_256_CBC_SHA
   MAP ( 0x0039, "DHE-RSA-AES256-SHA" ),             /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA
+  MAP ( 0x003A, "ADH-AES256-SHA" ),                 /// TLS_DH_anon_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 ( 0x0040, "DHE-DSS-AES128-SHA256" ),          /// TLS_DHE_DSS_WITH_AES_128_CBC_SHA256
+  MAP ( 0x0041, "CAMELLIA128-SHA" ),                /// TLS_RSA_WITH_CAMELLIA_128_CBC_SHA
+  MAP ( 0x0042, "DH-DSS-CAMELLIA128-SHA" ),         /// TLS_DH_DSS_WITH_CAMELLIA_128_CBC_SHA
+  MAP ( 0x0043, "DH-RSA-CAMELLIA128-SHA" ),         /// TLS_DH_RSA_WITH_CAMELLIA_128_CBC_SHA
+  MAP ( 0x0044, "DHE-DSS-CAMELLIA128-SHA" ),        /// TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA
+  MAP ( 0x0045, "DHE-RSA-CAMELLIA128-SHA" ),        /// TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA
+  MAP ( 0x0046, "ADH-CAMELLIA128-SHA" ),            /// TLS_DH_anon_WITH_CAMELLIA_128_CBC_SHA
   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 ( 0x006A, "DHE-DSS-AES256-SHA256" ),          /// TLS_DHE_DSS_WITH_AES_256_CBC_SHA256
   MAP ( 0x006B, "DHE-RSA-AES256-SHA256" ),          /// TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
+  MAP ( 0x006C, "ADH-AES128-SHA256" ),              /// TLS_DH_anon_WITH_AES_128_CBC_SHA256
+  MAP ( 0x006D, "ADH-AES256-SHA256" ),              /// TLS_DH_anon_WITH_AES_256_CBC_SHA256
+  MAP ( 0x0084, "CAMELLIA256-SHA" ),                /// TLS_RSA_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x0085, "DH-DSS-CAMELLIA256-SHA" ),         /// TLS_DH_DSS_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x0086, "DH-RSA-CAMELLIA256-SHA" ),         /// TLS_DH_RSA_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x0087, "DHE-DSS-CAMELLIA256-SHA" ),        /// TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x0088, "DHE-RSA-CAMELLIA256-SHA" ),        /// TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x0089, "ADH-CAMELLIA256-SHA" ),            /// TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA
+  MAP ( 0x008A, "PSK-RC4-SHA" ),                    /// TLS_PSK_WITH_RC4_128_SHA
+  MAP ( 0x008B, "PSK-3DES-EDE-CBC-SHA" ),           /// TLS_PSK_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x008C, "PSK-AES128-CBC-SHA" ),             /// TLS_PSK_WITH_AES_128_CBC_SHA
+  MAP ( 0x008D, "PSK-AES256-CBC-SHA" ),             /// TLS_PSK_WITH_AES_256_CBC_SHA
+  MAP ( 0x008F, "DHE-PSK-3DES-EDE-CBC-SHA" ),       /// TLS_DHE_PSK_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0090, "DHE-PSK-AES128-CBC-SHA" ),         /// TLS_DHE_PSK_WITH_AES_128_CBC_SHA
+  MAP ( 0x0091, "DHE-PSK-AES256-CBC-SHA" ),         /// TLS_DHE_PSK_WITH_AES_256_CBC_SHA
+  MAP ( 0x0093, "RSA-PSK-3DES-EDE-CBC-SHA" ),       /// TLS_RSA_PSK_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0x0094, "RSA-PSK-AES128-CBC-SHA" ),         /// TLS_RSA_PSK_WITH_AES_128_CBC_SHA
+  MAP ( 0x0095, "RSA-PSK-AES256-CBC-SHA" ),         /// TLS_RSA_PSK_WITH_AES_256_CBC_SHA
+  MAP ( 0x0096, "SEED-SHA" ),                       /// TLS_RSA_WITH_SEED_CBC_SHA
+  MAP ( 0x0097, "DH-DSS-SEED-SHA" ),                /// TLS_DH_DSS_WITH_SEED_CBC_SHA
+  MAP ( 0x0098, "DH-RSA-SEED-SHA" ),                /// TLS_DH_RSA_WITH_SEED_CBC_SHA
+  MAP ( 0x0099, "DHE-DSS-SEED-SHA" ),               /// TLS_DHE_DSS_WITH_SEED_CBC_SHA
+  MAP ( 0x009A, "DHE-RSA-SEED-SHA" ),               /// TLS_DHE_RSA_WITH_SEED_CBC_SHA
+  MAP ( 0x009B, "ADH-SEED-SHA" ),                   /// TLS_DH_anon_WITH_SEED_CBC_SHA
+  MAP ( 0x009C, "AES128-GCM-SHA256" ),              /// TLS_RSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0x009D, "AES256-GCM-SHA384" ),              /// TLS_RSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0x009E, "DHE-RSA-AES128-GCM-SHA256" ),      /// TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0x009F, "DHE-RSA-AES256-GCM-SHA384" ),      /// TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00A0, "DH-RSA-AES128-GCM-SHA256" ),       /// TLS_DH_RSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00A1, "DH-RSA-AES256-GCM-SHA384" ),       /// TLS_DH_RSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00A2, "DHE-DSS-AES128-GCM-SHA256" ),      /// TLS_DHE_DSS_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00A3, "DHE-DSS-AES256-GCM-SHA384" ),      /// TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00A4, "DH-DSS-AES128-GCM-SHA256" ),       /// TLS_DH_DSS_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00A5, "DH-DSS-AES256-GCM-SHA384" ),       /// TLS_DH_DSS_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00A6, "ADH-AES128-GCM-SHA256" ),          /// TLS_DH_anon_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00A7, "ADH-AES256-GCM-SHA384" ),          /// TLS_DH_anon_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00A8, "PSK-AES128-GCM-SHA256" ),          /// TLS_PSK_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00A9, "PSK-AES256-GCM-SHA384" ),          /// TLS_PSK_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00AA, "DHE-PSK-AES128-GCM-SHA256" ),      /// TLS_DHE_PSK_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00AB, "DHE-PSK-AES256-GCM-SHA384" ),      /// TLS_DHE_PSK_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00AC, "RSA-PSK-AES128-GCM-SHA256" ),      /// TLS_RSA_PSK_WITH_AES_128_GCM_SHA256
+  MAP ( 0x00AD, "RSA-PSK-AES256-GCM-SHA384" ),      /// TLS_RSA_PSK_WITH_AES_256_GCM_SHA384
+  MAP ( 0x00AE, "PSK-AES128-CBC-SHA256" ),          /// TLS_PSK_WITH_AES_128_CBC_SHA256
+  MAP ( 0x00AF, "PSK-AES256-CBC-SHA384" ),          /// TLS_PSK_WITH_AES_256_CBC_SHA384
+  MAP ( 0x00B2, "DHE-PSK-AES128-CBC-SHA256" ),      /// TLS_DHE_PSK_WITH_AES_128_CBC_SHA256
+  MAP ( 0x00B3, "DHE-PSK-AES256-CBC-SHA384" ),      /// TLS_DHE_PSK_WITH_AES_256_CBC_SHA384
+  MAP ( 0x00B6, "RSA-PSK-AES128-CBC-SHA256" ),      /// TLS_RSA_PSK_WITH_AES_128_CBC_SHA256
+  MAP ( 0x00B7, "RSA-PSK-AES256-CBC-SHA384" ),      /// TLS_RSA_PSK_WITH_AES_256_CBC_SHA384
+  MAP ( 0x00BA, "CAMELLIA128-SHA256" ),             /// TLS_RSA_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0x00BD, "DHE-DSS-CAMELLIA128-SHA256" ),     /// TLS_DHE_DSS_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0x00BE, "DHE-RSA-CAMELLIA128-SHA256" ),     /// TLS_DHE_RSA_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0x00BF, "ADH-CAMELLIA128-SHA256" ),         /// TLS_DH_anon_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0x00C0, "CAMELLIA256-SHA256" ),             /// TLS_RSA_WITH_CAMELLIA_256_CBC_SHA256
+  MAP ( 0x00C3, "DHE-DSS-CAMELLIA256-SHA256" ),     /// TLS_DHE_DSS_WITH_CAMELLIA_256_CBC_SHA256
+  MAP ( 0x00C4, "DHE-RSA-CAMELLIA256-SHA256" ),     /// TLS_DHE_RSA_WITH_CAMELLIA_256_CBC_SHA256
+  MAP ( 0x00C5, "ADH-CAMELLIA256-SHA256" ),         /// TLS_DH_anon_WITH_CAMELLIA_256_CBC_SHA256
+  MAP ( 0xC002, "ECDH-ECDSA-RC4-SHA" ),             /// TLS_ECDH_ECDSA_WITH_RC4_128_SHA
+  MAP ( 0xC003, "ECDH-ECDSA-DES-CBC3-SHA" ),        /// TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC004, "ECDH-ECDSA-AES128-SHA" ),          /// TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC005, "ECDH-ECDSA-AES256-SHA" ),          /// TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC007, "ECDHE-ECDSA-RC4-SHA" ),            /// TLS_ECDHE_ECDSA_WITH_RC4_128_SHA
+  MAP ( 0xC008, "ECDHE-ECDSA-DES-CBC3-SHA" ),       /// TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC009, "ECDHE-ECDSA-AES128-SHA" ),         /// TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC00A, "ECDHE-ECDSA-AES256-SHA" ),         /// TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC00C, "ECDH-RSA-RC4-SHA" ),               /// TLS_ECDH_RSA_WITH_RC4_128_SHA
+  MAP ( 0xC00D, "ECDH-RSA-DES-CBC3-SHA" ),          /// TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC00E, "ECDH-RSA-AES128-SHA" ),            /// TLS_ECDH_RSA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC00F, "ECDH-RSA-AES256-SHA" ),            /// TLS_ECDH_RSA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC011, "ECDHE-RSA-RC4-SHA" ),              /// TLS_ECDHE_RSA_WITH_RC4_128_SHA
+  MAP ( 0xC012, "ECDHE-RSA-DES-CBC3-SHA" ),         /// TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC013, "ECDHE-RSA-AES128-SHA" ),           /// TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC014, "ECDHE-RSA-AES256-SHA" ),           /// TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC016, "AECDH-RC4-SHA" ),                  /// TLS_ECDH_anon_WITH_RC4_128_SHA
+  MAP ( 0xC017, "AECDH-DES-CBC3-SHA" ),             /// TLS_ECDH_anon_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC018, "AECDH-AES128-SHA" ),               /// TLS_ECDH_anon_WITH_AES_128_CBC_SHA
+  MAP ( 0xC019, "AECDH-AES256-SHA" ),               /// TLS_ECDH_anon_WITH_AES_256_CBC_SHA
+  MAP ( 0xC01A, "SRP-3DES-EDE-CBC-SHA" ),           /// TLS_SRP_SHA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC01B, "SRP-RSA-3DES-EDE-CBC-SHA" ),       /// TLS_SRP_SHA_RSA_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC01C, "SRP-DSS-3DES-EDE-CBC-SHA" ),       /// TLS_SRP_SHA_DSS_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC01D, "SRP-AES-128-CBC-SHA" ),            /// TLS_SRP_SHA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC01E, "SRP-RSA-AES-128-CBC-SHA" ),        /// TLS_SRP_SHA_RSA_WITH_AES_128_CBC_SHA
+  MAP ( 0xC01F, "SRP-DSS-AES-128-CBC-SHA" ),        /// TLS_SRP_SHA_DSS_WITH_AES_128_CBC_SHA
+  MAP ( 0xC020, "SRP-AES-256-CBC-SHA" ),            /// TLS_SRP_SHA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC021, "SRP-RSA-AES-256-CBC-SHA" ),        /// TLS_SRP_SHA_RSA_WITH_AES_256_CBC_SHA
+  MAP ( 0xC022, "SRP-DSS-AES-256-CBC-SHA" ),        /// TLS_SRP_SHA_DSS_WITH_AES_256_CBC_SHA
+  MAP ( 0xC023, "ECDHE-ECDSA-AES128-SHA256" ),      /// TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
+  MAP ( 0xC024, "ECDHE-ECDSA-AES256-SHA384" ),      /// TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
+  MAP ( 0xC025, "ECDH-ECDSA-AES128-SHA256" ),       /// TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256
+  MAP ( 0xC026, "ECDH-ECDSA-AES256-SHA384" ),       /// TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384
+  MAP ( 0xC027, "ECDHE-RSA-AES128-SHA256" ),        /// TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
+  MAP ( 0xC028, "ECDHE-RSA-AES256-SHA384" ),        /// TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
+  MAP ( 0xC029, "ECDH-RSA-AES128-SHA256" ),         /// TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256
+  MAP ( 0xC02A, "ECDH-RSA-AES256-SHA384" ),         /// TLS_ECDH_RSA_WITH_AES_256_CBC_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 ( 0xC02D, "ECDH-ECDSA-AES128-GCM-SHA256" ),   /// TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0xC02E, "ECDH-ECDSA-AES256-GCM-SHA384" ),   /// TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0xC02F, "ECDHE-RSA-AES128-GCM-SHA256" ),    /// TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0xC030, "ECDHE-RSA-AES256-GCM-SHA384" ),    /// TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0xC031, "ECDH-RSA-AES128-GCM-SHA256" ),     /// TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256
+  MAP ( 0xC032, "ECDH-RSA-AES256-GCM-SHA384" ),     /// TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384
+  MAP ( 0xC034, "ECDHE-PSK-3DES-EDE-CBC-SHA" ),     /// TLS_ECDHE_PSK_WITH_3DES_EDE_CBC_SHA
+  MAP ( 0xC035, "ECDHE-PSK-AES128-CBC-SHA" ),       /// TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA
+  MAP ( 0xC036, "ECDHE-PSK-AES256-CBC-SHA" ),       /// TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA
+  MAP ( 0xC037, "ECDHE-PSK-AES128-CBC-SHA256" ),    /// TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA256
+  MAP ( 0xC038, "ECDHE-PSK-AES256-CBC-SHA384" ),    /// TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA384
+  MAP ( 0xC072, "ECDHE-ECDSA-CAMELLIA128-SHA256" ), /// TLS_ECDHE_ECDSA_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC073, "ECDHE-ECDSA-CAMELLIA256-SHA384" ), /// TLS_ECDHE_ECDSA_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC076, "ECDHE-RSA-CAMELLIA128-SHA256" ),   /// TLS_ECDHE_RSA_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC077, "ECDHE-RSA-CAMELLIA256-SHA384" ),   /// TLS_ECDHE_RSA_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC094, "PSK-CAMELLIA128-SHA256" ),         /// TLS_PSK_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC095, "PSK-CAMELLIA256-SHA384" ),         /// TLS_PSK_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC096, "DHE-PSK-CAMELLIA128-SHA256" ),     /// TLS_DHE_PSK_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC097, "DHE-PSK-CAMELLIA256-SHA384" ),     /// TLS_DHE_PSK_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC098, "RSA-PSK-CAMELLIA128-SHA256" ),     /// TLS_RSA_PSK_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC099, "RSA-PSK-CAMELLIA256-SHA384" ),     /// TLS_RSA_PSK_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC09A, "ECDHE-PSK-CAMELLIA128-SHA256" ),   /// TLS_ECDHE_PSK_WITH_CAMELLIA_128_CBC_SHA256
+  MAP ( 0xC09B, "ECDHE-PSK-CAMELLIA256-SHA384" ),   /// TLS_ECDHE_PSK_WITH_CAMELLIA_256_CBC_SHA384
+  MAP ( 0xC09C, "AES128-CCM" ),                     /// TLS_RSA_WITH_AES_128_CCM
+  MAP ( 0xC09D, "AES256-CCM" ),                     /// TLS_RSA_WITH_AES_256_CCM
+  MAP ( 0xC09E, "DHE-RSA-AES128-CCM" ),             /// TLS_DHE_RSA_WITH_AES_128_CCM
+  MAP ( 0xC09F, "DHE-RSA-AES256-CCM" ),             /// TLS_DHE_RSA_WITH_AES_256_CCM
+  MAP ( 0xC0A0, "AES128-CCM8" ),                    /// TLS_RSA_WITH_AES_128_CCM_8
+  MAP ( 0xC0A1, "AES256-CCM8" ),                    /// TLS_RSA_WITH_AES_256_CCM_8
+  MAP ( 0xC0A2, "DHE-RSA-AES128-CCM8" ),            /// TLS_DHE_RSA_WITH_AES_128_CCM_8
+  MAP ( 0xC0A3, "DHE-RSA-AES256-CCM8" ),            /// TLS_DHE_RSA_WITH_AES_256_CCM_8
+  MAP ( 0xC0A4, "PSK-AES128-CCM" ),                 /// TLS_PSK_WITH_AES_128_CCM
+  MAP ( 0xC0A5, "PSK-AES256-CCM" ),                 /// TLS_PSK_WITH_AES_256_CCM
+  MAP ( 0xC0A6, "DHE-PSK-AES128-CCM" ),             /// TLS_DHE_PSK_WITH_AES_128_CCM
+  MAP ( 0xC0A7, "DHE-PSK-AES256-CCM" ),             /// TLS_DHE_PSK_WITH_AES_256_CCM
+  MAP ( 0xC0A8, "PSK-AES128-CCM8" ),                /// TLS_PSK_WITH_AES_128_CCM_8
+  MAP ( 0xC0A9, "PSK-AES256-CCM8" ),                /// TLS_PSK_WITH_AES_256_CCM_8
+  MAP ( 0xC0AA, "DHE-PSK-AES128-CCM8" ),            /// TLS_PSK_DHE_WITH_AES_128_CCM_8
+  MAP ( 0xC0AB, "DHE-PSK-AES256-CCM8" ),            /// TLS_PSK_DHE_WITH_AES_256_CCM_8
+  MAP ( 0xC0AC, "ECDHE-ECDSA-AES128-CCM" ),         /// TLS_ECDHE_ECDSA_WITH_AES_128_CCM
+  MAP ( 0xC0AD, "ECDHE-ECDSA-AES256-CCM" ),         /// TLS_ECDHE_ECDSA_WITH_AES_256_CCM
+  MAP ( 0xC0AE, "ECDHE-ECDSA-AES128-CCM8" ),        /// TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8
+  MAP ( 0xC0AF, "ECDHE-ECDSA-AES256-CCM8" ),        /// TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8
+  MAP ( 0xCCA8, "ECDHE-RSA-CHACHA20-POLY1305" ),    /// TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCA9, "ECDHE-ECDSA-CHACHA20-POLY1305" ),  /// TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCAA, "DHE-RSA-CHACHA20-POLY1305" ),      /// TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCAB, "PSK-CHACHA20-POLY1305" ),          /// TLS_PSK_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCAC, "ECDHE-PSK-CHACHA20-POLY1305" ),    /// TLS_ECDHE_PSK_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCAD, "DHE-PSK-CHACHA20-POLY1305" ),      /// TLS_DHE_PSK_WITH_CHACHA20_POLY1305_SHA256
+  MAP ( 0xCCAE, "RSA-PSK-CHACHA20-POLY1305" ),      /// TLS_RSA_PSK_WITH_CHACHA20_POLY1305_SHA256
 };
 
 /**
   Gets the OpenSSL cipher suite mapping for the supplied IANA TLS cipher suite.
 
   @param[in]  CipherId    The supplied IANA TLS cipher suite ID.
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 10/13] CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file

[Laszlo Ersek]

In the next patches, we'll update the library classes in
"InternalTlsLib.h" and "TlsLib.inf". It is easier to verify correctness if
those sections are sorted.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsLib.inf | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index dc7f3a5dbd23..44789ceeefa3 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -38,18 +38,18 @@ [Packages]
   MdePkg/MdePkg.dec
   CryptoPkg/CryptoPkg.dec
 
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
-  MemoryAllocationLib
-  UefiRuntimeServicesTableLib
   DebugLib
-  OpensslLib
   IntrinsicLib
+  MemoryAllocationLib
+  OpensslLib
   PrintLib
+  UefiRuntimeServicesTableLib
 
 [BuildOptions]
   #
   # suppress the following warnings so we do not break the build with warnings-as-errors:
   # C4090: 'function' : different 'const' qualifiers
   #
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 11/13] CryptoPkg/TlsLib: sanitize lib classes in internal header and INF

[Laszlo Ersek]

"InternalTlsLib.h" includes "BaseCryptLib.h", but the lib class is not
listed in the INF file.

The INF file lists a good number of lib classes, but none of the lib class
headers are included by "InternalTlsLib.h".

Synchronize both lists, while removing those library classes that aren't
actually needed. (IntrinsicLib and OpensslLib have no edk2 class headers.)

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Library/TlsLib/TlsLib.inf       | 4 +---
 CryptoPkg/Library/TlsLib/InternalTlsLib.h | 3 +++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 44789ceeefa3..dbb737b2a147 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -36,20 +36,18 @@ [Sources]
 
 [Packages]
   MdePkg/MdePkg.dec
   CryptoPkg/CryptoPkg.dec
 
 [LibraryClasses]
+  BaseCryptLib
   BaseLib
   BaseMemoryLib
   DebugLib
   IntrinsicLib
-  MemoryAllocationLib
   OpensslLib
-  PrintLib
-  UefiRuntimeServicesTableLib
 
 [BuildOptions]
   #
   # suppress the following warnings so we do not break the build with warnings-as-errors:
   # C4090: 'function' : different 'const' qualifiers
   #
diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
index 88c4e3b38e4e..3f18a461a8d1 100644
--- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
+++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
@@ -16,12 +16,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define __INTERNAL_TLS_LIB_H__
 
 #undef _WIN32
 #undef _WIN64
 
 #include <Library/BaseCryptLib.h>
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
 #include <openssl/ssl.h>
 #include <openssl/bio.h>
 #include <openssl/err.h>
 
 typedef struct {
   //
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 12/13] CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()

[Laszlo Ersek]

The TlsSetCipherList() function documents its CipherId parameter
incorrectly. Document the parameter precisely and use the same
documentation in both the lib class header and the lib instance.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Include/Library/TlsLib.h   | 6 ++++--
 CryptoPkg/Library/TlsLib/TlsConfig.c | 5 ++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/CryptoPkg/Include/Library/TlsLib.h b/CryptoPkg/Include/Library/TlsLib.h
index e19a38a214ce..0ffbcb2b7c2a 100644
--- a/CryptoPkg/Include/Library/TlsLib.h
+++ b/CryptoPkg/Include/Library/TlsLib.h
@@ -345,14 +345,16 @@ TlsSetConnectionEnd (
 /**
   Set the ciphers list to be used by the TLS object.
 
   This function sets the ciphers for use by a specified TLS object.
 
   @param[in]  Tls          Pointer to a TLS object.
-  @param[in]  CipherId     Pointer to a string that contains one or more
-                           ciphers separated by a colon.
+  @param[in]  CipherId     Array of UINT16 cipher identifiers. Each UINT16
+                           cipher identifier comes from the TLS Cipher Suite
+                           Registry of the IANA, interpreting Byte1 and Byte2
+                           in network (big endian) byte order.
   @param[in]  CipherNum    The number of cipher in the list.
 
   @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.
 
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 9d21e6c1466d..ab786fc23849 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -401,13 +401,16 @@ TlsSetConnectionEnd (
 /**
   Set the ciphers list to be used by the TLS object.
 
   This function sets the ciphers for use by a specified TLS object.
 
   @param[in]  Tls          Pointer to a TLS object.
-  @param[in]  CipherId     Pointer to a UINT16 cipher Id.
+  @param[in]  CipherId     Array of UINT16 cipher identifiers. Each UINT16
+                           cipher identifier comes from the TLS Cipher Suite
+                           Registry of the IANA, interpreting Byte1 and Byte2
+                           in network (big endian) byte order.
   @param[in]  CipherNum    The number of cipher in the list.
 
   @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.
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 13/13] CryptoPkg/TlsLib: rewrite TlsSetCipherList()

[Laszlo Ersek]

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 <jiaxin.wu@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 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
 
 [LibraryClasses]
   BaseCryptLib
-  BaseLib
   BaseMemoryLib
   DebugLib
   IntrinsicLib
+  MemoryAllocationLib
   OpensslLib
+  SafeIntLib
 
 [BuildOptions]
   #
   # suppress the following warnings so we do not break the build with warnings-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 Byte2
                            in network (big endian) byte order.
   @param[in]  CipherNum    The number of cipher in the list.
 
   @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 CipherId.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
 
 **/
 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, EITHER EXPRESS OR IMPLIED.
 #define __INTERNAL_TLS_LIB_H__
 
 #undef _WIN32
 #undef _WIN64
 
 #include <Library/BaseCryptLib.h>
-#include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/SafeIntLib.h>
 #include <openssl/ssl.h>
 #include <openssl/bio.h>
 #include <openssl/err.h>
 
 typedef struct {
   //
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/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 Byte2
                            in network (big endian) byte order.
   @param[in]  CipherNum    The number of cipher in the list.
 
   @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 CipherId.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
 
 **/
 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;
 
   TlsConn = (TLS_CONNECTION *) Tls;
   if (TlsConn == NULL || TlsConn->Ssl == NULL || CipherId == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  Mapping     = NULL;
-  MappingName = NULL;
-
-  memset (CipherString, 0, sizeof (CipherString));
+  //
+  // Allocate the MappedCipher array for recording the mappings that we find
+  // for the input IANA identifiers in CipherId.
+  //
+  Status = SafeUintnMult (CipherNum, sizeof (*MappedCipher),
+             &MappedCipherBytes);
+  if (EFI_ERROR (Status)) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+  MappedCipher = AllocatePool (MappedCipherBytes);
+  if (MappedCipher == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
 
+  //
+  // Map the cipher IDs, and count the number of bytes for the full
+  // CipherString.
+  //
+  MappedCipherCount = 0;
+  CipherStringSize = 0;
   for (Index = 0; Index < CipherNum; Index++) {
     //
-    // Handling OpenSSL / RFC Cipher name mapping.
+    // Look up the IANA-to-OpenSSL mapping.
     //
-    Mapping = TlsGetCipherMapping (*(CipherId + Index));
+    Mapping = TlsGetCipherMapping (CipherId[Index]);
     if (Mapping == NULL) {
-      return EFI_UNSUPPORTED;
-    }
-    MappingName = Mapping->OpensslCipher;
-
-    if (Index != 0) {
+      DEBUG ((DEBUG_VERBOSE, "%a:%a: skipping CipherId=0x%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 this
+    // is not the first successful mapping, account for a colon (":") prefix
+    // too.
+    //
+    if (MappedCipherCount > 0) {
+      Status = SafeUintnAdd (CipherStringSize, 1, &CipherStringSize);
+      if (EFI_ERROR (Status)) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto FreeMappedCipher;
+      }
+    }
+    Status = SafeUintnAdd (CipherStringSize, Mapping->OpensslCipherLength,
+               &CipherStringSize);
+    if (EFI_ERROR (Status)) {
+      Status = EFI_OUT_OF_RESOURCES;
+      goto FreeMappedCipher;
     }
+    //
+    // Record the mapping.
+    //
+    MappedCipher[MappedCipherCount++] = Mapping;
+  }
 
-    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 == 0) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: no CipherId could be mapped\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    Status = EFI_UNSUPPORTED;
+    goto FreeMappedCipher;
+  }
+  Status = SafeUintnAdd (CipherStringSize, 1, &CipherStringSize);
+  if (EFI_ERROR (Status)) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeMappedCipher;
+  }
+  CipherString = AllocatePool (CipherStringSize);
+  if (CipherString == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeMappedCipher;
   }
 
-  AsciiStrCatS (CipherString, sizeof (CipherString), ":@STRENGTH");
+  //
+  // Go over the collected mappings and populate CipherString.
+  //
+  CipherStringPosition = CipherString;
+  for (Index = 0; Index < MappedCipherCount; Index++) {
+    Mapping = MappedCipher[Index];
+    //
+    // Append the colon (":") prefix except for the first mapping, then append
+    // Mapping->OpensslCipher.
+    //
+    if (Index > 0) {
+      *(CipherStringPosition++) = ':';
+    }
+    CopyMem (CipherStringPosition, Mapping->OpensslCipher,
+      Mapping->OpensslCipherLength);
+    CipherStringPosition += Mapping->OpensslCipherLength;
+  }
+
+  //
+  // NUL-terminate CipherString.
+  //
+  *(CipherStringPosition++) = '\0';
+  ASSERT (CipherStringPosition == CipherString + CipherStringSize);
+
+  //
+  // Log CipherString for debugging. CipherString can be very long if the
+  // caller provided a large CipherId array, so log CipherString in segments of
+  // 79 non-newline characters. (MAX_DEBUG_MESSAGE_LENGTH is usually 0x100 in
+  // DebugLib instances.)
+  //
+  DEBUG_CODE (
+    UINTN FullLength;
+    UINTN SegmentLength;
+
+    FullLength = CipherStringSize - 1;
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: CipherString={\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    for (CipherStringPosition = CipherString;
+         CipherStringPosition < CipherString + FullLength;
+         CipherStringPosition += SegmentLength) {
+      SegmentLength = FullLength - (CipherStringPosition - CipherString);
+      if (SegmentLength > 79) {
+        SegmentLength = 79;
+      }
+      DEBUG ((DEBUG_VERBOSE, "%.*a\n", SegmentLength, CipherStringPosition));
+    }
+    DEBUG ((DEBUG_VERBOSE, "}\n"));
+    //
+    // Restore the pre-debug value of CipherStringPosition by skipping over the
+    // trailing NUL.
+    //
+    CipherStringPosition++;
+    ASSERT (CipherStringPosition == CipherString + CipherStringSize);
+  );
 
   //
   // Sets the ciphers for use by the Tls object.
   //
   if (SSL_set_cipher_list (TlsConn->Ssl, CipherString) <= 0) {
-    return EFI_UNSUPPORTED;
+    Status = EFI_UNSUPPORTED;
+    goto FreeCipherString;
   }
 
-  return EFI_SUCCESS;
+  Status = EFI_SUCCESS;
+
+FreeCipherString:
+  FreePool (CipherString);
+
+FreeMappedCipher:
+  FreePool (MappedCipher);
+
+  return Status;
 }
 
 /**
   Set the compression method for TLS/SSL operations.
 
   This function handles TLS/SSL integrated compression methods.
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC

[Gao, Liming]

Laszlo:
  Could you use one pack to scope all structure definitions?

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fu,
> Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
> 
> The structures defined in RFC 5246 are not to have any padding between
> fields or at the end; use the "pack" pragma as necessary.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdePkg/Include/Protocol/Tls.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h
> index 2119f33c0f5b..dafaabcd2a8b 100644
> --- a/MdePkg/Include/Protocol/Tls.h
> +++ b/MdePkg/Include/Protocol/Tls.h
> @@ -138,33 +138,37 @@ typedef enum {
>  ///
>  /// EFI_TLS_CIPHER
>  /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, A.4.1.
>  ///       Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher
>  ///       Suite Registry of IANA.
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT8                         Data1;
>    UINT8                         Data2;
>  } EFI_TLS_CIPHER;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_COMPRESSION
>  /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749".
>  ///
>  typedef UINT8 EFI_TLS_COMPRESSION;
> 
>  ///
>  /// EFI_TLS_EXTENSION
>  /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1.
>  ///       Hello Messages".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT16                        ExtensionType;
>    UINT16                        Length;
>    UINT8                         Data[1];
>  } EFI_TLS_EXTENSION;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_VERIFY
>  /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two options
>  /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired.
>  ///
> @@ -191,35 +195,41 @@ typedef UINT32  EFI_TLS_VERIFY;
> 
>  ///
>  /// EFI_TLS_RANDOM
>  /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1.
>  ///       Hello Messages".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT32                        GmtUnixTime;
>    UINT8                         RandomBytes[28];
>  } EFI_TLS_RANDOM;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_MASTER_SECRET
>  /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1.
>  ///       Computing the Master Secret".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT8                         Data[48];
>  } EFI_TLS_MASTER_SECRET;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_SESSION_ID
>  /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. Hello Messages".
>  ///
>  #define MAX_TLS_SESSION_ID_LENGTH  32
> +#pragma pack (1)
>  typedef struct {
>    UINT16                        Length;
>    UINT8                         Data[MAX_TLS_SESSION_ID_LENGTH];
>  } EFI_TLS_SESSION_ID;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_SESSION_STATE
>  ///
>  typedef enum {
>    ///
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC

[Laszlo Ersek]

On 04/03/18 17:08, Gao, Liming wrote:
> Laszlo:
>   Could you use one pack to scope all structure definitions?
I didn't do that originally because the affected structure definitions
are spread out over a larger part of the code, intermixed with enums,
#defines, and other structures, *and* it is not clear from the source
code comments whether *all* of the structure definitions come from RFC
5246. Wherever the code comments did *not* indicate that a given
structure came from the RFC, I didn't add packing either.

Example structures for this:
- EFI_TLS_VERSION
- EFI_TLS_FRAGMENT_DATA

Looking at these in more detail, I don't think either of them should be
packed:

- EfiTlsVersion / EFI_TLS_VERSION are used in TlsSetSessionData()
[NetworkPkg/TlsDxe/TlsProtocol.c], and the constituent fields Major and
Minor are passed individually to TlsSetVersion()
[CryptoPkg/Library/TlsLib/TlsConfig.c]. So packing the structure is not
necessary.

- EFI_TLS_FRAGMENT_DATA contains a (VOID*) pointer, so it cannot be
serialized to the wire by definition. And, it is used in
TlsProcessPacket() [NetworkPkg/TlsDxe/TlsProtocol.c], and
TlsEncryptPacket() / TlsDecryptPacket() [NetworkPkg/TlsDxe/TlsImpl.c],
without any need for packing.

Given that this protocol header file defines both packed and non-packed
structures, without a very clear separation between them, I feel it is
cleaner to use the pack #pragma for individual structures.

If you disagree, then I think we should implement that "clear
separation" in the header file first. Namely, move all the packed
structures either after or before all the non-packed structures, and add
a very visible comment that "all of the following structures come from
RFC 5246 and need packing". Then we can use a single pack #pragma to
cover them all.

Thanks,
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, April 3, 2018 10:52 PM
>> To: edk2-devel-01 <edk2-devel@lists.01.org>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fu,
>> Siyuan <siyuan.fu@intel.com>
>> Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
>>
>> The structures defined in RFC 5246 are not to have any padding between
>> fields or at the end; use the "pack" pragma as necessary.
>>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  MdePkg/Include/Protocol/Tls.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h
>> index 2119f33c0f5b..dafaabcd2a8b 100644
>> --- a/MdePkg/Include/Protocol/Tls.h
>> +++ b/MdePkg/Include/Protocol/Tls.h
>> @@ -138,33 +138,37 @@ typedef enum {
>>  ///
>>  /// EFI_TLS_CIPHER
>>  /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246, A.4.1.
>>  ///       Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher
>>  ///       Suite Registry of IANA.
>>  ///
>> +#pragma pack (1)
>>  typedef struct {
>>    UINT8                         Data1;
>>    UINT8                         Data2;
>>  } EFI_TLS_CIPHER;
>> +#pragma pack ()
>>
>>  ///
>>  /// EFI_TLS_COMPRESSION
>>  /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749".
>>  ///
>>  typedef UINT8 EFI_TLS_COMPRESSION;
>>
>>  ///
>>  /// EFI_TLS_EXTENSION
>>  /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1.
>>  ///       Hello Messages".
>>  ///
>> +#pragma pack (1)
>>  typedef struct {
>>    UINT16                        ExtensionType;
>>    UINT16                        Length;
>>    UINT8                         Data[1];
>>  } EFI_TLS_EXTENSION;
>> +#pragma pack ()
>>
>>  ///
>>  /// EFI_TLS_VERIFY
>>  /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two options
>>  /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired.
>>  ///
>> @@ -191,35 +195,41 @@ typedef UINT32  EFI_TLS_VERIFY;
>>
>>  ///
>>  /// EFI_TLS_RANDOM
>>  /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1.
>>  ///       Hello Messages".
>>  ///
>> +#pragma pack (1)
>>  typedef struct {
>>    UINT32                        GmtUnixTime;
>>    UINT8                         RandomBytes[28];
>>  } EFI_TLS_RANDOM;
>> +#pragma pack ()
>>
>>  ///
>>  /// EFI_TLS_MASTER_SECRET
>>  /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1.
>>  ///       Computing the Master Secret".
>>  ///
>> +#pragma pack (1)
>>  typedef struct {
>>    UINT8                         Data[48];
>>  } EFI_TLS_MASTER_SECRET;
>> +#pragma pack ()
>>
>>  ///
>>  /// EFI_TLS_SESSION_ID
>>  /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1. Hello Messages".
>>  ///
>>  #define MAX_TLS_SESSION_ID_LENGTH  32
>> +#pragma pack (1)
>>  typedef struct {
>>    UINT16                        Length;
>>    UINT8                         Data[MAX_TLS_SESSION_ID_LENGTH];
>>  } EFI_TLS_SESSION_ID;
>> +#pragma pack ()
>>
>>  ///
>>  /// EFI_TLS_SESSION_STATE
>>  ///
>>  typedef enum {
>>    ///
>> --
>> 2.14.1.3.gb7cf6e02401b
>>
> 

Re: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC

[Fu, Siyuan]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Gao, Liming <liming.gao@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: [PATCH 02/13] MdePkg/Include/Protocol/Tls.h: pack structures from
> the TLS RFC
> 
> The structures defined in RFC 5246 are not to have any padding between
> fields or at the end; use the "pack" pragma as necessary.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  MdePkg/Include/Protocol/Tls.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h
> index 2119f33c0f5b..dafaabcd2a8b 100644
> --- a/MdePkg/Include/Protocol/Tls.h
> +++ b/MdePkg/Include/Protocol/Tls.h
> @@ -138,33 +138,37 @@ typedef enum {
>  ///
>  /// EFI_TLS_CIPHER
>  /// Note: The definition of EFI_TLS_CIPHER definition is from "RFC 5246,
> A.4.1.
>  ///       Hello Messages". The value of EFI_TLS_CIPHER is from TLS Cipher
>  ///       Suite Registry of IANA.
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT8                         Data1;
>    UINT8                         Data2;
>  } EFI_TLS_CIPHER;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_COMPRESSION
>  /// Note: The value of EFI_TLS_COMPRESSION definition is from "RFC 3749".
>  ///
>  typedef UINT8 EFI_TLS_COMPRESSION;
> 
>  ///
>  /// EFI_TLS_EXTENSION
>  /// Note: The definition of EFI_TLS_EXTENSION if from "RFC 5246 A.4.1.
>  ///       Hello Messages".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT16                        ExtensionType;
>    UINT16                        Length;
>    UINT8                         Data[1];
>  } EFI_TLS_EXTENSION;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_VERIFY
>  /// Use either EFI_TLS_VERIFY_NONE or EFI_TLS_VERIFY_PEER, the last two
> options
>  /// are 'ORed' with EFI_TLS_VERIFY_PEER if they are desired.
>  ///
> @@ -191,35 +195,41 @@ typedef UINT32  EFI_TLS_VERIFY;
> 
>  ///
>  /// EFI_TLS_RANDOM
>  /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1.
>  ///       Hello Messages".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT32                        GmtUnixTime;
>    UINT8                         RandomBytes[28];
>  } EFI_TLS_RANDOM;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_MASTER_SECRET
>  /// Note: The definition of EFI_TLS_MASTER_SECRET is from "RFC 5246 8.1.
>  ///       Computing the Master Secret".
>  ///
> +#pragma pack (1)
>  typedef struct {
>    UINT8                         Data[48];
>  } EFI_TLS_MASTER_SECRET;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_SESSION_ID
>  /// Note: The definition of EFI_TLS_SESSION_ID is from "RFC 5246 A.4.1.
> Hello Messages".
>  ///
>  #define MAX_TLS_SESSION_ID_LENGTH  32
> +#pragma pack (1)
>  typedef struct {
>    UINT16                        Length;
>    UINT8                         Data[MAX_TLS_SESSION_ID_LENGTH];
>  } EFI_TLS_SESSION_ID;
> +#pragma pack ()
> 
>  ///
>  /// EFI_TLS_SESSION_STATE
>  ///
>  typedef enum {
>    ///
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 03/13] NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList

[Fu, Siyuan]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>



> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 03/13] NetworkPkg/TlsDxe: verify DataSize for
> EfiTlsCipherList
> 
> TlsSetSessionData() shouldn't just ignore an incomplete EFI_TLS_CIPHER
> element at the end of "Data":
> 
> - Generally speaking, malformed input for a security API is best rejected
>   explicitly.
> 
> - Specifically speaking, the size of EFI_TLS_CIPHER is 2 bytes. If
>   DataSize is 1 on input, then the initial check for (DataSize == 0) will
>   fail, but then TlsSetCipherList() will be called with CipherNum=0.
> 
> Return EFI_INVALID_PARAMETER from TlsSetSessionData() if "Data" doesn't
> contain a whole number of EFI_TLS_CIPHER elements. While at it, introduce
> the dedicated variable CipherCount.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  NetworkPkg/TlsDxe/TlsProtocol.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c
> b/NetworkPkg/TlsDxe/TlsProtocol.c
> index ad4c922c60bd..a5f95a098345 100644
> --- a/NetworkPkg/TlsDxe/TlsProtocol.c
> +++ b/NetworkPkg/TlsDxe/TlsProtocol.c
> @@ -35,12 +35,13 @@ EFI_TLS_PROTOCOL  mTlsProtocol = {
> 
>    @retval EFI_SUCCESS             The TLS session data is set
> successfully.
>    @retval EFI_INVALID_PARAMETER   One or more of the following conditions
> is TRUE:
>                                    This is NULL.
>                                    Data is NULL.
>                                    DataSize is 0.
> +                                  DataSize is invalid for DataType.
>    @retval EFI_UNSUPPORTED         The DataType is unsupported.
>    @retval EFI_ACCESS_DENIED       If the DataType is one of below:
>                                    EfiTlsClientRandom
>                                    EfiTlsServerRandom
>                                    EfiTlsKeyMaterial
>    @retval EFI_NOT_READY           Current TLS session state is NOT
> @@ -56,12 +57,13 @@ TlsSetSessionData (
>    IN     UINTN                         DataSize
>    )
>  {
>    EFI_STATUS                Status;
>    TLS_INSTANCE              *Instance;
>    UINT16                    *CipherId;
> +  UINTN                     CipherCount;
>    UINTN                     Index;
> 
>    EFI_TPL                   OldTpl;
> 
>    Status = EFI_SUCCESS;
>    CipherId = NULL;
> @@ -97,23 +99,29 @@ TlsSetSessionData (
>        goto ON_EXIT;
>      }
> 
>      Status = TlsSetConnectionEnd (Instance->TlsConn,
> *((EFI_TLS_CONNECTION_END *) Data));
>      break;
>    case EfiTlsCipherList:
> +    if (DataSize % sizeof (EFI_TLS_CIPHER) != 0) {
> +      Status = EFI_INVALID_PARAMETER;
> +      goto ON_EXIT;
> +    }
> +
>      CipherId = AllocatePool (DataSize);
>      if (CipherId == NULL) {
>        Status = EFI_OUT_OF_RESOURCES;
>        goto ON_EXIT;
>      }
> 
> -    for (Index = 0; Index < DataSize / sizeof (EFI_TLS_CIPHER); Index++)
> {
> +    CipherCount = DataSize / sizeof (EFI_TLS_CIPHER);
> +    for (Index = 0; Index < CipherCount; Index++) {
>        *(CipherId +Index) = HTONS (*(((UINT16 *) Data) + Index));
>      }
> 
> -    Status = TlsSetCipherList (Instance->TlsConn, CipherId, DataSize /
> sizeof (EFI_TLS_CIPHER));
> +    Status = TlsSetCipherList (Instance->TlsConn, CipherId, CipherCount);
> 
>      FreePool (CipherId);
>      break;
>    case EfiTlsCompressionMethod:
>      //
>      // TLS seems only define one CompressionMethod.null, which specifies
> that data exchanged via the
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 04/13] NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList

[Fu, Siyuan]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 04/13] NetworkPkg/TlsDxe: clean up byte order conversion
> for EfiTlsCipherList
> 
> Fix the following style issues:
> 
> - "Data" is accessed through a pointer to UINT16 rather than to a pointer
>   to EFI_TLS_CIPHER. While technically correct, UINT16 is harder to
>   interpret against the UEFI spec.
> 
> - Array subscripting is written with weird *(Pointer + Offset)
>   expressions, rather than with Pointer[Offset].
> 
> - The byte order is converted with HTONS(), while it should be NTOHS().
>   Either way, use the Data1 and Data2 fields of EFI_TLS_CIPHER instead.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  NetworkPkg/TlsDxe/TlsProtocol.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c
> b/NetworkPkg/TlsDxe/TlsProtocol.c
> index a5f95a098345..298ffdd659a2 100644
> --- a/NetworkPkg/TlsDxe/TlsProtocol.c
> +++ b/NetworkPkg/TlsDxe/TlsProtocol.c
> @@ -57,12 +57,13 @@ TlsSetSessionData (
>    IN     UINTN                         DataSize
>    )
>  {
>    EFI_STATUS                Status;
>    TLS_INSTANCE              *Instance;
>    UINT16                    *CipherId;
> +  CONST EFI_TLS_CIPHER      *TlsCipherList;
>    UINTN                     CipherCount;
>    UINTN                     Index;
> 
>    EFI_TPL                   OldTpl;
> 
>    Status = EFI_SUCCESS;
> @@ -110,15 +111,17 @@ TlsSetSessionData (
>      CipherId = AllocatePool (DataSize);
>      if (CipherId == NULL) {
>        Status = EFI_OUT_OF_RESOURCES;
>        goto ON_EXIT;
>      }
> 
> +    TlsCipherList = (CONST EFI_TLS_CIPHER *) Data;
>      CipherCount = DataSize / sizeof (EFI_TLS_CIPHER);
>      for (Index = 0; Index < CipherCount; Index++) {
> -      *(CipherId +Index) = HTONS (*(((UINT16 *) Data) + Index));
> +      CipherId[Index] = ((TlsCipherList[Index].Data1 << 8) |
> +                         TlsCipherList[Index].Data2);
>      }
> 
>      Status = TlsSetCipherList (Instance->TlsConn, CipherId, CipherCount);
> 
>      FreePool (CipherId);
>      break;
> --
> 2.14.1.3.gb7cf6e02401b
> 

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Wu, Jiaxin]

Hi Laszlo 

Appreciate your contribution. I have reviewed the series patches you attached here. First, I assume you have verified the patches on OVMF and the functionality works well, then, below are my comments:  

1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable. After talked with Qin, I know the unsupported cipher suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, the cipher suite list that passed from the client to the server in the ClientHello message might also include such unsupported cipher suites. In such a case, the failure will happen once the server select the unsupported cipher suite. From the handshake process view, it's unreasonable since the client sent the desired cipher suites, then the server selected one of them but still met the error. Anyway, filtrating the unsupported cipher suites as early as possible is a wise choice. So, TlsCipherMappingTable should only include the supported cipher suites by reference the security requirement of CryptoPkg. 

3. For patch 0006, it's good to me to optimize the searching algorithm since the table is larger than before.

4. Can we combined some patches together to make the things simple? e.g. Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to fix the issues in 0013.

5. For patch 0008, I think it's unnecessary to provide such script. I prefer to maintain the TlsCipherMappingTable more statical since it's the internal mapping table. How about we keep it as internal assistant tool?

6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us to provide the supported cipher suites by reference the security requirement of CryptoPkg.

Thanks,
Jiaxin




> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gary Ching-Pang Lin
> <glin@suse.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Long, Qin <qin.long@intel.com>;
> Fu, Siyuan <siyuan.fu@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: [PATCH 00/13] {Ovmf,Mde,Network,Crypto}Pkg: fixes+features for
> setting HTTPS cipher suites
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: tls_ciphers
> 
> Earlier I posted two patch sets for better platform control of the CA
> certificates used in HTTPS booting (and for putting that control to use
> in OVMF):
> 
>   [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates
> 
>   [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list
>                      for HTTPS boot
> 
> These series have been committed; thank you everyone that helped with
> review and testing.
> 
> My next goal is better platform control of the TLS cipher suites that
> are used in HTTPS booting (and similarly, putting that control to use in
> OVMF). That's what this series is about.
> 
> You'll see references to TianoCore BZ#915 in the commit messages. The BZ
> is not public just yet, because I originally thought that I found
> security issues. It turns out that's not the case, so the BZ should be
> opened up soon. Either way, the commit messages contain enough
> information about the code changes.
> 
> I'm aware some of my reviewers are currently traveling for business --
> please take your time and feel free to review the patches whenever it
> best suits you.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Qin Long <qin.long@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> 
> Thanks!
> Laszlo
> 
> Laszlo Ersek (13):
>   OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS
>     boot
>   MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
>   NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList
>   NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList
>   CryptoPkg/TlsLib: replace TlsGetCipherString() with
>     TlsGetCipherMapping()
>   CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()
>     function
>   CryptoPkg/TlsLib: pre-compute OpensslCipherLength in
>     TlsCipherMappingTable
>   CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script
>   CryptoPkg/TlsLib: extend "TlsCipherMappingTable"
>   CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file
>   CryptoPkg/TlsLib: sanitize lib classes in internal header and INF
>   CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()
>   CryptoPkg/TlsLib: rewrite TlsSetCipherList()
> 
>  CryptoPkg/Include/Library/TlsLib.h                    |   9 +-
>  CryptoPkg/Library/TlsLib/InternalTlsLib.h             |   4 +
>  CryptoPkg/Library/TlsLib/TlsConfig.c                  | 448 +++++++++++++++++---
>  CryptoPkg/Library/TlsLib/TlsLib.inf                   |  11 +-
>  CryptoPkg/Library/TlsLib/TlsMappingTable.sh           | 140 ++++++
>  MdePkg/Include/Protocol/Tls.h                         |  10 +
>  NetworkPkg/TlsDxe/TlsProtocol.c                       |  17 +-
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c   |  98 +++++
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf |   3 +-
>  9 files changed, 664 insertions(+), 76 deletions(-)
>  create mode 100644 CryptoPkg/Library/TlsLib/TlsMappingTable.sh
> 
> --
> 2.14.1.3.gb7cf6e02401b

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Long, Qin]

Hi, Laszlo,

Some comments / discussions were added in https://bugzilla.tianocore.org/show_bug.cgi?id=915
with comment 09 & 11.
Back to the patch review. Some comments were appended:

#0001, #0003, #0004，#0010，#0011:
        Looks good to me.
#0002 - I personally think in general we should reduce using "#pragma pack", except that these
        data really have serialization requirement (e.g. variable) to match extra data layout.
        Here we just use these structures for setting / getting data, instead of direct data
        transport. I am thinking if it's better to update the implementation part.
        But too many sizeof() were used, and Ovmf part may also need to store preferred
        CipherList data. So it's still good to me to pack something.
#0008, #0009:
      - As the BZ comments. The TlsCipherMappingTable extension and generation with script looks
        incorrect. This table should include all available / supported ciphers, which was actually
        platform / configuration dependent.
        I prefer to maintain one static / limited table for edk2 tls implementation. Any new cipher
        requirement can be evaluated & enabled, and then added into this table.
#0005, #0006, #0007, #0012， #0013:
        These implementation looks good to me.
        But some of updates were based on the assumption of #0008-0009. I have no strong opinion
        if some original light implementation are good enough currently.


Best Regards & Thanks,
LONG, Qin

-----Original Message-----
From: Wu, Jiaxin 
Sent: Tuesday, April 10, 2018 12:09 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gary Ching-Pang Lin <glin@suse.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Long, Qin <qin.long@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye, Ting <ting.ye@intel.com>
Subject: RE: [PATCH 00/13] {Ovmf,Mde,Network,Crypto}Pkg: fixes+features for setting HTTPS cipher suites

Hi Laszlo 

Appreciate your contribution. I have reviewed the series patches you attached here. First, I assume you have verified the patches on OVMF and the functionality works well, then, below are my comments:  

1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable. After talked with Qin, I know the unsupported cipher suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, the cipher suite list that passed from the client to the server in the ClientHello message might also include such unsupported cipher suites. In such a case, the failure will happen once the server select the unsupported cipher suite. From the handshake process view, it's unreasonable since the client sent the desired cipher suites, then the server selected one of them but still met the error. Anyway, filtrating the unsupported cipher suites as early as possible is a wise choice. So, TlsCipherMappingTable should only include the supported cipher suites by reference the security requirement of CryptoPkg. 

3. For patch 0006, it's good to me to optimize the searching algorithm since the table is larger than before.

4. Can we combined some patches together to make the things simple? e.g. Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to fix the issues in 0013.

5. For patch 0008, I think it's unnecessary to provide such script. I prefer to maintain the TlsCipherMappingTable more statical since it's the internal mapping table. How about we keep it as internal assistant tool?

6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us to provide the supported cipher suites by reference the security requirement of CryptoPkg.

Thanks,
Jiaxin




> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gary Ching-Pang Lin 
> <glin@suse.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Justen, Jordan L 
> <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; 
> Kinney, Michael D <michael.d.kinney@intel.com>; Long, Qin 
> <qin.long@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Ye, Ting 
> <ting.ye@intel.com>
> Subject: [PATCH 00/13] {Ovmf,Mde,Network,Crypto}Pkg: fixes+features 
> for setting HTTPS cipher suites
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: tls_ciphers
> 
> Earlier I posted two patch sets for better platform control of the CA 
> certificates used in HTTPS booting (and for putting that control to 
> use in OVMF):
> 
>   [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates
> 
>   [edk2] [PATCH 0/4] MdeModulePkg, OvmfPkg: support large CA cert list
>                      for HTTPS boot
> 
> These series have been committed; thank you everyone that helped with 
> review and testing.
> 
> My next goal is better platform control of the TLS cipher suites that 
> are used in HTTPS booting (and similarly, putting that control to use 
> in OVMF). That's what this series is about.
> 
> You'll see references to TianoCore BZ#915 in the commit messages. The 
> BZ is not public just yet, because I originally thought that I found 
> security issues. It turns out that's not the case, so the BZ should be 
> opened up soon. Either way, the commit messages contain enough 
> information about the code changes.
> 
> I'm aware some of my reviewers are currently traveling for business -- 
> please take your time and feel free to review the patches whenever it 
> best suits you.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Qin Long <qin.long@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> 
> Thanks!
> Laszlo
> 
> Laszlo Ersek (13):
>   OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS
>     boot
>   MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC
>   NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList
>   NetworkPkg/TlsDxe: clean up byte order conversion for EfiTlsCipherList
>   CryptoPkg/TlsLib: replace TlsGetCipherString() with
>     TlsGetCipherMapping()
>   CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()
>     function
>   CryptoPkg/TlsLib: pre-compute OpensslCipherLength in
>     TlsCipherMappingTable
>   CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script
>   CryptoPkg/TlsLib: extend "TlsCipherMappingTable"
>   CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file
>   CryptoPkg/TlsLib: sanitize lib classes in internal header and INF
>   CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()
>   CryptoPkg/TlsLib: rewrite TlsSetCipherList()
> 
>  CryptoPkg/Include/Library/TlsLib.h                    |   9 +-
>  CryptoPkg/Library/TlsLib/InternalTlsLib.h             |   4 +
>  CryptoPkg/Library/TlsLib/TlsConfig.c                  | 448 +++++++++++++++++---
>  CryptoPkg/Library/TlsLib/TlsLib.inf                   |  11 +-
>  CryptoPkg/Library/TlsLib/TlsMappingTable.sh           | 140 ++++++
>  MdePkg/Include/Protocol/Tls.h                         |  10 +
>  NetworkPkg/TlsDxe/TlsProtocol.c                       |  17 +-
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.c   |  98 +++++
>  OvmfPkg/Library/TlsAuthConfigLib/TlsAuthConfigLib.inf |   3 +-
>  9 files changed, 664 insertions(+), 76 deletions(-)  create mode 
> 100644 CryptoPkg/Library/TlsLib/TlsMappingTable.sh
> 
> --
> 2.14.1.3.gb7cf6e02401b

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek]

On 04/10/18 06:09, Wu, Jiaxin wrote:
> Hi Laszlo 
> 
> Appreciate your contribution. I have reviewed the series patches you attached here. First, I assume you have verified the patches on OVMF and the functionality works well,

That's right; I tested cipher suite negotiation failures and successes.

For example, I configured apache to "Disable All SSL and TLS Protocols
Except TLS 1 and Up"
<https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers#s2-apache-mod_ssl-enabling>,
and then I verified that HTTPS boot would succeed vs. fail dependent on
whether I passed strong ciphers too, or only weak ones, to OVMF.

> then, below are my comments:  
> 
> 1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

Thanks. For patch #2, "MdePkg/Include/Protocol/Tls.h: pack structures
from the TLS RFC", we exchanged some points with Liming earlier:

http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E1F1B10@SHSMSX104.ccr.corp.intel.com

(Please see also my response to that.)

I see that both you and Siyuan are OK with patch #2, i.e. with the
separate #pragma directives. I'd also like Liming to confirm that he
accepts the patch as-is.

> 
> 2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable.

OK.

I think this means that I should preserve patches #5 through #7, and
drop patches #8 ('CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX
shell script') and #9 ('CryptoPkg/TlsLib: extend "TlsCipherMappingTable"').

Is that correct?

> After talked with Qin, I know the unsupported cipher suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, the cipher suite list that passed from the client to the server in the ClientHello message might also include such unsupported cipher suites. In such a case, the failure will happen once the server select the unsupported cipher suite. From the handshake process view, it's unreasonable since the client sent the desired cipher suites, then the server selected one of them but still met the error.

Oh! You are totally right. I apologize for missing this -- I didn't
realize this from Qin's comments on TianoCore #915.

In other words, it is actually *important* that "TlsCipherMappingTable"
match the cipher suites that we build into edk2. I understand now. Thanks!

> Anyway, filtrating the unsupported cipher suites as early as possible is a wise choice. So, TlsCipherMappingTable should only include the supported cipher suites by reference the security requirement of CryptoPkg. 

Yes.

> 
> 3. For patch 0006, it's good to me to optimize the searching algorithm since the table is larger than before.
> 
> 4. Can we combined some patches together to make the things simple? e.g. Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to fix the issues in 0013.

I'm not against squashing these patches together, but separating patch
#6 (the binary search) out of the middle is not possible without a
rewrite of that patch, because it has context dependencies on patch #5.

Do you want me to do that? I.e., first implement the binary search for
TlsGetCipherString() -- without changing the interface --, and *then*
switch it over to TlsGetCipherMapping(), as part of the large squashed
patch?

> 
> 5. For patch 0008, I think it's unnecessary to provide such script. I prefer to maintain the TlsCipherMappingTable more statical since it's the internal mapping table. How about we keep it as internal assistant tool?

Sure, given that TlsCipherMappingTable *must* match the ciphers that we
build into OpensslLib, updating that table semi-automatically is out of
question (it can only be done manually, in synch with OpensslLib.inf
changes). Therefore the shell script is useless. I'll just drop the patch.

> 
> 6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us to provide the supported cipher suites by reference the security requirement of CryptoPkg.

Right -- I think I'll simply drop patch #9 from the v2 series, and I'll
let Qin post a separate patch later, so that TlsCipherMappingTable
matches the ciphers we build in 100%.

Qin, do you prefer that I open a separate TianoCore BZ for this? Also,
would you like to post the patch for TlsCipherMappingTable before or
after my v2?

Thanks!
Laszlo

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek]

On 04/10/18 09:40, Long, Qin wrote:
> Hi, Laszlo,
> 
> Some comments / discussions were added in https://bugzilla.tianocore.org/show_bug.cgi?id=915
> with comment 09 & 11.

Right. There I missed your main point about "TlsCipherMappingTable"; I'm
sorry about that. With Jiaxin's more detailed explanation, I get it now.

> Back to the patch review. Some comments were appended:
> 
> #0001, #0003, #0004，#0010，#0011:
>         Looks good to me.

Thanks -- are you OK if I squash 10 and 11, like Jiaxin suggests?

> #0002 - I personally think in general we should reduce using "#pragma pack", except that these
>         data really have serialization requirement (e.g. variable) to match extra data layout.
>         Here we just use these structures for setting / getting data, instead of direct data
>         transport. I am thinking if it's better to update the implementation part.
>         But too many sizeof() were used, and Ovmf part may also need to store preferred
>         CipherList data. So it's still good to me to pack something.

Well, I think the *set* of structures that should be packed is clear --
given that we make explicit references to the TLS RFC, I believe we
*must* pack those structures.

The remaining question I see (with reference to Liming's suggestion
earlier) is whether we should use separate #pragma directives for the
individual struct types, or if we should move those structs to a common
section of the header file called "structures from the TLS RFC" or
something like that, and pack them with a single #pragma.

> #0008, #0009:
>       - As the BZ comments. The TlsCipherMappingTable extension and generation with script looks
>         incorrect. This table should include all available / supported ciphers, which was actually
>         platform / configuration dependent.
>         I prefer to maintain one static / limited table for edk2 tls implementation. Any new cipher
>         requirement can be evaluated & enabled, and then added into this table.

Yup, I'm ready to drop patches #8 and #9. Thanks for your patience
explaining.

> #0005, #0006, #0007, #0012， #0013:
>         These implementation looks good to me.
>         But some of updates were based on the assumption of #0008-0009. I have no strong opinion
>         if some original light implementation are good enough currently.

Thanks. My personal preference would be the following -- making sure
Jiaxin is CC'd... yes, he is:

(a) clarify how exactly we want to pack the structures in patch #2, and
rework patch #2 according to agreement

(b) keep *each* of the following patches separate:

  01 OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS
     boot

  02 MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC

  03 NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList

  04 NetworkPkg/TlsDxe: clean up byte order conversion for
     EfiTlsCipherList

  05 CryptoPkg/TlsLib: replace TlsGetCipherString() with
     TlsGetCipherMapping()

  06 CryptoPkg/TlsLib: use binary search in the TlsGetCipherMapping()
     function

  07 CryptoPkg/TlsLib: pre-compute OpensslCipherLength in
     TlsCipherMappingTable

(c) drop the following patches:

  08 CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script

  09 CryptoPkg/TlsLib: extend "TlsCipherMappingTable"

(d) squash the following patches together (into one patch):

  10 CryptoPkg/TlsLib: sort [LibraryClasses] section in the INF file

  11 CryptoPkg/TlsLib: sanitize lib classes in internal header and INF

(e) squash the following patches together (into another patch):

  12 CryptoPkg/TlsLib: clean up leading comment for TlsSetCipherList()

  13 CryptoPkg/TlsLib: rewrite TlsSetCipherList()

Jiaxin, does this look OK to you?

Thanks!
Laszlo

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek]

On 04/10/18 12:02, Laszlo Ersek wrote:
> On 04/10/18 09:40, Long, Qin wrote:

>> #0005, #0006, #0007, #0012， #0013:
>>         These implementation looks good to me.
>>         But some of updates were based on the assumption of #0008-0009. I have no strong opinion
>>         if some original light implementation are good enough currently.

I'd like to comment on this in more detail (namely that "some original
light implementation are good enough currently"):

- I now agree that "TlsCipherMappingTable" should match the ciphers
built into OpensslLib exactly. However, that makes it only more
important that we *not* return EFI_UNSUPPORTED immediately when we find
a cipher suite in the platform's preference list that we don't support.
Instead, we should filter the platform's list down to what we do support.

- The stack allocation with 500 bytes for CipherString is questionable
practice, in my opinion, given that we add a variable list of cipher
suite names. It's just not deterministic. It can produce confusing
results that don't match the caller's (the platform's) intent, and it
will only become worse when you extend "TlsCipherMappingTable" to the
full cipher list that we build into OpensslLib *right now*. (And that's
not considering any future cipher enablements.)

- "@STRENGTH" must be dropped. I have no doubt about that. :)

So, I'd like to keep patch #13 as-is, perhaps squahed together with
patch #12 if you all prefer that.

Thanks!
Laszlo

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Long, Qin]

Thanks, Laszlo.
In fact, these implementation optimizations are good to me.  ☺


On 04/10/18 12:02, Laszlo Ersek wrote:
> On 04/10/18 09:40, Long, Qin wrote:

>> #0005, #0006, #0007, #0012， #0013:
>>         These implementation looks good to me.
>>         But some of updates were based on the assumption of #0008-0009. I have no strong opinion
>>         if some original light implementation are good enough currently.

I'd like to comment on this in more detail (namely that "some original
light implementation are good enough currently"):

- I now agree that "TlsCipherMappingTable" should match the ciphers
built into OpensslLib exactly. However, that makes it only more
important that we *not* return EFI_UNSUPPORTED immediately when we find
a cipher suite in the platform's preference list that we don't support.
Instead, we should filter the platform's list down to what we do support.

[qlong] Yes, I agree it’s better to filter out any unavailable items.

- The stack allocation with 500 bytes for CipherString is questionable
practice, in my opinion, given that we add a variable list of cipher
suite names. It's just not deterministic. It can produce confusing
results that don't match the caller's (the platform's) intent, and it
will only become worse when you extend "TlsCipherMappingTable" to the
full cipher list that we build into OpensslLib *right now*. (And that's
not considering any future cipher enablements.)

[qlong] Yes, the original fixed buffer is limited to the future extension.
        It’s good to me to have more flexible implementation.

- "@STRENGTH" must be dropped. I have no doubt about that. :)

[qlong] I agree. “@STRENGTH” will cause to re-order the preferred cipher lists.
          I prefer to keep the configuration-defined order.

So, I'd like to keep patch #13 as-is, perhaps squahed together with
patch #12 if you all prefer that.

[qlong] Sure. It’s OK for me.


Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Long, Qin]

Hi, Laszlo,

I prefer to open a separate BZ for this TlsCipherMappingTable update.
Current list was produced by some rough collections from Jiaxin and me, which meet the basic cipher requirement for TLS(v1.0/1.1/1.2) to set up one successful connection.

Will re-sorted this table based on IANA & IETF-RFCs & EDKII-openssl build options.


Best Regards & Thanks,
LONG, Qin

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, April 10, 2018 5:48 PM
To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Ye, Ting <ting.ye@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Gary Ching-Pang Lin <glin@suse.com>; Long, Qin <qin.long@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
Subject: Re: [edk2] [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

On 04/10/18 06:09, Wu, Jiaxin wrote:
> Hi Laszlo
>
> Appreciate your contribution. I have reviewed the series patches you attached here. First, I assume you have verified the patches on OVMF and the functionality works well,

That's right; I tested cipher suite negotiation failures and successes.

For example, I configured apache to "Disable All SSL and TLS Protocols
Except TLS 1 and Up"
<https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers#s2-apache-mod_ssl-enabling>,
and then I verified that HTTPS boot would succeed vs. fail dependent on
whether I passed strong ciphers too, or only weak ones, to OVMF.

> then, below are my comments:
>
> 1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me.

Thanks. For patch #2, "MdePkg/Include/Protocol/Tls.h: pack structures
from the TLS RFC", we exchanged some points with Liming earlier:

http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E1F1B10@SHSMSX104.ccr.corp.intel.com

(Please see also my response to that.)

I see that both you and Siyuan are OK with patch #2, i.e. with the
separate #pragma directives. I'd also like Liming to confirm that he
accepts the patch as-is.

>
> 2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable.

OK.

I think this means that I should preserve patches #5 through #7, and
drop patches #8 ('CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX
shell script') and #9 ('CryptoPkg/TlsLib: extend "TlsCipherMappingTable"').

Is that correct?

> After talked with Qin, I know the unsupported cipher suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, the cipher suite list that passed from the client to the server in the ClientHello message might also include such unsupported cipher suites. In such a case, the failure will happen once the server select the unsupported cipher suite. From the handshake process view, it's unreasonable since the client sent the desired cipher suites, then the server selected one of them but still met the error.

Oh! You are totally right. I apologize for missing this -- I didn't
realize this from Qin's comments on TianoCore #915.

In other words, it is actually *important* that "TlsCipherMappingTable"
match the cipher suites that we build into edk2. I understand now. Thanks!

> Anyway, filtrating the unsupported cipher suites as early as possible is a wise choice. So, TlsCipherMappingTable should only include the supported cipher suites by reference the security requirement of CryptoPkg.

Yes.

>
> 3. For patch 0006, it's good to me to optimize the searching algorithm since the table is larger than before.
>
> 4. Can we combined some patches together to make the things simple? e.g. Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to fix the issues in 0013.

I'm not against squashing these patches together, but separating patch
#6 (the binary search) out of the middle is not possible without a
rewrite of that patch, because it has context dependencies on patch #5.

Do you want me to do that? I.e., first implement the binary search for
TlsGetCipherString() -- without changing the interface --, and *then*
switch it over to TlsGetCipherMapping(), as part of the large squashed
patch?

>
> 5. For patch 0008, I think it's unnecessary to provide such script. I prefer to maintain the TlsCipherMappingTable more statical since it's the internal mapping table. How about we keep it as internal assistant tool?

Sure, given that TlsCipherMappingTable *must* match the ciphers that we
build into OpensslLib, updating that table semi-automatically is out of
question (it can only be done manually, in synch with OpensslLib.inf
changes). Therefore the shell script is useless. I'll just drop the patch.

>
> 6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us to provide the supported cipher suites by reference the security requirement of CryptoPkg.

Right -- I think I'll simply drop patch #9 from the v2 series, and I'll
let Qin post a separate patch later, so that TlsCipherMappingTable
matches the ciphers we build in 100%.

Qin, do you prefer that I open a separate TianoCore BZ for this? Also,
would you like to post the patch for TlsCipherMappingTable before or
after my v2?

Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Laszlo Ersek]

On 04/10/18 19:06, Long, Qin wrote:
> Hi, Laszlo,
> 
> I prefer to open a separate BZ for this TlsCipherMappingTable update.
> Current list was produced by some rough collections from Jiaxin and me, which meet the basic cipher requirement for TLS(v1.0/1.1/1.2) to set up one successful connection.
> 
> Will re-sorted this table based on IANA & IETF-RFCs & EDKII-openssl build options.

Thank you, I've just filed
<https://bugzilla.tianocore.org/show_bug.cgi?id=929> and assigned it to
you; I hope that's OK.

I hope to send v2 of this series tomorrow, or the day after, after I get
answers to the (remaining) open questions I asked today.

Thanks!
Laszlo

Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites

[Wu, Jiaxin]

It's good to me with new coming series patches:).

Thanks,
Jiaxin

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, April 11, 2018 4:06 AM
> To: Long, Qin <qin.long@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>;
> edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Ye, Ting
> <ting.ye@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Gao,
> Liming <liming.gao@intel.com>; Gary Ching-Pang Lin <glin@suse.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>; Fu, Siyuan
> <siyuan.fu@intel.com>
> Subject: Re: [edk2] [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg:
> fixes+features for setting HTTPS cipher suites
> 
> On 04/10/18 19:06, Long, Qin wrote:
> > Hi, Laszlo,
> >
> > I prefer to open a separate BZ for this TlsCipherMappingTable update.
> > Current list was produced by some rough collections from Jiaxin and me,
> which meet the basic cipher requirement for TLS(v1.0/1.1/1.2) to set up one
> successful connection.
> >
> > Will re-sorted this table based on IANA & IETF-RFCs & EDKII-openssl build
> options.
> 
> Thank you, I've just filed
> <https://bugzilla.tianocore.org/show_bug.cgi?id=929> and assigned it to
> you; I hope that's OK.
> 
> I hope to send v2 of this series tomorrow, or the day after, after I get
> answers to the (remaining) open questions I asked today.
> 
> Thanks!
> Laszlo