From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124])
 by mx.groups.io with SMTP id smtpd.web11.7032.1605778525736061972
 for <devel@edk2.groups.io>;
 Thu, 19 Nov 2020 01:35:26 -0800
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZfBrg1p9;
 spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
	s=mimecast20190719; t=1605778524;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
	 content-transfer-encoding:content-transfer-encoding:
	 in-reply-to:in-reply-to:references:references;
	bh=uIRpi7xYi5bE7zC6wVJaVl8y26HoJrzbOuXSp9AZtjw=;
	b=ZfBrg1p9nrP8JHyU3LSBo3ZnUgT+80oPCelJi3z4Gi2yN3gsT4xj6z9XH9XuM72jDeuC1l
	VkZgE0yFJPx3/XUnhJmOL6oN3s8d+XBaRohMrjrmOk2aqiZR7XgtpthLkh+6iK/AmU32CE
	FxgG+5pVm45o1iDOlurf9YqBCjpFUyc=
Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com
 [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id
 us-mta-302-USQy265fPHOKqlSKKdKVQQ-1; Thu, 19 Nov 2020 04:35:10 -0500
X-MC-Unique: USQy265fPHOKqlSKKdKVQQ-1
Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7922D8144F3;
	Thu, 19 Nov 2020 09:35:08 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-236.ams2.redhat.com [10.36.112.236])
	by smtp.corp.redhat.com (Postfix) with ESMTP id F27415D9C6;
	Thu, 19 Nov 2020 09:34:56 +0000 (UTC)
Subject: Re: [edk2-devel] Propose on enabling TLSv1.3
To: "Liu, Zhiguang" <zhiguang.liu@intel.com>,
 "Huang, Matthew (HPS SW)" <chao-jui.huang@hpe.com>,
 "devel@edk2.groups.io" <devel@edk2.groups.io>,
 "Yao, Jiewen" <jiewen.yao@intel.com>, "Wang, Jian J"
 <jian.j.wang@intel.com>,
 "maciej.rabeda@linux.intel.com" <maciej.rabeda@linux.intel.com>,
 "Wu, Jiaxin" <jiaxin.wu@intel.com>, "Fu, Siyuan" <siyuan.fu@intel.com>,
 "Xu, Min M" <min.m.xu@intel.com>, "Zhang, Qi1" <qi1.zhang@intel.com>,
 "Kumar, Rahul1" <rahul1.kumar@intel.com>
Cc: "Wei, Kent (HPS SW)" <kent.wei@hpe.com>,
 "Lin, Derek (HPS SW)" <derek.lin2@hpe.com>,
 "Wang, Nickle (HPS SW)" <nickle.wang@hpe.com>,
 "Wang, Sunny (HPS SW)" <sunnywang@hpe.com>,
 "vladimir.olovyannikov@broadcom.com" <vladimir.olovyannikov@broadcom.com>,
 "Tian, Hot" <hot.tian@intel.com>,
 "Madhavi Sinha, Fnu" <fnu.madhavi.sinha@intel.com>,
 "Ard Biesheuvel (ARM address)" <ard.biesheuvel@arm.com>,
 Nikos Mavrogiannopoulos <nmav@redhat.com>, Tomas Mraz <tmraz@redhat.com>,
 =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= <philmd@redhat.com>
References: <DF4PR8401MB1067E1659BBAA855D904A51CCD4D0@DF4PR8401MB1067.NAMPRD84.PROD.OUTLOOK.COM>
 <CY4PR11MB168717AEA14BE9306DDDED1C90440@CY4PR11MB1687.namprd11.prod.outlook.com>
 <1629CD946C53C473.23035@groups.io> <162A80E91C03CB2F.12108@groups.io>
 <DF4PR8401MB106700274C830BC47F00CCFBCD5D0@DF4PR8401MB1067.NAMPRD84.PROD.OUTLOOK.COM>
 <CY4PR11MB1687142E5C75D5E612B025A3902D0@CY4PR11MB1687.namprd11.prod.outlook.com>
 <CY4PR11MB168759C3C2B4E7ED9A67E0FE90E00@CY4PR11MB1687.namprd11.prod.outlook.com>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <59631a69-a285-b195-8f8c-de135e877def@redhat.com>
Date: Thu, 19 Nov 2020 10:34:55 +0100
MIME-Version: 1.0
In-Reply-To: <CY4PR11MB168759C3C2B4E7ED9A67E0FE90E00@CY4PR11MB1687.namprd11.prod.outlook.com>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14
Authentication-Results: relay.mimecast.com;
	auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 11/19/20 03:07, Liu, Zhiguang wrote:
> Hi all,
>
> I did some research about TLS v1.3 and here is my suggestion.

Related BZ:

https://bugzilla.tianocore.org/show_bug.cgi?id=2424

