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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel