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

["Long, Qin" <qin.long@intel.com>]

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