Re: [PATCH v2 1/9] OvmfPkg/TlsAuthConfigLib: configure trusted cipher suites for HTTPS boot

[Gary Lin <glin@suse.com>]

On Wed, Apr 11, 2018 at 12:42:39PM +0200, Laszlo Ersek wrote:
> 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()).
> 
Hi Laszlo,

The description mentioned "update-crypto-policies" to format the cipher
list. The command is not available in openSUSE and I downloaded the command
from github repo[*]. However, I didn't find any command in the repo
could create the binary cipher list. Anyway, I found you also mentioned
"openssl ciphers -V" in the cover letter, and I managed to convert the
plaintext cipher list to the binary array. Maybe the description can be
improved to avoid the confusion. (Or, I just found the wrong program...)

BTW, the code looks good and works for me.

Reviewed-by: Gary Lin <glin@suse.com>
Tested-by: Gary Lin <glin@suse.com>

Cheers,

Gary Lin

[*] https://github.com/nmav/fedora-crypto-policies

> 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>
> ---
> 
> Notes:
>     v2:
>     - no change
> 
>  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
> 
> 
>