>
> I want to first collect information if we should continue to support
> TLS v1.1 or below.
> I personally suggest stopping support TLS v1.1 or below, for this is a
> trend.
>
> The first suggestion is about TLS version configuration.
> The supported TLS version should be a range not just a fixed value.
> For example, a TLS version from 1.2 to 1.3 are both accepted.
> However, there is not a direct UEFI API to set TLS version as a range.
> SetSessionData method in EFI_TLS_PROTOCOL will set the maximum and
> minimum TLS version as a same value.
> I have two solutions about this issue and want to collect feedback.
>
> Firstly, use a PCD to set the minimum TLS version in TlsDxe
> drivers entry point. Like:
> TlsCtxNew (PCD_Minimum_version_major, PCD_ Minimum_version_minor);
> Then the accepted TLS version will be from PCD_Minimum_version to 1.3
>
> Secondly, use loop to try different TLS versions, which can be
> implemented in platform side. Like:
>                 TLSVersionList = [1.3, 1.2, 1.1];
>                 for (int i=0; i<len(TLSVersionList); i++) {
>                                 SetSessionData(EfiTlsVersion, TLSVersionList[i]);
>                                 // then, try to connect, if it works, break
>                }
>
>
> The second comment is about how to set Cipher list.
> We can now change the gEdkiiHttpTlsCipherListGuid variable to change
> cipher list.
> The same variable and API is used from TLS v1.0 to TLS v1.2
> However, TLS v1.3 has a different API and totally different cipher
> list parameters.
> Below is the comparison:
> SSL_set_cipher_list() sets the list of ciphers (TLSv1.2 and below)
> The input cipher suite list must include at least one available cipher
> suite for TLSv1.2 and below.
> SSL_set_ciphersuites() is used to configure the available TLSv1.3
> ciphersuites
> The input cipher suite list must only include available cipher suite
> for TLSv1.3.
>
> I suggest introducing another table TlsV13CipherMappingTable to store
> the cipher list suit for TLS v1.3.
> Before calling SSL_set_ciphersuites(), use the mapping table to filter
> the cipher list, only remain the available ones.
> In this way, we can save all the cipher lists and cipher suites for
> TLS 1.3 and TLS v1.2 or below in one same variable.
> And before calling API to set cipher list, use the corresponding
> mapping table to filter.
>
> Also, for current TlsCipherMappingTable used for TLS v1.2 or below, I
> think the below cipher list are all insecure for they use MD5 and SHA.
> We should remove them.
> MAP ( 0x0001, "NULL-MD5" ),
> MAP ( 0x0002, "NULL-SHA" ),
> MAP ( 0x0004, "RC4-MD5" ),
> MAP ( 0x0005, "RC4-SHA" ),
> MAP ( 0x000A, "DES-CBC3-SHA" ),
> MAP ( 0x0016, "DHE-RSA-DES-CBC3-SHA" ),
> MAP ( 0x002F, "AES128-SHA" ),
> MAP ( 0x0030, "DH-DSS-AES128-SHA" ),
> MAP ( 0x0031, "DH-RSA-AES128-SHA" ),
> MAP ( 0x0033, "DHE-RSA-AES128-SHA" ),
> MAP ( 0x0035, "AES256-SHA" ),
> MAP ( 0x0036, "DH-DSS-AES256-SHA" ),
> MAP ( 0x0037, "DH-RSA-AES256-SHA" ),
> MAP ( 0x0039, "DHE-RSA-AES256-SHA" ),
>
> And for TLS v1.3, I suggest we enable the following cipher suite.
>
>   1.  TLS_AES_256_GCM_SHA384
>   2.  TLS_AES_128_GCM_SHA256
>   3.  TLS_AES_128_CCM_SHA256
>   4.  TLS_CHACHA20_POLY1305_SHA256
>
> Please correct me if I am wrong. Any comments are welcomed.

I'm not a crypto person, so please bear with me.

At Red Hat, we discussed TLSv1.3 in edk2 in the year 2018.

In particular, we discussed what UEFI (the spec) and edk2 (the reference
implementation) would need, for honoring what we call the "crypto
policy", with TLSv1.3.

After lots of discussions (which was basically a "learning from zero"
experience for me), I tried to summarize my then-understanding at:

  https://bugzilla.redhat.com/show_bug.cgi?id=1559564#c17

Let me re-hash that here, in order to remove the OVMF-specific bits,
plus it's going to be easier for others to respond here in-line.

There are five aspects of TLSv1.3 that we would like to control. These
are:

(a) TLS protocol version.
(b) Acceptable ciphers.
(c) Acceptable groups (secp256r1/ffdhe etc).
(d) Acceptable signature algorithms (rsa-sha1, rsa-sha256, ...).
(e) Minimum acceptable DH parameters (for <tls1.2).

For each of these, it needs to be investigated whether the UEFI spec is
already flexible enough to express the particular configuration trait.
If so, then the question is whether edk2 implements that configuration
interface already.

