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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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