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
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
© 2016 - 2024 Red Hat, Inc.