(a) TLS protocol version:

    - Currently, the UEFI spec allows for a single version setting.

    - For accepting a version range (or set), spec changes don't look
      necessary. The set of desired TLS version should be possible to
      pass via the following interface:

      - EFI_TLS_PROTOCOL.SetSessionData()

      - DataType=EfiTlsExtensionData

      - ExtensionType=43 ("supported_versions"):
        https://tools.ietf.org/html/rfc8446#section-4.2.1

    - Edk2 implementation: A PCD doesn't appear necessary. Edk2 does not
      implement DataType=EfiTlsExtensionData though, at the moment
      ("Status = EFI_UNSUPPORTED").

      Suggestion (in accordance with the spec interface):

      #pragma pack (1)
      typedef struct {
        UINT16                        ExtensionType; // this is where
                                                     // the protocol
                                                     // user would put
                                                     // value 43
        UINT16                        Length;
        UINT8                         Data[1]; // encode TLS
                                               // ProtocolVersion
                                               // (UINT16) values,
                                               // likely as
                                               // little-endian
      } EFI_TLS_EXTENSION;
      #pragma pack ()

      Use of this interface would supersede / conflict with
      EfiTlsVersion. In other words, using this facility after setting
      EfiTlsVersion would fail, and vice versa. The return value could
      be EFI_ALREADY_STARTED.

      Multiple TCP connections should not be created, especially not
      driven from the platform side -- OpenSSL should be pre-configured
      with the acceptable version list, and negotiation should occur
      internally to TLS, using just one TCP socket.


(b) Acceptable ciphers

  - The UEFI spec allows for an array of cipher IDs (EFI_TLS_CIPHER),
    which seems suitable wrt. TLSv1.3 as well.

  - Edk2 implements the interface already.

  - The edk2 implementation is not universally loved. There are (or have
    been) mismatches between the set of ciphers we include in the
    OpensslLib INF files and the "cipher suite filtering table" in the C
    code. Also, some contributors either dislike the specific content of
    the filter table, or even the fact that edk2 attempts to perform any
    kind of filtering there -- if a protocol client requests a
    particular cipher, there's an argument that the edk2 core should not
    judge whether that's a reasonable request or not. Under this idea
    the whole edk2-internal filtering should be removed (sane defaults
    should not be removed though).


(c) Acceptable groups (secp256r1/ffdhe etc)

    - Regarding the spec, this config property should map to the
      following (no new interface needed):

      - EFI_TLS_PROTOCOL.SetSessionData()

      - DataType=EfiTlsExtensionData

      - ExtensionType=10 ("supported_groups"):
        https://tools.ietf.org/html/rfc8446#section-4.2.7

    - Edk2 implementation: none currently; suggested:

      #pragma pack (1)
      typedef struct {
        UINT16                        ExtensionType; // this is where
                                                     // the protocol
                                                     // user would put
                                                     // value 10
        UINT16                        Length;
        UINT8                         Data[1]; // encode TLS
                                               // NamedGroup (UINT16)
                                               // values, likely as
                                               // little-endian
      } EFI_TLS_EXTENSION;
      #pragma pack ()


(d) Acceptable signature algorithms (rsa-sha1, rsa-sha256, ...)

    Same status as seen under (a) and (c), only with ExtensionType=13/50
    ("signature_algorithms" / "signature_algorithms_cert"):
    https://tools.ietf.org/html/rfc8446#section-4.2.3

    Suggested implementation (in accordance with the spec interface):

      #pragma pack (1)
      typedef struct {
        UINT16                        ExtensionType; // this is where
                                                     // the protocol
                                                     // user would put
                                                     // value 10 or 50
        UINT16                        Length;
        UINT8                         Data[1]; // encode TLS
                                               // SignatureScheme
                                               // (UINT16) values,
                                               // likely as
                                               // little-endian
      } EFI_TLS_EXTENSION;
      #pragma pack ()


(e) Minimum acceptable DH parameters (for <tls1.2)

    - UEFI spec interface:

      - key length is inexpressible

      - permitted groups map to (c)

      - no changes required because (at least from my notes from the
        last discussion at RH) expressing the permitted groups through
        (c) is sufficient

    - Edk2 implementation: none. However, a TLS protocol client can get
      around this limitation by forbidding such TLS cipher suites
      altogether that use DH key exchange (so DH parameters won't
      matter), via (b) / (c).


So, if I remember correctly our perspective at Red Hat,

- we'd need no spec changes,

- we'd need support for DataType=EfiTlsExtensionData in
  EFI_TLS_PROTOCOL.SetSessionData(), wherein ExtensionType values 43,
  10, and 13/50 should be propagated to OpenSSL, with their
  corresponding UINT16 (ProtocolVersion / NamedGroup / SignatureScheme)
  arrays encoded in EFI_TLS_EXTENSION.Length and EFI_TLS_EXTENSION.Data,

- the cipher suite filtering that's internal to edk2 remains dubious --
  minimally the INF file contents (OpenSSL source file list) should be
  synchronized with the filtering table(s).


... I'm fully aware this is not overly helpful, I'm just trying to
bridge the gap between our crypto experts (who haven't delved too much
into UEFI/edk2) and UEFI/edk2, with myself not being a crypto guy at
all.

Thanks
Laszlo