CryptoPkg/Include/Library/TlsLib.h | 20 ++++++++ CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++- MdePkg/Include/Protocol/Tls.h | 68 +++++++++++++++++++++++----- NetworkPkg/HttpDxe/HttpProto.h | 1 + NetworkPkg/HttpDxe/HttpsSupport.c | 21 +++++++-- NetworkPkg/TlsDxe/TlsProtocol.c | 44 ++++++++++++++++-- 6 files changed, 173 insertions(+), 19 deletions(-)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960 CVE: CVE-2019-14553 The series patches are to support HTTPS hostname validation feature. It fixes the issue exposed @ https://bugzilla.tianocore.org/show_bug.cgi?id=960. In the patches, we add the new data type named "EfiTlsVerifyHost" and the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to enable the host name check so as to avoid the potential Man-In-The-Middle attack. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com> Reviewed-by: Ye Ting <ting.ye@intel.com> Reviewed-by: Long Qin <qin.long@intel.com> Reviewed-by: Fu Siyuan <siyuan.fu@intel.com> Acked-by: Laszlo Ersek <lersek@redhat.com> Jiaxin Wu (4): MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE-2019-14553) CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553) NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE-2019-14553) NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553) CryptoPkg/Include/Library/TlsLib.h | 20 ++++++++ CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++- MdePkg/Include/Protocol/Tls.h | 68 +++++++++++++++++++++++----- NetworkPkg/HttpDxe/HttpProto.h | 1 + NetworkPkg/HttpDxe/HttpsSupport.c | 21 +++++++-- NetworkPkg/TlsDxe/TlsProtocol.c | 44 ++++++++++++++++-- 6 files changed, 173 insertions(+), 19 deletions(-) -- 2.17.1.windows.2 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48183): https://edk2.groups.io/g/devel/message/48183 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
For this patch series, 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more. Remove it at push time and no need to send a v2. 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b required. Please raise it asap if any objections. 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin > Sent: Friday, September 27, 2019 11:45 AM > To: devel@edk2.groups.io > Cc: Wu, Jiaxin <jiaxin.wu@intel.com> > Subject: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation > feature(CVE-2019-14553) > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960 > CVE: CVE-2019-14553 > The series patches are to support HTTPS hostname validation feature. > It fixes the issue exposed @ > https://bugzilla.tianocore.org/show_bug.cgi?id=960. > In the patches, we add the new data type named "EfiTlsVerifyHost" and > the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to > enable the host name check so as to avoid the potential > Man-In-The-Middle attack. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com> > Reviewed-by: Ye Ting <ting.ye@intel.com> > Reviewed-by: Long Qin <qin.long@intel.com> > Reviewed-by: Fu Siyuan <siyuan.fu@intel.com> > Acked-by: Laszlo Ersek <lersek@redhat.com> > > Jiaxin Wu (4): > MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE- > 2019-14553) > CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553) > NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE- > 2019-14553) > NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553) > > CryptoPkg/Include/Library/TlsLib.h | 20 ++++++++ > CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++- > MdePkg/Include/Protocol/Tls.h | 68 +++++++++++++++++++++++----- > NetworkPkg/HttpDxe/HttpProto.h | 1 + > NetworkPkg/HttpDxe/HttpsSupport.c | 21 +++++++-- > NetworkPkg/TlsDxe/TlsProtocol.c | 44 ++++++++++++++++-- > 6 files changed, 173 insertions(+), 19 deletions(-) > > -- > 2.17.1.windows.2 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48258): https://edk2.groups.io/g/devel/message/48258 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/29/19 08:09, Wang, Jian J wrote: > For this patch series, > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more. > Remove it at push time and no need to send a v2. > 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b > required. Please raise it asap if any objections. > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> * Can you please confirm that these patches match those that we discussed here: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 * In the BZ, David and Bret raised some questions: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 and https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 The latest comment in the bug is c#41. I'm not under the impression that all concerns raised by David and Bret have been addressed (or abandoned). I'd like David and Bret to ACK the patches. Thanks, Laszlo >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin >> Sent: Friday, September 27, 2019 11:45 AM >> To: devel@edk2.groups.io >> Cc: Wu, Jiaxin <jiaxin.wu@intel.com> >> Subject: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation >> feature(CVE-2019-14553) >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960 >> CVE: CVE-2019-14553 >> The series patches are to support HTTPS hostname validation feature. >> It fixes the issue exposed @ >> https://bugzilla.tianocore.org/show_bug.cgi?id=960. >> In the patches, we add the new data type named "EfiTlsVerifyHost" and >> the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to >> enable the host name check so as to avoid the potential >> Man-In-The-Middle attack. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com> >> Reviewed-by: Ye Ting <ting.ye@intel.com> >> Reviewed-by: Long Qin <qin.long@intel.com> >> Reviewed-by: Fu Siyuan <siyuan.fu@intel.com> >> Acked-by: Laszlo Ersek <lersek@redhat.com> >> >> Jiaxin Wu (4): >> MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE- >> 2019-14553) >> CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553) >> NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE- >> 2019-14553) >> NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553) >> >> CryptoPkg/Include/Library/TlsLib.h | 20 ++++++++ >> CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++- >> MdePkg/Include/Protocol/Tls.h | 68 +++++++++++++++++++++++----- >> NetworkPkg/HttpDxe/HttpProto.h | 1 + >> NetworkPkg/HttpDxe/HttpsSupport.c | 21 +++++++-- >> NetworkPkg/TlsDxe/TlsProtocol.c | 44 ++++++++++++++++-- >> 6 files changed, 173 insertions(+), 19 deletions(-) >> >> -- >> 2.17.1.windows.2 >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48328): https://edk2.groups.io/g/devel/message/48328 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > On 09/29/19 08:09, Wang, Jian J wrote: > > For this patch series, > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more. > > Remove it at push time and no need to send a v2. > > 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b > > required. Please raise it asap if any objections. > > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > > > * Can you please confirm that these patches match those that we > discussed here: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > * In the BZ, David and Bret raised some questions: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > and > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > The latest comment in the bug is c#41. I'm not under the impression that > all concerns raised by David and Bret have been addressed (or > abandoned). I'd like David and Bret to ACK the patches. I do not believe my comment #35 has been addressed, nor the requested testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48340): https://edk2.groups.io/g/devel/message/48340 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi David, I just realized you have the comments on Bugzilla 960: >"...given that testing is failing and code inspection shows it would never have been expected to work." Do you mean you didn't pass the verification if URLs with IPv6 literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me where the code inspection indicated it would never have been expected to work? We do pass the testing for the URLs with IPv6 if the CN or SAN in certificate has the corresponding IPv6 address (at least working with openssl 1.1.0). For the series patches here, we are intending to support the host name validation, I think we can commit the series patches since we pass the verification of IPV6 URL, what do you think? Thanks, Jiaxin > -----Original Message----- > From: David Woodhouse <dwmw2@infradead.org> > Sent: Tuesday, October 1, 2019 5:02 PM > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wang, Jian J > <jian.j.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Bret Barkelew > <Bret.Barkelew@microsoft.com> > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > > On 09/29/19 08:09, Wang, Jian J wrote: > > > For this patch series, > > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not > needed any more. > > > Remove it at push time and no need to send a v2. > > > 2. Since it's security patch which had been reviewed separately, I see no > reason for new r-b > > > required. Please raise it asap if any objections. > > > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > > > > > > * Can you please confirm that these patches match those that we > > discussed here: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > > > > * In the BZ, David and Bret raised some questions: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > and > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > The latest comment in the bug is c#41. I'm not under the impression that > > all concerns raised by David and Bret have been addressed (or > > abandoned). I'd like David and Bret to ACK the patches. > > I do not believe my comment #35 has been addressed, nor the requested > testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48547): https://edk2.groups.io/g/devel/message/48547 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote: > Hi David, > > I just realized you have the comments on Bugzilla 960: > > > "...given that testing is failing and code inspection shows it > > would never have been expected to work." > > Do you mean you didn't pass the verification if URLs with IPv6 > literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me > where the code inspection indicated it would never have been expected > to work? We do pass the testing for the URLs with IPv6 if the CN or > SAN in certificate has the corresponding IPv6 address (at least > working with openssl 1.1.0). I have not tested this, but I started looking when there was a message on the edk2 list from someone who was reporting that it didn't work for IPv6 URIs, IIRC. You are using SSL_set1_host(), and I believe you're just passing in the bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]". That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the SSL connection. In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the code simply iterates over the members of that list, calling X509_check_host() for each one. It never calls X509_check_ip(). If you look in openssl/crypto/x509/v3_utl.c you can see the X509_check_host() really does only check hostnames. You'd need to call X509_check_ip_asc() to check hostnames. And something would need to have stripped the [] which surround an IPv6 literal. I can't see how this can work. Have you tested it since the report on the list that it wasn't working? cf. https://github.com/openssl/openssl/pull/9201 which is being ignored by the OpenSSL developers — OpenSSL really doesn't make life easy for you here, which is a shame. > For the series patches here, we are intending to support the host > name validation, I think we can commit the series patches since we > pass the verification of IPV6 URL, what do you think? If it passes the verification of IPv6 literals, then all my analysis is broken and so was the report on the list that prompted me to start looking (or I'm misremembering that report). In that case, sure, go ahead and commit. > Thanks, > Jiaxin > > > -----Original Message----- > > From: David Woodhouse <dwmw2@infradead.org> > > Sent: Tuesday, October 1, 2019 5:02 PM > > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wang, > > Jian J > > <jian.j.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Bret > > Barkelew > > <Bret.Barkelew@microsoft.com> > > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > > validation feature(CVE-2019-14553) > > > > On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > > > On 09/29/19 08:09, Wang, Jian J wrote: > > > > For this patch series, > > > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" > > > > is not > > > > needed any more. > > > > Remove it at push time and no need to send a v2. > > > > 2. Since it's security patch which had been reviewed > > > > separately, I see no > > > > reason for new r-b > > > > required. Please raise it asap if any objections. > > > > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > > > > > > > > > * Can you please confirm that these patches match those that we > > > discussed here: > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > > > > > > > * In the BZ, David and Bret raised some questions: > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > > > and > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > > > The latest comment in the bug is c#41. I'm not under the > > > impression that > > > all concerns raised by David and Bret have been addressed (or > > > abandoned). I'd like David and Bret to ACK the patches. > > > > I do not believe my comment #35 has been addressed, nor the > > requested > > testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48626): https://edk2.groups.io/g/devel/message/48626 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi All, (multi-hour composition ahead...) On 10/09/19 09:53, David Woodhouse wrote: > On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote: >> Hi David, >> >> I just realized you have the comments on Bugzilla 960: >> >>> "...given that testing is failing and code inspection shows it >>> would never have been expected to work." >> >> Do you mean you didn't pass the verification if URLs with IPv6 >> literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me >> where the code inspection indicated it would never have been expected >> to work? We do pass the testing for the URLs with IPv6 if the CN or >> SAN in certificate has the corresponding IPv6 address (at least >> working with openssl 1.1.0). > > I have not tested this, but I started looking when there was a message > on the edk2 list from someone who was reporting that it didn't work > for IPv6 URIs, IIRC. > > You are using SSL_set1_host(), and I believe you're just passing in > the bare hostname part of the URI, be it "1.2.3.4" or > "[2001:8b0:10b::5]". > > That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the > SSL connection. > > In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the > code simply iterates over the members of that list, calling > X509_check_host() for each one. It never calls X509_check_ip(). > > If you look in openssl/crypto/x509/v3_utl.c you can see the > X509_check_host() really does only check hostnames. You'd need to call > X509_check_ip_asc() to check hostnames. And something would need to > have stripped the [] which surround an IPv6 literal. > > I can't see how this can work. Have you tested it since the report on > the list that it wasn't working? > > cf. https://github.com/openssl/openssl/pull/9201 which is being > ignored by the OpenSSL developers OpenSSL really doesn't make > life easy for you here, which is a shame. > > >> For the series patches here, we are intending to support the host >> name validation, I think we can commit the series patches since we >> pass the verification of IPV6 URL, what do you think? > > If it passes the verification of IPv6 literals, then all my analysis > is broken and so was the report on the list that prompted me to start > looking (or I'm misremembering that report). In that case, sure, go > ahead and commit. Here's a summary of my setup. * I've generated a brand new CA certificate, and two HTTP server certificates, signed by the CA. * One HTTP server certificate is for Common Name = 192.168.124.2 * The other HTTP server certificate is for Common Name = fd33:eb1b:9b36::2 * I have a "net-server" virtual machine that runs Apache on the above IP addresses (TCP port 443). - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. - The DHCP servers send the following boot file names: - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv4] - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv6] * For sanity-checking the environment, I run the following two commands on the *host* (connecting to the "net-server" virtual machine): - curl -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - The host is configured to trust the brand new test CA certificate (see near the top). - When the certificates are assigned *correctly* to the IP addresses in the Apache configuration, the above "curl" commands complete just fine. If I add the "-v" option to "curl", it confirms the right certificates are used, and it confirms the test CA as issuer too. - When the certificates are (intentionally) *cross-assigned* to the IP addresses in the Apache configuration, then both "curl" commands break with the following error message: > curl: (51) Unable to communicate securely with peer: requested domain > name does not match the server's certificate. - If I add the "-v" option, I also see > NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) - As a side comment: Apache itself warns about the misconfig, in "/var/log/httpd/ssl_error_log": > ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 > does NOT include an ID which matches the server name * I have a "net-client" virtual machine, running OVMF. - The edk2 HTTPS/TLS client booting in this virtual machine is configured to trust the exact same set of CA certificates that the host trusts too. - In other words, HTTPS boot in the "net-client" VM accepts server certificates signed by the new test CA. * The following is the test plan. 1. The patch set is *not* applied (that is, OVMF is built at current master, commit 976d0353a6ce). 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (correct behavior, establishes baseline) 2. HTTPSv6 boot --> expect success (correct behavior, establishes baseline) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect success (for reproducing the bug) 2. HTTPSv6 boot --> expect success (for reproducing the bug) 2. With the patch set applied: 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (failure means a regression) 2. HTTPSv6 boot --> expect success (failure means a regression) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect failure (for verifying the bugfix) 2. HTTPSv6 boot --> expect failure (for verifying the bugfix) * Results: - 1.1.1. as expected (HTTPSv4 baseline established) - 1.1.2. as expected (HTTPSv6 baseline established) - 1.2.1. as expected (HTTPSv4 MITM bug reproduced) - 1.2.2. as expected (HTTPSv6 MITM bug reproduced) - 2.1.1. as expected (HTTPSv4 not regressed by series) - 2.1.2. as expected (HTTPSv6 not regressed by series) - 2.2.1. as expected (HTTPSv4 MITM averted) - 2.2.2. as expected (HTTPSv6 MITM averted) * In cases 2.2.1. and 2.2.2.: - The UEFI console contains, respectively: > >>Start HTTP Boot over IPv4.... > Station IP address is 192.168.124.106 > > URI: https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso > > Error: Could not retrieve NBP file size from HTTP server. > > Error: Unexpected network error. > >>Start HTTP Boot over IPv6.... > Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8 > > URI: https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso > > Error: Could not retrieve NBP file size from HTTP server. > > Error: Unexpected network error. - The OVMF log contains (in both cases): > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86 - Decoding: - Library 0x14 -> ERR_LIB_SSL - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE - Reason 0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED - So this means that the ssl_verify_cert_chain() call fails in the tls_process_server_certificate() function, in "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c". Normally the above would be sufficient for me to give a "Tested-by" for this patch set. But now I'm uncertain whether (a) my results contradict David's analysis, or (b) I tested something that David's analysis doesn't *apply* to. (IOW if my test plan doesn't actually verify "IPv6 literals".) FWIW, the brackets in the IPv6 notation are stripped in EfiHttpRequest() [NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable. (The stripping comes from earlier commit 7191827f90b4 ("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed HostName.", 2018-08-03).) Later in EfiHttpRequest(), "HttpInstance->RemoteHost" is assigned "HostName". Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c], we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to "HttpInstance->RemoteHost". This is done in patch#4. Also in patch#4, in the same function, we pass "HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from patch#3. There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which resides in patch#2. At that point, we pass the hostname -- the IPv6 address, with the brackets stripped -- to SSL_set1_host(). So, my take is that the comparison is done simply on the textual representation (with the IPv6 brackets stripped), not the numerical value. Is that bad? The textual comparison may certainly report a mismatch when the numerical values actually match (for an IPv4 example, "192.168.0.1" would not match "192.168.000.001"). But that errs in the safe direction, does it not? Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48669): https://edk2.groups.io/g/devel/message/48669 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on your certs please. Would like to check if you really have a cert for the hostname string "192.168.124.2" or to the IP address. They are different things. On 9 October 2019 21:24:34 BST, Laszlo Ersek <lersek@redhat.com> wrote: >Hi All, > >(multi-hour composition ahead...) > >On 10/09/19 09:53, David Woodhouse wrote: >> On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote: >>> Hi David, >>> >>> I just realized you have the comments on Bugzilla 960: >>> >>>> "...given that testing is failing and code inspection shows it >>>> would never have been expected to work." >>> >>> Do you mean you didn't pass the verification if URLs with IPv6 >>> literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me >>> where the code inspection indicated it would never have been >expected >>> to work? We do pass the testing for the URLs with IPv6 if the CN or >>> SAN in certificate has the corresponding IPv6 address (at least >>> working with openssl 1.1.0). >> >> I have not tested this, but I started looking when there was a >message >> on the edk2 list from someone who was reporting that it didn't work >> for IPv6 URIs, IIRC. >> >> You are using SSL_set1_host(), and I believe you're just passing in >> the bare hostname part of the URI, be it "1.2.3.4" or >> "[2001:8b0:10b::5]". >> >> That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for >the >> SSL connection. >> >> In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the >> code simply iterates over the members of that list, calling >> X509_check_host() for each one. It never calls X509_check_ip(). >> >> If you look in openssl/crypto/x509/v3_utl.c you can see the >> X509_check_host() really does only check hostnames. You'd need to >call >> X509_check_ip_asc() to check hostnames. And something would need to >> have stripped the [] which surround an IPv6 literal. >> >> I can't see how this can work. Have you tested it since the report on >> the list that it wasn't working? >> >> cf. https://github.com/openssl/openssl/pull/9201 which is being >> ignored by the OpenSSL developers OpenSSL really doesn't make >> life easy for you here, which is a shame. >> >> >>> For the series patches here, we are intending to support the host >>> name validation, I think we can commit the series patches since we >>> pass the verification of IPV6 URL, what do you think? >> >> If it passes the verification of IPv6 literals, then all my analysis >> is broken and so was the report on the list that prompted me to start >> looking (or I'm misremembering that report). In that case, sure, go >> ahead and commit. > >Here's a summary of my setup. > >* I've generated a brand new CA certificate, and two HTTP server > certificates, signed by the CA. > >* One HTTP server certificate is for Common Name = 192.168.124.2 > >* The other HTTP server certificate is for Common Name = > fd33:eb1b:9b36::2 > >* I have a "net-server" virtual machine that runs Apache on the above >IP > addresses (TCP port 443). > > - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. > > - The DHCP servers send the following boot file names: > >- "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" > [IPv4] >- >"https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" >[IPv6] > >* For sanity-checking the environment, I run the following two commands > on the *host* (connecting to the "net-server" virtual machine): > >- curl -I >'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' >- curl --globoff -I >'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' > > - The host is configured to trust the brand new test CA certificate > (see near the top). > > - When the certificates are assigned *correctly* to the IP addresses > in the Apache configuration, the above "curl" commands complete just > fine. If I add the "-v" option to "curl", it confirms the right > certificates are used, and it confirms the test CA as issuer too. > > - When the certificates are (intentionally) *cross-assigned* to the IP > addresses in the Apache configuration, then both "curl" commands > break with the following error message: > >> curl: (51) Unable to communicate securely with peer: requested domain >> name does not match the server's certificate. > > - If I add the "-v" option, I also see > >> NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) > > - As a side comment: Apache itself warns about the misconfig, in > "/var/log/httpd/ssl_error_log": > >> ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 >> does NOT include an ID which matches the server name > >* I have a "net-client" virtual machine, running OVMF. > > - The edk2 HTTPS/TLS client booting in this virtual machine is > configured to trust the exact same set of CA certificates that the > host trusts too. > > - In other words, HTTPS boot in the "net-client" VM accepts server > certificates signed by the new test CA. > >* The following is the test plan. > >1. The patch set is *not* applied (that is, OVMF is built at current > master, commit 976d0353a6ce). > > 1. Properly assigned certificates: > > 1. HTTPSv4 boot --> expect success (correct behavior, establishes > baseline) > > 2. HTTPSv6 boot --> expect success (correct behavior, establishes > baseline) > > 2. Cross-assigned certificates: > > 1. HTTPSv4 boot --> expect success (for reproducing the bug) > > 2. HTTPSv6 boot --> expect success (for reproducing the bug) > >2. With the patch set applied: > > 1. Properly assigned certificates: > > 1. HTTPSv4 boot --> expect success (failure means a regression) > > 2. HTTPSv6 boot --> expect success (failure means a regression) > > 2. Cross-assigned certificates: > > 1. HTTPSv4 boot --> expect failure (for verifying the bugfix) > > 2. HTTPSv6 boot --> expect failure (for verifying the bugfix) > >* Results: > >- 1.1.1. as expected (HTTPSv4 baseline established) >- 1.1.2. as expected (HTTPSv6 baseline established) >- 1.2.1. as expected (HTTPSv4 MITM bug reproduced) >- 1.2.2. as expected (HTTPSv6 MITM bug reproduced) >- 2.1.1. as expected (HTTPSv4 not regressed by series) >- 2.1.2. as expected (HTTPSv6 not regressed by series) >- 2.2.1. as expected (HTTPSv4 MITM averted) >- 2.2.2. as expected (HTTPSv6 MITM averted) > >* In cases 2.2.1. and 2.2.2.: > >- The UEFI console contains, respectively: > >> >>Start HTTP Boot over IPv4.... >> Station IP address is 192.168.124.106 >> >> URI: >https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso >> >> Error: Could not retrieve NBP file size from HTTP server. >> >> Error: Unexpected network error. > >> >>Start HTTP Boot over IPv6.... >> Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8 >> >> URI: >https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso >> >> Error: Could not retrieve NBP file size from HTTP server. >> >> Error: Unexpected network error. > >- The OVMF log contains (in both cases): > >> TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL >> TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86 > >- Decoding: > > - Library 0x14 -> ERR_LIB_SSL > - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE > - Reason 0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED > > - So this means that the ssl_verify_cert_chain() call fails in the > tls_process_server_certificate() function, in > "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c". > > >Normally the above would be sufficient for me to give a "Tested-by" for >this patch set. > >But now I'm uncertain whether (a) my results contradict David's >analysis, or (b) I tested something that David's analysis doesn't >*apply* to. (IOW if my test plan doesn't actually verify "IPv6 >literals".) > > >FWIW, the brackets in the IPv6 notation are stripped in >EfiHttpRequest() >[NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable. >(The stripping comes from earlier commit 7191827f90b4 >("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed >HostName.", 2018-08-03).) Later in EfiHttpRequest(), >"HttpInstance->RemoteHost" is assigned "HostName". > >Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c], >we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to >"HttpInstance->RemoteHost". This is done in patch#4. > >Also in patch#4, in the same function, we pass >"HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from >patch#3. > >There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which >resides in patch#2. > >At that point, we pass the hostname -- the IPv6 address, with the >brackets stripped -- to SSL_set1_host(). > >So, my take is that the comparison is done simply on the textual >representation (with the IPv6 brackets stripped), not the numerical >value. > >Is that bad? The textual comparison may certainly report a mismatch >when >the numerical values actually match (for an IPv4 example, "192.168.0.1" >would not match "192.168.000.001"). But that errs in the safe >direction, >does it not? > >Thanks! >Laszlo -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48670): https://edk2.groups.io/g/devel/message/48670 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Below summary of setup from Laszlo are also the part of our test cases. Beside those verification that only focus on the subject Common Name (CN), we also created the certificate contains Subject Alternative Name (SAN) to cover more test cases related to the subjectAltName. That’s also the parts of hostname verification. With the subjectAltName, we can set the multiple subject names (including IP address format defined in RFC 5280) in one certificate to pass the different URIs (IPv4/IPv6) retrieved from the DHCP server. > So, my take is that the comparison is done simply on the textual > representation (with the IPv6 brackets stripped), not the numerical > value. > Is that bad? The textual comparison may certainly report a mismatch when > the numerical values actually match (for an IPv4 example, "192.168.0.1" > would not match "192.168.000.001"). But that errs in the safe direction, > does it not? I think it’s not the problem here by setting textual IP in hostname since the numerical IP is not the target to be vitrificated. And I also post my explain in the previous email: > UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), > not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: > "...TLS session hostname for validation which is used to verify whether the name > within the peer certificate matches a given host name..." > In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real > FQDN (www.xxx.com<http://www.xxx.com>) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), > and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. > That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature > by following the standard TLS HostName verification capability. If above is incorrect, please correct me. Anyway, thanks Laszlo and David’s comments. Jiaxin From: David Woodhouse <dwmw2@infradead.org> Sent: Thursday, October 10, 2019 4:34 AM To: Laszlo Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com> Cc: Richard Levitte <levitte@openssl.org> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553) Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on your certs please. Would like to check if you really have a cert for the hostname string "192.168.124.2" or to the IP address. They are different things. On 9 October 2019 21:24:34 BST, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote: Hi All, (multi-hour composition ahead...) On 10/09/19 09:53, David Woodhouse wrote: On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote: Hi David, I just realized you have the comments on Bugzilla 960: "...given that testing is failing and code inspection shows it would never have been expected to work." Do you mean you didn't pass the verification if URLs with IPv6 literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me where the code inspection indicated it would never have been expected to work? We do pass the testing for the URLs with IPv6 if the CN or SAN in certificate has the corresponding IPv6 address (at least working with openssl 1.1.0). I have not tested this, but I started looking when there was a message on the edk2 list from someone who was reporting that it didn't work for IPv6 URIs, IIRC. You are using SSL_set1_host(), and I believe you're just passing in the bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]". That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the SSL connection. In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the code simply iterates over the members of that list, calling X509_check_host() for each one. It never calls X509_check_ip(). If you look in openssl/crypto/x509/v3_utl.c you can see the X509_check_host() really does only check hostnames. You'd need to call X509_check_ip_asc() to check hostnames. And something would need to have stripped the [] which surround an IPv6 literal. I can't see how this can work. Have you tested it since the report on the list that it wasn't working? cf. https://github.com/openssl/openssl/pull/9201 which is being ignored by the OpenSSL developers OpenSSL really doesn't make life easy for you here, which is a shame. For the series patches here, we are intending to support the host name validation, I think we can commit the series patches since we pass the verification of IPV6 URL, what do you think? If it passes the verification of IPv6 literals, then all my analysis is broken and so was the report on the list that prompted me to start looking (or I'm misremembering that report). In that case, sure, go ahead and commit. Here's a summary of my setup. * I've generated a brand new CA certificate, and two HTTP server certificates, signed by the CA. * One HTTP server certificate is for Common Name = 192.168.124.2 * The other HTTP server certificate is for Common Name = fd33:eb1b:9b36::2 * I have a "net-server" virtual machine that runs Apache on the above IP addresses (TCP port 443). - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. - The DHCP servers send the following boot file names: - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv4] - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv6] * For sanity-checking the environment, I run the following two commands on the *host* (connecting to the "net-server" virtual machine): - curl -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - The host is configured to trust the brand new test CA certificate (see near the top). - When the certificates are assigned *correctly* to the IP addresses in the Apache configuration, the above "curl" commands complete just fine. If I add the "-v" option to "curl", it confirms the right certificates are used, and it confirms the test CA as issuer too. - When the certificates are (intentionally) *cross-assigned* to the IP addresses in the Apache configuration, then both "curl" commands break with the following error message: curl: (51) Unable to communicate securely with peer: requested domain name does not match the server's certificate. - If I add the "-v" option, I also see NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) - As a side comment: Apache itself warns about the misconfig, in "/var/log/httpd/ssl_error_log": ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 does NOT include an ID which matches the server name * I have a "net-client" virtual machine, running OVMF. - The edk2 HTTPS/TLS client booting in this virtual machine is configured to trust the exact same set of CA certificates that the host trusts too. - In other words, HTTPS boot in the "net-client" VM accepts server certificates signed by the new test CA. * The following is the test plan. 1. The patch set is *not* applied (that is, OVMF is built at current master, commit 976d0353a6ce). 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (correct behavior, establishes baseline) 2. HTTPSv6 boot --> expect success (correct behavior, establishes baseline) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect success (for reproducing the bug) 2. HTTPSv6 boot --> expect success (for reproducing the bug) 2. With the patch set applied: 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (failure means a regression) 2. HTTPSv6 boot --> expect success (failure means a regression) 2. Cross-assigned certificates: 1. HTTPSv4 boot --> expect failure (for verifying the bugfix) 2. HTTPSv6 boot --> expect failure (for verifying the bugfix) * Results: - 1.1.1. as expected (HTTPSv4 baseline established) - 1.1.2. as expected (HTTPSv6 baseline established) - 1.2.1. as expected (HTTPSv4 MITM bug reproduced) - 1.2.2. as expected (HTTPSv6 MITM bug reproduced) - 2.1.1. as expected (HTTPSv4 not regressed by series) - 2.1.2. as expected (HTTPSv6 not regressed by series) - 2.2.1. as expected (HTTPSv4 MITM averted) - 2.2.2. as expected (HTTPSv6 MITM averted) * In cases 2.2.1. and 2.2.2.: - The UEFI console contains, respectively: Start HTTP Boot over IPv4.... Station IP address is 192.168.124.106 URI: https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso Error: Could not retrieve NBP file size from HTTP server. Error: Unexpected network error. Start HTTP Boot over IPv6.... Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8 URI: https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso Error: Could not retrieve NBP file size from HTTP server. Error: Unexpected network error. - The OVMF log contains (in both cases): TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86 - Decoding: - Library 0x14 -> ERR_LIB_SSL - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE - Reason 0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED - So this means that the ssl_verify_cert_chain() call fails in the tls_process_server_certificate() function, in "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c". Normally the above would be sufficient for me to give a "Tested-by" for this patch set. But now I'm uncertain whether (a) my results contradict David's analysis, or (b) I tested something that David's analysis doesn't *apply* to. (IOW if my test plan doesn't actually verify "IPv6 literals".) FWIW, the brackets in the IPv6 notation are stripped in EfiHttpRequest() [NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable. (The stripping comes from earlier commit 7191827f90b4 ("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed HostName.", 2018-08-03).) Later in EfiHttpRequest(), "HttpInstance->RemoteHost" is assigned "HostName". Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c], we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to "HttpInstance->RemoteHost". This is done in patch#4. Also in patch#4, in the same function, we pass "HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from patch#3. There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which resides in patch#2. At that point, we pass the hostname -- the IPv6 address, with the brackets stripped -- to SSL_set1_host(). So, my take is that the comparison is done simply on the textual representation (with the IPv6 brackets stripped), not the numerical value. Is that bad? The textual comparison may certainly report a mismatch when the numerical values actually match (for an IPv4 example, "192.168.0.1" would not match "192.168.000.001"). But that errs in the safe direction, does it not? Thanks! Laszlo -- Sent from my Android device with K-9 Mail. Please excuse my brevity. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48686): https://edk2.groups.io/g/devel/message/48686 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/09/19 22:34, David Woodhouse wrote: > Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on > your certs please. Sure. I had thought of that actually (I could have attached the certificates at once), but I figured, let me not share crypto stuff unless specifically asked for :) > Would like to check if you really have a cert for the hostname string > "192.168.124.2" or to the IP address. They are different things. Very interesting! This makes me curious. The *host* certificates were generated with the "genkey" utility ("Red Hat Keypair Generation"), not with naked openssl tool invocations. That's because Apache setup is not for the faint-hearted, and I had decided up-front (when I first looked into setting up mod_ssl) that I'd follow the official documentation, here: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers So I ran genkey 'fd33:eb1b:9b36::2' genkey 192.168.124.2 and went through the dialogs, which were already filled in with the argument passed on the command line (which the genkey manual calls "hostname"). The *CA* cert was generated with openssl directly, however. Also, for signing the CSRs (certificate signing requests), produced by "genkey", I also used openssl manually. Please find the certificates below (including the CA certificate): > Certificate: > Data: > Version: 3 (0x2) > Serial Number: > a7:b5:04:75:6a:2f:ee:7e > Signature Algorithm: sha256WithRSAEncryption > Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com > Validity > Not Before: Oct 9 16:06:08 2019 GMT > Not After : Nov 8 16:06:08 2019 GMT > Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com > Subject Public Key Info: > Public Key Algorithm: rsaEncryption > Public-Key: (2048 bit) > Modulus: > 00:d4:db:3b:fa:98:bd:15:02:3a:32:ea:64:d5:1d: > d8:80:03:fa:fb:4e:d8:47:45:3b:57:6a:36:80:83: > e1:6d:c4:0b:f7:24:00:ad:54:63:77:dd:86:71:f3: > fc:f4:e5:81:d4:6c:7d:23:b9:58:9e:cc:93:ee:93: > ed:24:62:8b:94:8f:de:3a:6a:a9:cd:1a:38:f0:df: > 17:91:7a:22:ca:35:94:3a:1f:cb:56:97:be:bb:69: > 1f:12:3a:7c:a4:35:6c:15:0e:27:a0:0a:2b:3d:61: > bd:ed:a6:84:29:bc:0a:d8:0c:98:4c:e9:7d:6b:36: > da:29:1d:49:57:8d:89:2a:62:e7:4f:00:56:46:9b: > a7:21:9f:b6:83:c7:dd:69:b0:8b:a3:bf:fa:ef:50: > a4:05:6f:b1:87:83:87:93:c7:ba:0e:cb:50:28:05: > f5:a7:0a:fe:be:13:71:1a:4b:6c:7b:f6:c9:cc:b2: > 65:cd:62:29:e3:08:c7:da:5c:ca:4d:dd:74:a4:d1: > 37:52:ea:ad:72:87:cf:48:f3:85:af:15:ab:e8:dc: > 50:f2:f7:7a:59:ed:b7:69:4f:a2:55:39:e5:ae:09: > 75:45:69:a5:3a:a0:cd:52:ee:f2:15:d5:3c:fc:c0: > 00:b7:25:71:ba:00:ba:63:08:8a:fc:b6:af:94:a4: > 5b:cb > Exponent: 65537 (0x10001) > X509v3 extensions: > X509v3 Subject Key Identifier: > 2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5 > X509v3 Authority Key Identifier: > keyid:2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5 > > X509v3 Basic Constraints: > CA:TRUE > Signature Algorithm: sha256WithRSAEncryption > 5f:fb:ca:7c:52:c6:81:f3:3f:48:02:c6:31:64:dc:a5:2f:a6: > 83:ef:b2:9d:68:7f:1a:e6:1d:8d:e0:50:08:ed:96:4f:56:8a: > eb:cd:3a:ac:74:f6:e4:68:55:0d:73:b3:bf:45:cd:35:e0:4b: > 70:bf:25:30:75:62:e1:5a:53:00:ff:ca:3c:c0:86:ad:44:73: > 5b:64:d8:85:ea:32:38:3b:4b:60:85:95:e5:10:f3:92:19:0e: > 55:67:26:c5:56:50:92:8c:d2:33:5b:5f:c6:27:c1:9a:6a:a5: > ad:1b:21:04:47:ce:94:f0:2a:38:26:43:5f:9b:c0:f5:33:80: > 59:72:33:e8:06:89:e1:7e:44:5c:cb:67:fa:f4:de:27:94:9c: > 44:1d:81:40:6f:46:bc:2f:93:89:30:48:99:bc:53:29:44:a7: > e1:9d:9c:05:98:14:3d:ab:e2:1c:3e:91:83:4a:74:9d:ed:14: > d4:86:2d:c0:4e:4e:20:fc:29:31:60:b9:63:50:23:52:b1:2f: > 9f:50:b4:d7:23:3e:08:c2:b3:bf:d3:b6:84:16:6d:71:fe:2a: > b0:71:f0:94:67:84:1d:45:8d:22:3d:4a:f4:65:73:0c:81:8a: > 4e:b7:52:b8:21:ab:ce:8d:50:e0:22:af:4f:2e:30:4f:95:8c: > 9c:26:0d:fe > Certificate: > Data: > Version: 1 (0x0) > Serial Number: > d1:ea:de:57:cb:4b:44:7b > Signature Algorithm: sha256WithRSAEncryption > Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com > Validity > Not Before: Oct 9 16:22:49 2019 GMT > Not After : Nov 8 16:22:49 2019 GMT > Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2 > Subject Public Key Info: > Public Key Algorithm: rsaEncryption > Public-Key: (2048 bit) > Modulus: > 00:d5:94:80:83:2c:fa:35:20:89:2c:b5:e8:f4:48: > 9a:f7:77:77:5c:9e:f4:cc:a7:f5:c7:d3:e8:29:0c: > 95:36:9f:ca:5c:dd:c0:d1:f5:14:14:eb:89:19:cc: > 8c:0e:35:d2:02:02:b5:56:d1:56:c4:f1:61:6c:cd: > ba:21:6d:27:e8:77:db:ec:12:c8:f0:c0:a9:97:2c: > 77:00:71:2d:36:b5:e7:08:10:d3:28:4b:96:ce:a1: > 4a:a6:ca:ca:9a:11:56:61:21:f2:2a:45:8c:02:a9: > 26:5d:46:d3:11:dd:35:b9:d8:19:26:cf:63:cf:d0: > f6:04:4f:07:24:27:e3:91:8b:9b:4b:01:61:ab:0a: > 4b:c9:c7:04:36:99:9b:94:e8:56:be:b8:14:08:73: > d4:f6:c1:0b:7f:10:20:bd:89:79:e5:9c:24:9f:03: > d2:b8:6b:20:b5:d1:dc:fe:ce:0d:8d:2b:a5:9b:a5: > 26:c4:85:90:4f:b9:e9:39:7b:c6:d7:9a:8a:e1:9f: > 47:93:db:dd:bc:d6:56:6c:16:10:b7:08:49:6b:38: > 7b:43:55:18:b3:e4:d4:69:94:df:51:21:7f:da:02: > 79:0f:da:65:b0:11:25:70:93:99:9d:a2:ab:8c:c9: > fb:ca:e2:7e:5f:b8:90:65:dd:fc:54:a7:ca:de:e3: > 6b:57 > Exponent: 65537 (0x10001) > Signature Algorithm: sha256WithRSAEncryption > 4d:e8:e4:17:02:74:af:a7:19:99:de:4b:44:e7:c1:7e:78:99: > b2:e3:8f:a6:59:67:c7:b7:df:83:b9:2a:aa:aa:01:65:2d:6f: > 0a:87:ec:49:31:01:9a:03:fd:66:36:37:cf:9a:d7:59:4f:ef: > a4:d8:c1:9d:55:b2:76:c4:1e:d7:98:3d:4e:12:25:1a:0a:51: > 40:e3:a0:45:3c:37:ad:f0:f8:c3:af:a1:52:47:97:77:ef:3f: > ad:d0:56:97:e0:83:c5:e9:f0:d5:e5:74:57:fe:b2:85:86:e7: > cd:e9:36:c0:54:cf:aa:35:4d:b6:81:42:6f:3a:9c:c4:5b:84: > 31:fc:3c:d6:42:1c:07:2d:62:59:ff:0b:f4:c4:56:e0:2c:bf: > 41:f4:63:40:05:c3:27:08:6c:a0:04:4c:c2:d2:8d:23:a0:30: > 7c:c2:58:92:ad:14:25:e3:39:3f:53:37:1d:3e:20:57:94:3f: > c9:64:d9:a3:11:d9:41:52:2e:28:00:49:72:0e:0c:96:89:02: > 1c:10:7e:cf:81:61:6c:54:97:40:f2:06:98:96:51:da:62:3a: > c6:10:fb:3a:4c:47:65:ea:e7:86:b2:bb:94:3d:de:93:ff:72: > b5:29:a7:53:ec:32:f5:dd:7d:09:18:70:89:58:b9:e9:41:8c: > ca:0e:2a:5b > Certificate: > Data: > Version: 1 (0x0) > Serial Number: > d1:ea:de:57:cb:4b:44:7a > Signature Algorithm: sha256WithRSAEncryption > Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com > Validity > Not Before: Oct 9 16:20:46 2019 GMT > Not After : Nov 8 16:20:46 2019 GMT > Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv4 cert, CN=192.168.124.2 > Subject Public Key Info: > Public Key Algorithm: rsaEncryption > Public-Key: (2048 bit) > Modulus: > 00:c8:e8:5a:b7:57:de:8c:5f:e9:71:8b:ca:4f:41: > 9c:6b:33:c7:d7:45:ab:41:6e:b0:77:00:bd:a6:39: > ab:e0:fb:23:05:e6:69:c4:56:9b:12:ec:46:fb:86: > 75:7e:11:66:c7:8f:f4:db:ce:e7:ce:70:56:26:3c: > e1:d9:81:6e:44:cd:56:90:9f:20:fd:ed:a7:be:ad: > 8a:7b:c3:98:3e:c9:ba:ec:eb:79:4d:a2:1c:93:89: > 89:0a:1a:4e:7b:31:c5:2f:cd:cc:90:d6:07:22:56: > 9c:e8:e9:51:65:18:e2:31:05:cd:49:52:54:8c:a1: > 1b:51:fa:8a:fa:66:9e:39:29:88:1a:e1:c5:31:ea: > 16:25:4a:95:47:4c:53:d1:20:dd:2c:29:32:e5:d8: > 0b:32:2d:52:c5:32:68:f7:7a:cf:15:d3:4f:95:03: > c9:0b:8b:46:b9:c8:e4:46:77:01:08:d7:ad:c3:0a: > 15:e2:70:62:a1:6f:d8:08:f0:60:9f:ee:34:f6:95: > 5c:f3:c2:e0:e2:18:ec:ff:4c:74:9e:92:5a:8f:8b: > be:38:f0:99:44:0f:95:34:a6:cb:f7:5d:59:72:30: > 58:f7:0c:3b:5a:23:d4:bc:26:71:47:7f:08:f0:fa: > 11:79:55:56:ed:bf:da:4f:df:55:d6:87:91:13:9e: > 54:8d > Exponent: 65537 (0x10001) > Signature Algorithm: sha256WithRSAEncryption > a2:f7:e8:7c:28:be:66:97:73:80:9a:38:4f:58:de:44:6f:10: > 97:b1:e6:d4:1b:00:e8:49:77:09:3f:b2:34:0c:cc:93:42:cc: > ff:7e:97:51:7e:ce:3d:f6:9f:e8:22:e5:d5:da:e5:db:f9:96: > 4c:07:11:0b:ff:57:07:98:89:df:27:9d:30:f5:e3:6d:19:d0: > b4:50:36:63:63:a9:9c:e1:a9:80:14:c7:07:9b:f0:d1:58:a8: > 00:3d:34:0c:7d:25:2f:a1:0c:4a:b8:98:61:9d:c8:8e:83:b0: > e2:b1:06:61:62:5d:67:28:32:f4:88:18:4a:13:52:55:ef:75: > 23:f2:6b:ce:d2:64:f7:07:c4:1f:50:6e:f4:1f:1f:d5:0f:fe: > fa:33:62:e5:af:0a:1d:19:fb:e0:73:82:82:ed:6e:5b:21:81: > 5e:62:b8:30:8a:20:f9:4f:a8:5b:f2:55:c2:68:92:ad:d8:b1: > ae:ea:a0:d5:39:60:fd:f2:5f:23:62:43:de:2e:b6:3e:11:ac: > 12:7b:94:e3:b9:84:6c:2f:8e:04:8a:2b:af:5a:08:f4:7b:dc: > 61:b4:9a:19:00:d5:fb:14:33:52:4c:49:c2:18:cd:73:8e:e8: > d1:45:9b:ba:57:6b:5a:db:31:d7:70:13:d5:a8:26:5c:77:88: > ba:be:09:d3 Futhermore, my Apache config is, for the "proper" certificate assignment case (excerpt from "/etc/httpd/conf.d/ssl.conf"): > <VirtualHost [fd33:eb1b:9b36::2]:443> > SSLProtocol -all +TLSv1 +TLSv1.1 +TLSv1.2 > SSLCipherSuite HIGH:3DES:!aNULL:!MD5:!SEED:!IDEA > SSLCertificateFile /etc/pki/tls/certs/fd33:eb1b:9b36::2.signedcert.pem > SSLCertificateKeyFile /etc/pki/tls/private/fd33:eb1b:9b36::2.key > <VirtualHost _default_:443> > SSLProtocol -all +TLSv1 +TLSv1.1 +TLSv1.2 > SSLCipherSuite HIGH:3DES:!aNULL:!MD5:!SEED:!IDEA > SSLCertificateFile /etc/pki/tls/certs/192.168.124.2.signedcert.pem > SSLCertificateKeyFile /etc/pki/tls/private/192.168.124.2.key Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48709): https://edk2.groups.io/g/devel/message/48709 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: > > Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2 Yeah, you're not actually testing the case I'm talking about. You want a GEN_IP in the x509v3 Subject Alternative Name. Compare with... $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl x509 -noout -text | grep -A1 Alternative X509v3 Subject Alternative Name: DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP Address:134.191.232.101 $ curl https://134.191.232.101/ -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48738): https://edk2.groups.io/g/devel/message/48738 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/10/19 17:45, David Woodhouse wrote: > On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: >>> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2 > > Yeah, you're not actually testing the case I'm talking about. You want > a GEN_IP in the x509v3 Subject Alternative Name. > > Compare with... > > $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl x509 -noout -text | grep -A1 Alternative > X509v3 Subject Alternative Name: > DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP Address:134.191.232.101 > > $ curl https://134.191.232.101/ > OK, thank you. I can imagine two failure modes, with the patches applied. (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server certificate. (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid (mismatched) server certificate. Can we tell which failure mode applies? (I can't test it easily myself, as I don't even know how to create a server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) Case (2) is clearly bad, and it would be a sign that the patch series does not fully fix the issue. Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is pretty rare in practice. Thus regressing it (perhaps temporarily) should be an acceptable trade-off for fixing the current gaping hole (= subject name not checked at all). Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48751): https://edk2.groups.io/g/devel/message/48751 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo & David, I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting). IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc(). Post my explain again: > UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), > not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: > "...TLS session hostname for validation which is used to verify whether the name > within the peer certificate matches a given host name..." > In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real > FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), > and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. > That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature > by following the standard TLS HostName verification capability. Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here. Thanks, Jiaxin > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Friday, October 11, 2019 2:04 AM > To: David Woodhouse <dwmw2@infradead.org>; Wu, Jiaxin > <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J > <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com> > Cc: Richard Levitte <levitte@openssl.org> > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On 10/10/19 17:45, David Woodhouse wrote: > > On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: > >>> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, > OU=IPv6 cert, CN=fd33:eb1b:9b36::2 > > > > Yeah, you're not actually testing the case I'm talking about. You want > > a GEN_IP in the x509v3 Subject Alternative Name. > > > > Compare with... > > > > $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl > x509 -noout -text | grep -A1 Alternative > > X509v3 Subject Alternative Name: > > DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP > Address:134.191.232.101 > > > > $ curl https://134.191.232.101/ > > > > OK, thank you. > > I can imagine two failure modes, with the patches applied. > > (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server > certificate. > > (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid > (mismatched) server certificate. > > Can we tell which failure mode applies? > > (I can't test it easily myself, as I don't even know how to create a > server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) > > Case (2) is clearly bad, and it would be a sign that the patch series > does not fully fix the issue. > > Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is > pretty rare in practice. Thus regressing it (perhaps temporarily) should > be an acceptable trade-off for fixing the current gaping hole (= subject > name not checked at all). > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48777): https://edk2.groups.io/g/devel/message/48777 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, 2019-10-11 at 02:24 +0000, Wu, Jiaxin wrote: > Hi Laszlo & David, > > I think I have *repeated* several times that we are targeting to fix > the HostName validation issue, not the IP or email address. *But* > even so, the series patches for UEFI TLS is also allowable to > specify IP as host name for CN or dNSName of SAN in the certificate. > That's why I said "if the CN or SAN in the certificate are set > correctly, it should be OK to pass the verification". The failure you > mentioned here is to set the IP in iPAddress of SAN, I agree it's the > routine and suggested setting, *but* obviously, it's not the target > we are supported according the implementation/description of > TlsSetVerifyHost. We are targeting to the hostname verification, and > meanwhile compatible with the IP in the URI (But need the *correct* > certificate setting). > > IP addresses stored in the DNS names and CN are of cause ignored by > X509_check_ip & X509_check_ip_asc(). I cannot coherently express how disappointed I am by this response. The current state is that EDK2 doesn't check the subject of the certificate at all. We're trying to fix that, and you have expended more effort typing in poor excuses for doing an incomplete job, than the typing it would have taken just to get it right in the first place. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48800): https://edk2.groups.io/g/devel/message/48800 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
I'm surprising my detailed and patient explanation become a poor excuses! If you think there is anything wrong with my explanation, please correct me instead of blaming directly. > > I think I have *repeated* several times that we are targeting to fix > > the HostName validation issue, not the IP or email address. *But* > > even so, the series patches for UEFI TLS is also allowable to > > specify IP as host name for CN or dNSName of SAN in the certificate. > > That's why I said "if the CN or SAN in the certificate are set > > correctly, it should be OK to pass the verification". The failure you > > mentioned here is to set the IP in iPAddress of SAN, I agree it's the > > routine and suggested setting, *but* obviously, it's not the target > > we are supported according the implementation/description of > > TlsSetVerifyHost. We are targeting to the hostname verification, and > > meanwhile compatible with the IP in the URI (But need the *correct* > > certificate setting). > > > > IP addresses stored in the DNS names and CN are of cause ignored by > > X509_check_ip & X509_check_ip_asc(). > > I cannot coherently express how disappointed I am by this response. > > The current state is that EDK2 doesn't check the subject of the > certificate at all. Highlight again: we do check the certificate peername in SAN & Subject CommonName (CN) instead of nothing. > > We're trying to fix that, and you have expended more effort typing in > poor excuses for doing an incomplete job, than the typing it would have > taken just to get it right in the first place. My typing is only poor excuses? I'm trying my best to explain the patch intention. I said in the previous email, "We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI". I also agree your suggestion & requires is reasonable & meaning to support the IP check in the certificate. So, my friendly advice is to separate the issues you raised instead of mixing them up. Thanks, Jiaxin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48803): https://edk2.groups.io/g/devel/message/48803 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/11/19 04:24, Wu, Jiaxin wrote: > Hi Laszlo & David, > > I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting). > > IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc(). > > Post my explain again: >> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), >> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: >> "...TLS session hostname for validation which is used to verify whether the name >> within the peer certificate matches a given host name..." >> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real >> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), >> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. >> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature >> by following the standard TLS HostName verification capability. > > Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here. (To clarify my stance.) Given the above statement of scope, and given the test env details that I included in my previous message: series Tested-by: Laszlo Ersek <lersek@redhat.com> I understand that my testing does not cover the use case described by David. So my Tested-by is certainly *not* intended as the "last word" in this thread, or anything like that. I'm only saying this patch set is good enough for me, not that everyone should find it good enough for them. Thanks Laszlo >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Friday, October 11, 2019 2:04 AM >> To: David Woodhouse <dwmw2@infradead.org>; Wu, Jiaxin >> <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J >> <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com> >> Cc: Richard Levitte <levitte@openssl.org> >> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName >> validation feature(CVE-2019-14553) >> >> On 10/10/19 17:45, David Woodhouse wrote: >>> On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: >>>>> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, >> OU=IPv6 cert, CN=fd33:eb1b:9b36::2 >>> >>> Yeah, you're not actually testing the case I'm talking about. You want >>> a GEN_IP in the x509v3 Subject Alternative Name. >>> >>> Compare with... >>> >>> $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl >> x509 -noout -text | grep -A1 Alternative >>> X509v3 Subject Alternative Name: >>> DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP >> Address:134.191.232.101 >>> >>> $ curl https://134.191.232.101/ >>> >> >> OK, thank you. >> >> I can imagine two failure modes, with the patches applied. >> >> (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server >> certificate. >> >> (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid >> (mismatched) server certificate. >> >> Can we tell which failure mode applies? >> >> (I can't test it easily myself, as I don't even know how to create a >> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) >> >> Case (2) is clearly bad, and it would be a sign that the patch series >> does not fully fix the issue. >> >> Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is >> pretty rare in practice. Thus regressing it (perhaps temporarily) should >> be an acceptable trade-off for fixing the current gaping hole (= subject >> name not checked at all). >> >> Thanks >> Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48813): https://edk2.groups.io/g/devel/message/48813 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote: > On 10/11/19 04:24, Wu, Jiaxin wrote: > > Hi Laszlo & David, > > > > I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting). > > > > IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc(). > > > > Post my explain again: > > > UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), > > > not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: > > > "...TLS session hostname for validation which is used to verify whether the name > > > within the peer certificate matches a given host name..." > > > In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real > > > FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), > > > and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. > > > That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature > > > by following the standard TLS HostName verification capability. > > > > Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here. > > (To clarify my stance.) > > Given the above statement of scope, and given the test env details that > I included in my previous message: > > series > Tested-by: Laszlo Ersek <lersek@redhat.com> > > I understand that my testing does not cover the use case described by David. > > So my Tested-by is certainly *not* intended as the "last word" in this > thread, or anything like that. I'm only saying this patch set is good > enough for me, not that everyone should find it good enough for them. As you wish. Note for the record that that this is a functional regression. We go from accepting all certs regardless of the target/subject, to rejecting some valid certificates. I first started looking at this when it was reported as such, on the list. Given the length of time it's taken to fix this CVE already, I understand that Laszlo is keen to get *something* committed, at last. But I stand by the assertion that this could be done properly, without the regression, with much less additional typing even than we've already done in this thread the last couple of days. It just isn't that hard. Ultimately, though, do whatever you like. I am not the final arbiter of engineering sanity and common sense for the EDK2 project. If I were, the world would be a very different place. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48820): https://edk2.groups.io/g/devel/message/48820 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/11/19 13:16, David Woodhouse wrote: > On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote: >> On 10/11/19 04:24, Wu, Jiaxin wrote: >>> Hi Laszlo & David, >>> >>> I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so, the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting). >>> >>> IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc(). >>> >>> Post my explain again: >>>> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), >>>> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: >>>> "...TLS session hostname for validation which is used to verify whether the name >>>> within the peer certificate matches a given host name..." >>>> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real >>>> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), >>>> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. >>>> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature >>>> by following the standard TLS HostName verification capability. >>> >>> Now, let's talking the iPAddress setting in SAN (same as email address), if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here. >> >> (To clarify my stance.) >> >> Given the above statement of scope, and given the test env details that >> I included in my previous message: >> >> series >> Tested-by: Laszlo Ersek <lersek@redhat.com> >> >> I understand that my testing does not cover the use case described by David. >> >> So my Tested-by is certainly *not* intended as the "last word" in this >> thread, or anything like that. I'm only saying this patch set is good >> enough for me, not that everyone should find it good enough for them. > > > As you wish. > > Note for the record that that this is a functional regression. We go > from accepting all certs regardless of the target/subject, to rejecting > some valid certificates. Indeed that is the case; I'm aware of it. Thanks for pointing it out. This functional regression does not translate to an actual end-user regression, under my circumstances. From my perspective, UEFI HTTPS boot is a feature that cannot be enabled *at all* in a production environment, due to the possible MITM. In other words, I cannot build OVMF with -D NETWORK_HTTP_BOOT_ENABLE -D NETWORK_TLS_ENABLE and give the binary to users with a clear conscience. They'd see UEFI HTTPS boot, say "hey this is secure!", and use it. That would be a false sense of security. With the patches applied, yes, some valid certificates would be rejected. That would be a normal bug report, or a known limitation, or even a "layered" feature request. Not a security bug. It would not prevent the basic -- or let's say, "somewhat restricted" -- enablement of UEFI HTTPS boot. Not a deal-breaker. It would be possible for us to announce, "and now we're giving you UEFI HTTPS boot that is secure to the best of our knowledge, only it does not *yet* support GEN_IP in SAN. Stay tuned for that." This would not be a regression, from an end-user perspective. Again, I'm not attempting to "override" or "suppress" your (implied) NACK. If the agreement turns out to be that we should *first* add support for GEN_IP in the SAN, I will not campaign for merging the v1 patches *upstream*, right now. I respect your feedback. No patch should be merged while there are outstanding questions, let alone well-founded NACKs. > I first started looking at this when it was > reported as such, on the list. I believe you. Can you somehow find that thread? I tried, but I couldn't find it. My mailbox (going back 9 years) is indexed, but my searches have failed. I must be using the wrong search terms. If I try "GEN_IP" or "Subject Alternative Name", I only get this thread. > Given the length of time it's taken to fix this CVE already, I > understand that Laszlo is keen to get *something* committed, at last. More precisely, I'd be happy to see patches committed that plug the security hole. Which has been *completely* preventing us from enabling this feature. I agree that, while working towards a feature or a bugfix, already functional things should *not* be regressed -- even temporarily, even if the regression lasts only from patch#n to patch#(n+1) in a single series. This is a very strong guideline, minimally for bisectability. But to me, this feature has never existed in practice. I found and reported the security issue (BZ#960) while working on the downstream enablement of UEFI HTTPS boot. The problem caused me to postpone the downstream enablement. So for me there's nothing to regress. Similarly, knowledge of this issue prevented me from porting NETWORK_TLS_ENABLE from OvmfPkg to ArmVirtPkg, up-stream: https://bugzilla.tianocore.org/show_bug.cgi?id=1007 https://bugzilla.tianocore.org/show_bug.cgi?id=1009 For a long time, I couldn't publicly tell the truth about splitting BZ#1009 off of BZ#1007. My *real* incentive was to save at least ArmVirtPkg users from exposure. I called Ard on the phone, and we agreed to postpone TLS in ArmVirPkg. (I couldn't do the same for OVMF users, as I would have had to back out TLS settings from OvmfPkg, and that would have drawn stares.) I went on to intentionally ignore BZ#1009, hoping no-one would ask me about it. Then Guillaume did. See my answer here: <https://bugzilla.tianocore.org/show_bug.cgi?id=1009#c4>. Therefore, UEFI HTTPS boot has not existed for me, in practice, even in the *upstream* project, to the extent that I was able to block the "spreading" of the feature. > But I stand by the assertion that this could be done properly, without > the regression, with much less additional typing even than we've > already done in this thread the last couple of days. It just isn't that > hard. David: it *is* hard! It is hard for me. I wouldn't know where to begin. I assume it may be hard for Jiaxin too. If it's not hard for you, can you please post the patches? Feel free to pick up this series as a starting point, rework it, and then post an updated variant. If it takes two more weeks and you can solve the problem to everyone's satisfaction (including your own!), I'll be overjoyed. I'll be happy to wait! If it takes four more months... then I might apply the present patchset down-stream only. As always, I strongly favor "upstream first". Show us the code, please? > Ultimately, though, do whatever you like. I am not the final arbiter of > engineering sanity and common sense for the EDK2 project. No well-founded NACK or pending question should be ignored in patch review. > > If I were, the world would be a very different place. No doubt! ;) Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48840): https://edk2.groups.io/g/devel/message/48840 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Fri, 2019-10-11 at 17:36 +0200, Laszlo Ersek wrote: > On 10/11/19 13:16, David Woodhouse wrote: > > I first started looking at this when it was > > reported as such, on the list. > > I believe you. Can you somehow find that thread? I tried, but I couldn't > find it. My mailbox (going back 9 years) is indexed, but my searches > have failed. I must be using the wrong search terms. If I try "GEN_IP" > or "Subject Alternative Name", I only get this thread. https://www.mail-archive.com/devel@edk2.groups.io/msg03339.html In that thread you pointed me at the bug, and I immediately pointed out the error in the patch series: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 Followed by a bit more detail on how to fix it, with examples to look at: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > David: it *is* hard! It is hard for me. I wouldn't know where to begin. I suspect this is false modesty on your part. Given the pointers and the examples above, I have lots of confidence that if this were the task on your plate, you would accomplish it with ease. I would, of course, be happy to provide further pointers, and even work with upstream OpenSSL to make this even easier. Crypto libraries should make it hard for application developers to get things wrong, and they often let us down in that respect. In fact, I did that last bit already: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c33 > As always, I strongly favor "upstream first". Show us the code, please? It's already linked from that Bugzilla comment I referenced: https://github.com/openssl/openssl/pull/9201 Pull that into your OpenSSL tree, then make a trivial change following the example in that PR, to do if (SSL_set1_ip_asc(ssl, hostname) < 0) SSL_set1_host(ssl, hostname); instead of just the SSL_set1_host() call. That way, *if* the string happens to be a valid IPv6 or Legacy IP address, the SSL_set1_ip_asc() call will work; otherwise it's treated as a hostname. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48843): https://edk2.groups.io/g/devel/message/48843 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/11/19 18:01, David Woodhouse wrote: > On Fri, 2019-10-11 at 17:36 +0200, Laszlo Ersek wrote: >> On 10/11/19 13:16, David Woodhouse wrote: >>> I first started looking at this when it was >>> reported as such, on the list. >> >> I believe you. Can you somehow find that thread? I tried, but I couldn't >> find it. My mailbox (going back 9 years) is indexed, but my searches >> have failed. I must be using the wrong search terms. If I try "GEN_IP" >> or "Subject Alternative Name", I only get this thread. > > https://www.mail-archive.com/devel@edk2.groups.io/msg03339.html Thank you. IIUC, Sivaraman attempted to integrate the patches from TianoCore#960, and then they ran into the SAN/GEN_IP regression *in practice*. Unfortunately, in retrospect, that has been a *complete* communication fiasco. - Sivaraman tested the patches from TianoCore#960, but he did not report his findings (the regression) in the Bugzilla entry. - Conversely, the discussion on the list was missed by Jiaxin. - I participated in both places, and I failed to realize we were talking about any kind of regression. (To be honest, even if I re-read the initial message from Sivaraman, I still find it very difficult to understand: When we create certificates with CN as Host Name and SAN as IP TLS Handshake works only for Host Name and it provides Handshake Error when the request are IP Based. Even a *modicum* of punctuation would have helped with that *soup* of acronyms. Such as: When we create certificates with CN as Host Name, and SAN as IP, TLS Handshake works only for Host Name, and it provides Handshake Error when the request are IP Based. "SAN as IP" is the expression with 90% of the information content, and it received a fraction of the space / focus. Not to mention, back then I had zero idea about "Subject Alternative Name" and "GEN_IP".) > In that thread you pointed me at the bug, and I immediately pointed out > the error in the patch series: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 That's correct. The word "regression" was not uttered however. From the rest of the discussion in the BZ, I gather that Jiaxin never treated the problem as a regression. If this were not a practical regression, I'd agree with Jiaxin that we should consider the SAN/GEN_IP use case on top of the present patch set, optionally, separately, incrementally. But, with evidence that AMI reported their practical regression back in June -- even though they failed to (a) describe their use case clearly, (b) state that it was a regression, (c) make the statement in the same place where the patches existed --, I'm now inclined to reverse my position. *Unless* Jiaxin can show that what AMI is trying to do is outside of the UEFI spec (v2.8), I agree we should fix the SAN/GEN_IP case in *this* series. (The spec writes, EfiTlsVerifyHost -- TLS session hostname for validation which is used to verify whether the name within the peer certificate matches a given host name. [...] The "given host name" expression (EFI_TLS_VERIFY_HOST.HostName) seems to map to "URL used in request". The question is whether the "name within the peer certificate" part includes GEN_IP in the Subject Alternative Name. If it does, then this series is not complete.) > Followed by a bit more detail on how to fix it, with examples to look > at: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > >> David: it *is* hard! It is hard for me. I wouldn't know where to begin. > > I suspect this is false modesty on your part. Nope. > Given the pointers and > the examples above, I have lots of confidence that if this were the > task on your plate, you would accomplish it with ease. Definitely not with ease. > > I would, of course, be happy to provide further pointers, and even work > with upstream OpenSSL to make this even easier. Crypto libraries should > make it hard for application developers to get things wrong, and they > often let us down in that respect. > > In fact, I did that last bit already: > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c33 > >> As always, I strongly favor "upstream first". Show us the code, please? > > It's already linked from that Bugzilla comment I referenced: > https://github.com/openssl/openssl/pull/9201 > > Pull that into your OpenSSL tree, then make a trivial change following > the example in that PR, to do > > if (SSL_set1_ip_asc(ssl, hostname) < 0) > SSL_set1_host(ssl, hostname); > > instead of just the SSL_set1_host() call. > > That way, *if* the string happens to be a valid IPv6 or Legacy IP > address, the SSL_set1_ip_asc() call will work; otherwise it's treated > as a hostname. My understanding is that a fix purely in edk2 -- that is, without advancing our openssl submodule reference at once -- is possible, based on your comment https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). The last argument of that call is currently NULL. We should change that, to a callback function that implements what ssl_app_verify_callback() and match_cert_hostname() do, in your source file http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c There seems to be a GEN_* switch inside a loop in there. Jiaxin: I think David has a point. AMI's use case appears covered by the UEFI-2.8 spec, to me anyway (though I'm not an OpenSSL expert!!!), and they've encountered a practical regression with this patch set. Do you think you can implement the verify callback outlined by David above, or do you prefer if we wait until we can consume David's OpenSSL fix through the next openssl submodule rebase (and then the edk2 change is minimal)? David: another way to prevent the regression is to commit the current patches, but disable them with a BOOLEAN PCD, by default. (This need not be a feature PCD; it could even be dynamic.) Then platforms accepting the SAN/GEN_IP regression temporarily could enable the PCD. This solution would permit a separate (follow-up) series for the SAN/GEN_IP case. We could file a reminder BZ now, and implement the "easy" solution when we next rebase the openssl submodule. Would that be tolerable? Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48913): https://edk2.groups.io/g/devel/message/48913 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/14/19 18:15, Laszlo Ersek wrote: > David: another way to prevent the regression is to commit the current > patches, but disable them with a BOOLEAN PCD, by default. (This need not > be a feature PCD; it could even be dynamic.) Then platforms accepting > the SAN/GEN_IP regression temporarily could enable the PCD. This > solution would permit a separate (follow-up) series for the SAN/GEN_IP > case. We could file a reminder BZ now, and implement the "easy" solution > when we next rebase the openssl submodule. Would that be tolerable? ... to clarify, in this case, the upstream edk2 project should *not* claim to have fixed CVE-2019-14553, until the reminder BZ is also closed! The new BZ should actually block TianoCore#960. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48914): https://edk2.groups.io/g/devel/message/48914 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote: > My understanding is that a fix purely in edk2 -- that is, without > advancing our openssl submodule reference at once Haha, I love the fact that I am hoist by my own petard on patching OpenSSL. I evidently did such a good job of upstreaming all the quirks we need for EDK2, that we're now *incapable* of carrying any local patches to OpenSSL. I'll take that as a win, I suppose :) > -- is possible, based > on your comment > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", > 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). > The last argument of that call is currently NULL. > > We should change that, to a callback function that implements what > ssl_app_verify_callback() and match_cert_hostname() do, in your source file > > http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c > > There seems to be a GEN_* switch inside a loop in there. That's harder than it needs to be; it's the version for OpenSSL < 1.0.2 where they made the users jump through *lots* of hoops to validate certs correctly. These days it's much easier; you only need the version at http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1369 which is called from the actual callback at http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1507 I'll see if I can throw something together for you at least as an example. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48916): https://edk2.groups.io/g/devel/message/48916 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote: > My understanding is that a fix purely in edk2 -- that is, without > advancing our openssl submodule reference at once -- is possible, based > on your comment > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", > 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). > The last argument of that call is currently NULL. > > We should change that, to a callback function that implements what > ssl_app_verify_callback() and match_cert_hostname() do, in your source file > > http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c Hm, you are lost in a twisty maze of verify callbacks, all alike. Actually the one you can set with SSL_set_verify() isn't the one you want. That's a low-level one, called from within the generic X509_verify_cert() function. The "app callback" in my OpenConnect example is set on the SSL_CTX not the SSL object, and is called from the top-level ssl_verify_cert_chain() function *instead* of X509_verify_cert(). It is X509_verify_cert() which can do the hostname/IP checks for us, if we can only tell it that we want it to. But the X509_VERIFY_PARAM object is private to the SSL. As discussed, we have the SSL_set1_host() accessor function which lets us set the hostname. The implementation really is a simple one-liner, calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way for use to set the IP address from the outside, without an equivalent accessor function for that (and without SSL_set1_host() spotting that the string it's given is an IP address, and doing so). But what we can do is stash the target string in some ex_data hanging off the SSL object, then have an app callback — which *can* reach the underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the normal X509_verify_cert() function that it has overridden. Something like this... and instead of calling SSL_set1_host(ssl, host) your own code now has to call SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c index f9ad6f6b946c..add5810cc4bd 100644 --- a/CryptoPkg/Library/TlsLib/TlsInit.c +++ b/CryptoPkg/Library/TlsLib/TlsInit.c @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include "InternalTlsLib.h" +/* You are lost in a twisty maze of SSL cert verify callbacks, all + * alike. All we really wanted to do was call SSL_set1_host() and + * have it work for IP addresses too, which OpenSSL PR#9201 will do + * for us. But until we update OpenSSL, that doesn't work. And we + * can't get at the underlying X509_VERIFY_PARAM to set the IP address + * for ourselves. + * + * So we install an app_verify_callback in the SSL_CTX (which is + * different to the per-SSL callback wae can use, because it happens + * sooner. All our callback does it set the hostname or IP address in + * the X509_VERIFY_PARAM like we wanted to in the first place, and + * then call X509_verify_param() which is the default function. + * + * How does it find the hostname/IP string? It's attached to the SSL + * as ex_data, using this index: + */ +static int ssl_target_idx; + +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, + int idx, long argl, void *argp) +{ + /* Free it */ +} + +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, + void *from_d, int idx, long argl, void *argp) +{ + /* strdup it */ + return 0; +} + +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) +{ + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx); + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx); + + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); + + return X509_verify_cert(ctx); +} + /** Initializes the OpenSSL library. @@ -40,6 +83,9 @@ TlsInitialize ( return FALSE; } + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL, + ssl_target_dup, ssl_target_free); + // // Initialize the pseudorandom number generator. // @@ -106,6 +152,10 @@ TlsCtxNew ( // SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which + * we could have done as SSL_set_verify(). Twisty maze, remember? */ + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL); + return (VOID *) TlsCtx; } -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49002): https://edk2.groups.io/g/devel/message/49002 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2019-10-15 at 12:03 +0100, David Woodhouse wrote: > > Something like this... and instead of calling SSL_set1_host(ssl, host) > your own code now has to call > SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); Here's how I tested that in the OpenSSL tree in userspace, FWIW... diff --git a/demos/bio/sconnect.c b/demos/bio/sconnect.c index 7e46bf0ad8..1fd2829a79 100644 --- a/demos/bio/sconnect.c +++ b/demos/bio/sconnect.c @@ -25,11 +25,40 @@ #define HOSTPORT "localhost:4433" #define CAFILE "root.pem" +static int ssl_target_idx; + +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, + int idx, long argl, void *argp) +{ + printf("Freeing %s\n", ptr); + free(ptr); +} + +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, + void *from_d, int idx, long argl, void *argp) +{ + /* strdup it */ + return 0; +} + +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) +{ + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx); + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx); + + printf("Setting %s\n", hostname); + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); + + return X509_verify_cert(ctx); +} + int main(int argc, char *argv[]) { const char *hostport = HOSTPORT; const char *CAfile = CAFILE; - char *hostname; + const char *hostname; char *cp; BIO *out = NULL; char buf[1024 * 10], *p; @@ -43,10 +72,6 @@ int main(int argc, char *argv[]) if (argc > 2) CAfile = argv[2]; - hostname = OPENSSL_strdup(hostport); - if ((cp = strchr(hostname, ':')) != NULL) - *cp = 0; - #ifdef WATT32 dbug_init(); sock_init(); @@ -54,6 +79,8 @@ int main(int argc, char *argv[]) ssl_ctx = SSL_CTX_new(TLS_client_method()); + SSL_CTX_set_cert_verify_callback(ssl_ctx, app_verify_callback, NULL); + /* Enable trust chain verification */ SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, NULL); SSL_CTX_load_verify_locations(ssl_ctx, CAfile, NULL); @@ -62,9 +89,6 @@ int main(int argc, char *argv[]) ssl = SSL_new(ssl_ctx); SSL_set_connect_state(ssl); - /* Enable peername verification */ - if (SSL_set1_host(ssl, hostname) <= 0) - goto err; /* Use it inside an SSL BIO */ ssl_bio = BIO_new(BIO_f_ssl()); @@ -73,6 +97,13 @@ int main(int argc, char *argv[]) /* Lets use a connect BIO under the SSL BIO */ out = BIO_new(BIO_s_connect()); BIO_set_conn_hostname(out, hostport); + + /* The BIO has parsed the host:port and even IPv6 literals in [] */ + hostname = BIO_get_conn_hostname(out); + + if (hostname) + SSL_set_ex_data(ssl, ssl_target_idx, strdup(hostname)); + BIO_set_nbio(out, 1); out = BIO_push(ssl_bio, out); -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49004): https://edk2.groups.io/g/devel/message/49004 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/15/19 13:03, David Woodhouse wrote: > On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote: >> My understanding is that a fix purely in edk2 -- that is, without >> advancing our openssl submodule reference at once -- is possible, based >> on your comment >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 >> >> Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", >> 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). >> The last argument of that call is currently NULL. >> >> We should change that, to a callback function that implements what >> ssl_app_verify_callback() and match_cert_hostname() do, in your source file >> >> http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c > > > Hm, you are lost in a twisty maze of verify callbacks, all alike. Definitely. > Actually the one you can set with SSL_set_verify() isn't the one you > want. That's a low-level one, called from within the generic > X509_verify_cert() function. Well, above I referred to SSL_set_verify() only because you had named that function in <https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32>: > Note: the callback you're after is the final argument to > SSL_set_verify(), which you are calling from TlsSetVerify() with a > NULL argument. You need to point it at your own function of type > SSL_verify_cb which does the checking correctly. Back to the email: On 10/15/19 13:03, David Woodhouse wrote: > The "app callback" in my OpenConnect example is set on the SSL_CTX not > the SSL object, and is called from the top-level > ssl_verify_cert_chain() function *instead* of X509_verify_cert(). > > It is X509_verify_cert() which can do the hostname/IP checks for us, if > we can only tell it that we want it to. But the X509_VERIFY_PARAM > object is private to the SSL. > > As discussed, we have the SSL_set1_host() accessor function which lets > us set the hostname. The implementation really is a simple one-liner, > calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way > for use to set the IP address from the outside, without an equivalent > accessor function for that (and without SSL_set1_host() spotting that > the string it's given is an IP address, and doing so). > > But what we can do is stash the target string in some ex_data hanging > off the SSL object, then have an app callback — which *can* reach the > underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or > X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the > normal X509_verify_cert() function that it has overridden. > > Something like this... and instead of calling SSL_set1_host(ssl, host) > your own code now has to call > SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); > > diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c > index f9ad6f6b946c..add5810cc4bd 100644 > --- a/CryptoPkg/Library/TlsLib/TlsInit.c > +++ b/CryptoPkg/Library/TlsLib/TlsInit.c > @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include "InternalTlsLib.h" > > +/* You are lost in a twisty maze of SSL cert verify callbacks, all > + * alike. All we really wanted to do was call SSL_set1_host() and > + * have it work for IP addresses too, which OpenSSL PR#9201 will do > + * for us. But until we update OpenSSL, that doesn't work. And we > + * can't get at the underlying X509_VERIFY_PARAM to set the IP address > + * for ourselves. > + * > + * So we install an app_verify_callback in the SSL_CTX (which is > + * different to the per-SSL callback wae can use, because it happens > + * sooner. All our callback does it set the hostname or IP address in > + * the X509_VERIFY_PARAM like we wanted to in the first place, and > + * then call X509_verify_param() which is the default function. > + * > + * How does it find the hostname/IP string? It's attached to the SSL > + * as ex_data, using this index: > + */ > +static int ssl_target_idx; > + > +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, > + int idx, long argl, void *argp) > +{ > + /* Free it */ > +} > + > +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, > + void *from_d, int idx, long argl, void *argp) > +{ > + /* strdup it */ > + return 0; > +} > + > +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) > +{ > + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); > + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx); > + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx); > + > + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) > + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); > + > + return X509_verify_cert(ctx); > +} > + > /** > Initializes the OpenSSL library. > > @@ -40,6 +83,9 @@ TlsInitialize ( > return FALSE; > } > > + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL, > + ssl_target_dup, ssl_target_free); > + > // > // Initialize the pseudorandom number generator. > // > @@ -106,6 +152,10 @@ TlsCtxNew ( > // > SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); > > + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which > + * we could have done as SSL_set_verify(). Twisty maze, remember? */ > + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL); > + > return (VOID *) TlsCtx; > } Thank you very much for this code. To me it seems like this patch should be squashed into patch#2 (which modifies TlsLib), after adapting it to the edk2 coding style. I have some comments / questions. (1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be replaced with SSL_set_ex_data(). OK. In case the client does not use the TlsSetVerifyHost() function, SSL_set_ex_data() would not be called. However, app_verify_callback() would still be called. Is my understanding correct that "hostname" (from SSL_get_ex_data()) would be NULL in that case, and therefore both X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would be omitted? (I mean this looks like the correct behavior to me; just asking if that is indeed the intent of the code.) (2) The documentation of the APIs seems a bit strange. I've found the following "directory" web pages: https://www.openssl.org/docs/man1.0.2/man3/ https://www.openssl.org/docs/man1.1.0/man3/ https://www.openssl.org/docs/man1.1.1/man3/ edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only look at the last page. However, SSL_get_ex_new_index() is only documented in the first page. Is my understanding correct that each OpenSSL API should be looked up in the latest documentation directory that appears to describe it? In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1 manual, and fail to find it, does that mean that the API is deprecated, or only that I should look at an earlier release of the manual? (3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around CRYPTO_get_ex_new_index(). https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html Based on that, I think the second ("argp") parameter of our SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of the "free" and "dup" functions are unused. Do you agree? (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has already been stored for the same index? Do we have to first fetch it with SSL_get_ex_data() and free it, or will it be automatically freed with "free_func"? (Note: I think that, if we used a "new_func" for allocating anything, this question could be relevant the very first time SSL_set_ex_data() were called.) (5) The most confusing part... If I look at the 1.0.2 documentation, it says about "dup_func": > dup_func() is called when a structure is being copied. [...] The > from_d parameter is passed a pointer to the source application data > when the function is called, when the function returns the value is > copied to the destination: the application can thus modify the data > pointed to by from_d and have different values in the source and > destination This makes no sense: I wouldn't want to modify the source application data in-place, just to give the new parent object a modified instance of it! Also, how would OpenSSL know how many bytes to copy? So I looked at the code, and it turns out dup_func is called with a (void**), not a (void*): if (!storage[i]->dup_func(to, from, &ptr, i, storage[i]->argl, storage[i]->argp)) The 1.1.1 documentation is more accurate; it says: https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html > The from_d parameter needs to be cast to a void **pptr as the API has > currently the wrong signature; that will be changed in a future > version. The *pptr is a pointer to the source exdata. When the > dup_func() returns, the value in *pptr is copied to the destination > ex_data. If the pointer contained in *pptr is not modified by the > dup_func(), then both to and from will point to the same data So let me summarize the documentation status: - SSL_get_ex_new_index is not documented in 1.1.1, - SSL_get_ex_new_index is not documented in 1.1.0, - SSL_get_ex_new_index has a "shim" documentation in 1.0.2, and it refers the reader to RSA_get_ex_new_index(), - RSA_get_ex_new_index is documented in 1.0.2 *incorrectly* (with regard to dup_func), - the valid documentation is in 1.1.1, under CRYPTO_get_ex_new_index -- but for learning about that function, I had to look at the library source code. When you suggested the IP checking would be easy to imlement, you were joking, right? :) (6) Given that we are introducing callbacks (from CryptoPkg/Library/OpensslLib to CryptoPkg/Library/TlsLib), and OpenSSL does not declare these function prototypes with EFIAPI, the callbacks too must be defined with "native" (not EFIAPI) calling convention. Correct? Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49010): https://edk2.groups.io/g/devel/message/49010 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2019-10-15 at 15:54 +0200, Laszlo Ersek wrote: > On 10/15/19 13:03, David Woodhouse wrote: > > On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote: > > > My understanding is that a fix purely in edk2 -- that is, without > > > advancing our openssl submodule reference at once -- is possible, based > > > on your comment > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > > > > > Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", > > > 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()). > > > The last argument of that call is currently NULL. > > > > > > We should change that, to a callback function that implements what > > > ssl_app_verify_callback() and match_cert_hostname() do, in your source file > > > > > > http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c > > > > > > Hm, you are lost in a twisty maze of verify callbacks, all alike. > > Definitely. > > > Actually the one you can set with SSL_set_verify() isn't the one you > > want. That's a low-level one, called from within the generic > > X509_verify_cert() function. > > Well, above I referred to SSL_set_verify() only because you had named > that function in <https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32>: Yeah, I was wrong. You were the one who called me an expert; I didn't :) > Thank you very much for this code. > > To me it seems like this patch should be squashed into patch#2 (which > modifies TlsLib), after adapting it to the edk2 coding style. > > I have some comments / questions. > > > (1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be > replaced with SSL_set_ex_data(). OK. > > In case the client does not use the TlsSetVerifyHost() function, > SSL_set_ex_data() would not be called. However, app_verify_callback() > would still be called. > > Is my understanding correct that "hostname" (from SSL_get_ex_data()) > would be NULL in that case, and therefore both > X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would > be omitted? > > (I mean this looks like the correct behavior to me; just asking if that > is indeed the intent of the code.) Yep. In the case where there's no such ex_data attached to the SSL object, our own app_verify_callback() function would do nothing special, just call through to the standard X509_verify_cert() function that would have been called if we hadn't registered our own override. You may call this "correct". I think you're right, in the context you meant it. But I suggest there is a conversation to be had about whether it's ever "correct" for a crypto library to silently accept any bloody certificate it sees, regardless of what the Subject of that certificate is. Or whether perhaps it ought to fail safe, and require you to jump through extra hoops if you *really* want to accept any certificate. In a world where crypto libraries made it hard for the users to Do The Wrong Thing, CVE-2019-14553 would never have happened. But we digress. Is it late enough in the day for me to start drinking yet? > (2) The documentation of the APIs seems a bit strange. I've found the > following "directory" web pages: > > https://www.openssl.org/docs/man1.0.2/man3/ > https://www.openssl.org/docs/man1.1.0/man3/ > https://www.openssl.org/docs/man1.1.1/man3/ > > edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only > look at the last page. However, SSL_get_ex_new_index() is only > documented in the first page. > > Is my understanding correct that each OpenSSL API should be looked up in > the latest documentation directory that appears to describe it? > > In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1 > manual, and fail to find it, does that mean that the API is deprecated, > or only that I should look at an earlier release of the manual? I was just looking at the code. I happened to be looking at OpenSSL HEAD but the sconnect demo I showed in my second email works with 1.1.1. > > (3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around > CRYPTO_get_ex_new_index(). > > https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html > https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html > > Based on that, I think the second ("argp") parameter of our > SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of > the "free" and "dup" functions are unused. > > Do you agree? Yeah, I had just cargo-culted that from elsewhere, and was assuming it was performing some kind of de-duplication or alternative lookup functionality. It does look like you can leave it NULL and it doesn't matter. > > (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has > already been stored for the same index? > > Do we have to first fetch it with SSL_get_ex_data() and free it, or will > it be automatically freed with "free_func"? It will be automatically freed with free_func. Note that this doesn't work in the sconnect.c patch I sent, but only because I'd forgotten to actually add the call to SSL_get_ex_new_index(). After I fixed that and wasn't just using an index of zero, I saw the printf() that I'd added in my free_func. > (Note: I think that, if we used a "new_func" for allocating anything, > this question could be relevant the very first time SSL_set_ex_data() > were called.) I believe new_func is only for when a new SSL object is created. We don't need that. > (5) The most confusing part... If I look at the 1.0.2 documentation, it > says about "dup_func": > > > dup_func() is called when a structure is being copied. [...] The > > from_d parameter is passed a pointer to the source application data > > when the function is called, when the function returns the value is > > copied to the destination: the application can thus modify the data > > pointed to by from_d and have different values in the source and > > destination > > This makes no sense: I wouldn't want to modify the source application > data in-place, just to give the new parent object a modified instance of it! > > Also, how would OpenSSL know how many bytes to copy? > > So I looked at the code, and it turns out dup_func is called with a > (void**), not a (void*): > > if (!storage[i]->dup_func(to, from, &ptr, i, > storage[i]->argl, storage[i]->argp)) > > The 1.1.1 documentation is more accurate; it says: > > https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html > > > The from_d parameter needs to be cast to a void **pptr as the API has > > currently the wrong signature; that will be changed in a future > > version. The *pptr is a pointer to the source exdata. When the > > dup_func() returns, the value in *pptr is copied to the destination > > ex_data. If the pointer contained in *pptr is not modified by the > > dup_func(), then both to and from will point to the same data Yes, I think dup_func wants to do something like: int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, void *from_d, int idx, long argl, void *argp) { char **p = from_d; if (*p) { *p = strdup(*p); return !!*p; } return 1; } > So let me summarize the documentation status: > > - SSL_get_ex_new_index is not documented in 1.1.1, > > - SSL_get_ex_new_index is not documented in 1.1.0, > > - SSL_get_ex_new_index has a "shim" documentation in 1.0.2, and it > refers the reader to RSA_get_ex_new_index(), > > - RSA_get_ex_new_index is documented in 1.0.2 *incorrectly* (with regard > to dup_func), > > - the valid documentation is in 1.1.1, under CRYPTO_get_ex_new_index -- > but for learning about that function, I had to look at the library > source code. > > When you suggested the IP checking would be easy to imlement, you were > joking, right? :) I confess I hadn't realised that we needed to jump through ex_data hoops to even get the hostname information to the right place in the callback. But I don't think I actually said "easy", did I? Only that it would take less typing than we'd already expended in trying to make excuses for the regression. I also said you could manage it, which you seem to have proved by working out the details I had... erm... left as an exercise for you :) > (6) Given that we are introducing callbacks (from > CryptoPkg/Library/OpensslLib to CryptoPkg/Library/TlsLib), and OpenSSL > does not declare these function prototypes with EFIAPI, the callbacks > too must be defined with "native" (not EFIAPI) calling convention. Yes, that makes sense. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49015): https://edk2.groups.io/g/devel/message/49015 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/15/19 15:54, Laszlo Ersek wrote: > On 10/15/19 13:03, David Woodhouse wrote: >> The "app callback" in my OpenConnect example is set on the SSL_CTX not >> the SSL object, and is called from the top-level >> ssl_verify_cert_chain() function *instead* of X509_verify_cert(). >> >> It is X509_verify_cert() which can do the hostname/IP checks for us, if >> we can only tell it that we want it to. But the X509_VERIFY_PARAM >> object is private to the SSL. >> >> As discussed, we have the SSL_set1_host() accessor function which lets >> us set the hostname. The implementation really is a simple one-liner, >> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way >> for use to set the IP address from the outside, without an equivalent >> accessor function for that (and without SSL_set1_host() spotting that >> the string it's given is an IP address, and doing so). >> >> But what we can do is stash the target string in some ex_data hanging >> off the SSL object, then have an app callback — which *can* reach the >> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or >> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the >> normal X509_verify_cert() function that it has overridden. >> >> Something like this... and instead of calling SSL_set1_host(ssl, host) >> your own code now has to call >> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); >> >> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c >> index f9ad6f6b946c..add5810cc4bd 100644 >> --- a/CryptoPkg/Library/TlsLib/TlsInit.c >> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c >> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> >> #include "InternalTlsLib.h" >> >> +/* You are lost in a twisty maze of SSL cert verify callbacks, all >> + * alike. All we really wanted to do was call SSL_set1_host() and >> + * have it work for IP addresses too, which OpenSSL PR#9201 will do >> + * for us. But until we update OpenSSL, that doesn't work. And we >> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address >> + * for ourselves. >> + * >> + * So we install an app_verify_callback in the SSL_CTX (which is >> + * different to the per-SSL callback wae can use, because it happens >> + * sooner. All our callback does it set the hostname or IP address in >> + * the X509_VERIFY_PARAM like we wanted to in the first place, and >> + * then call X509_verify_param() which is the default function. >> + * >> + * How does it find the hostname/IP string? It's attached to the SSL >> + * as ex_data, using this index: >> + */ >> +static int ssl_target_idx; >> + >> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, >> + int idx, long argl, void *argp) >> +{ >> + /* Free it */ >> +} >> + >> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, >> + void *from_d, int idx, long argl, void *argp) >> +{ >> + /* strdup it */ >> + return 0; >> +} >> + >> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) >> +{ >> + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); >> + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx); >> + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx); >> + >> + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) >> + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); >> + >> + return X509_verify_cert(ctx); >> +} >> + >> /** >> Initializes the OpenSSL library. >> >> @@ -40,6 +83,9 @@ TlsInitialize ( >> return FALSE; >> } >> >> + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL, >> + ssl_target_dup, ssl_target_free); >> + >> // >> // Initialize the pseudorandom number generator. >> // >> @@ -106,6 +152,10 @@ TlsCtxNew ( >> // >> SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); >> >> + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which >> + * we could have done as SSL_set_verify(). Twisty maze, remember? */ >> + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL); >> + >> return (VOID *) TlsCtx; >> } > (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has > already been stored for the same index? > > Do we have to first fetch it with SSL_get_ex_data() and free it, or will > it be automatically freed with "free_func"? > > (Note: I think that, if we used a "new_func" for allocating anything, > this question could be relevant the very first time SSL_set_ex_data() > were called.) A similar question: is it possible that app_verify_callback() is called more frequently than SSL_set_ex_data() (in TlsSetVerify())? Because that means that the frequency of SSL_set1_host() calls changes. Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it could be called multiple times per TlsSetVerify(). Is that the case? If it is, is it OK? To me the ownership of these strings (i.e., what component is responsible for freeing the strings) is impenetrable. :( Even the UEFI 2.8 spec doesn't explain whether EFI_TLS_SET_SESSION_DATA saves the array of characters pointed-to by "EFI_TLS_VERIFY_HOST.HostName", or just the pointer itself. :/ Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49019): https://edk2.groups.io/g/devel/message/49019 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/15/19 18:56, Laszlo Ersek wrote: > On 10/15/19 15:54, Laszlo Ersek wrote: >> On 10/15/19 13:03, David Woodhouse wrote: > >>> The "app callback" in my OpenConnect example is set on the SSL_CTX not >>> the SSL object, and is called from the top-level >>> ssl_verify_cert_chain() function *instead* of X509_verify_cert(). >>> >>> It is X509_verify_cert() which can do the hostname/IP checks for us, if >>> we can only tell it that we want it to. But the X509_VERIFY_PARAM >>> object is private to the SSL. >>> >>> As discussed, we have the SSL_set1_host() accessor function which lets >>> us set the hostname. The implementation really is a simple one-liner, >>> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way >>> for use to set the IP address from the outside, without an equivalent >>> accessor function for that (and without SSL_set1_host() spotting that >>> the string it's given is an IP address, and doing so). >>> >>> But what we can do is stash the target string in some ex_data hanging >>> off the SSL object, then have an app callback — which *can* reach the >>> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or >>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the >>> normal X509_verify_cert() function that it has overridden. >>> >>> Something like this... and instead of calling SSL_set1_host(ssl, host) >>> your own code now has to call >>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); >>> >>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c >>> index f9ad6f6b946c..add5810cc4bd 100644 >>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c >>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c >>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> #include "InternalTlsLib.h" >>> >>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all >>> + * alike. All we really wanted to do was call SSL_set1_host() and >>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do >>> + * for us. But until we update OpenSSL, that doesn't work. And we >>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address >>> + * for ourselves. >>> + * >>> + * So we install an app_verify_callback in the SSL_CTX (which is >>> + * different to the per-SSL callback wae can use, because it happens >>> + * sooner. All our callback does it set the hostname or IP address in >>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and >>> + * then call X509_verify_param() which is the default function. >>> + * >>> + * How does it find the hostname/IP string? It's attached to the SSL >>> + * as ex_data, using this index: >>> + */ >>> +static int ssl_target_idx; >>> + >>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, >>> + int idx, long argl, void *argp) >>> +{ >>> + /* Free it */ >>> +} >>> + >>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, >>> + void *from_d, int idx, long argl, void *argp) >>> +{ >>> + /* strdup it */ >>> + return 0; >>> +} >>> + >>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) >>> +{ >>> + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); >>> + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx); >>> + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx); >>> + >>> + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) >>> + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); >>> + >>> + return X509_verify_cert(ctx); >>> +} >>> + >>> /** >>> Initializes the OpenSSL library. >>> >>> @@ -40,6 +83,9 @@ TlsInitialize ( >>> return FALSE; >>> } >>> >>> + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL, >>> + ssl_target_dup, ssl_target_free); >>> + >>> // >>> // Initialize the pseudorandom number generator. >>> // >>> @@ -106,6 +152,10 @@ TlsCtxNew ( >>> // >>> SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); >>> >>> + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which >>> + * we could have done as SSL_set_verify(). Twisty maze, remember? */ >>> + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL); >>> + >>> return (VOID *) TlsCtx; >>> } > >> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has >> already been stored for the same index? >> >> Do we have to first fetch it with SSL_get_ex_data() and free it, or will >> it be automatically freed with "free_func"? >> >> (Note: I think that, if we used a "new_func" for allocating anything, >> this question could be relevant the very first time SSL_set_ex_data() >> were called.) > > A similar question: > > is it possible that app_verify_callback() is called more frequently than > SSL_set_ex_data() (in TlsSetVerify())? > > Because that means that the frequency of SSL_set1_host() calls changes. > Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it > could be called multiple times per TlsSetVerify(). Is that the case? > > If it is, is it OK? Ehh, I failed to ask the actual question. Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, every time just before we call X509_verify_cert()? My concern is not with the crypto functionality, but whether we could be leaking memory allocations. Thanks Laszlo > To me the ownership of these strings (i.e., what component is > responsible for freeing the strings) is impenetrable. :( > > Even the UEFI 2.8 spec doesn't explain whether EFI_TLS_SET_SESSION_DATA > saves the array of characters pointed-to by > "EFI_TLS_VERIFY_HOST.HostName", or just the pointer itself. :/ > > Laszlo > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49026): https://edk2.groups.io/g/devel/message/49026 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote: > Ehh, I failed to ask the actual question. > > Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, > every time just before we call X509_verify_cert()? > > My concern is not with the crypto functionality, but whether we could be > leaking memory allocations. You had to ask yourself that before approving the original version of TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed any restriction on calling TlsSetVerifyHost() more than once... The answer is yes, btw — it's fine. Note also my observation that we should insist on TlsSetVerifyHost being called at *least* once, or the connection should fail. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49085): https://edk2.groups.io/g/devel/message/49085 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/16/19 11:40, David Woodhouse wrote: > On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote: >> Ehh, I failed to ask the actual question. >> >> Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, >> every time just before we call X509_verify_cert()? >> >> My concern is not with the crypto functionality, but whether we could be >> leaking memory allocations. > > You had to ask yourself that before approving the original version of > TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed > any restriction on calling TlsSetVerifyHost() more than once... You are correct, of course. I seem to recall that I hand-waved that question away, seeing that TlsSetVerifyHost() simply passed the hostname (the pointer to the char array) into an OpenSSL API. I guess when I first looked at that call with any kind of focus, I wasn't *that* concerned about the life-cycle yet... > > The answer is yes, btw — it's fine. Thanks! > > Note also my observation that we should insist on TlsSetVerifyHost > being called at *least* once, or the connection should fail. > I wonder if we could make this an implementation detail in edk2 *first*, while a matching USWG Mantis ticket were in progress. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49087): https://edk2.groups.io/g/devel/message/49087 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote: > (I can't test it easily myself, as I don't even know how to create a > server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) I had to look it up again, but here goes... $ cat v3.ext subjectAltName = @alt_names [alt_names] DNS.1 = lersek-test.redhat.com IP.2 = 192.168.124.2 IP.3 = fd33:eb1b:9b36::2 $ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr ... $ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem -extfile v3.ext Signature ok subject=C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Getting Private key $ openssl x509 -in cert.pem -noout -text Certificate: Data: Version: 3 (0x2) Serial Number: 56:c5:33:0f:b1:2d:e5:b5:1e:89:e5:a7:a2:45:a9:06:43:1f:4a:1e Signature Algorithm: sha256WithRSAEncryption Issuer: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Validity Not Before: Oct 15 15:56:11 2019 GMT Not After : Oct 12 15:56:11 2029 GMT Subject: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Subject Public Key Info: Public Key Algorithm: rsaEncryption RSA Public-Key: (2432 bit) Modulus: 00:b4:6b:27:98:25:af:c1:ff:1e:ca:b0:7e:f4:d8: bc:ed:43:86:67:54:5d:da:b4:1e:c2:90:5f:83:3c: 02:11:fc:13:72:85:b2:88:a4:65:41:0b:76:5f:23: be:8a:9f:fe:79:4b:73:3b:2e:c7:4b:3c:bf:16:c9: 97:55:35:17:f3:a1:72:4b:30:c2:e0:27:94:12:f3: 56:00:e6:ce:82:4b:11:5d:a4:1e:9b:fa:fa:b9:1b: 2a:4d:18:b5:ba:a5:e6:0c:c7:a8:a8:a1:6d:aa:88: 84:dc:96:0e:b2:6c:1c:35:aa:e7:c7:94:3d:f9:d5: c7:c2:a2:0d:4b:b3:6e:7a:f7:08:5f:c5:09:cd:15: 93:1a:f7:98:df:2a:4c:66:89:24:ed:1f:d0:16:63: 81:65:a5:58:3b:a1:cd:25:62:9b:99:81:54:08:17: 18:ec:7c:2f:08:a2:3b:28:57:32:9d:17:47:0a:86: fb:62:b1:41:99:e6:fb:de:a8:ea:20:7e:f3:1b:ee: ba:ea:9a:21:64:29:92:f2:ad:73:e5:19:05:9d:37: 53:e2:11:9f:18:5f:22:ba:e2:8b:0d:00:8c:9e:2f: a7:87:3d:40:be:4a:a2:a5:92:08:0c:2e:61:c0:58: 7c:9a:99:e1:d6:ac:83:39:25:cf:3e:1b:ed:eb:a3: 6d:9d:cb:c5:38:de:c1:c7:6e:9b:34:14:be:30:3e: 82:90:1e:b9:4a:9a:76:e4:ef:33:0c:46:a2:31:72: f6:c3:61:0b:f8:aa:67:89:f4:a5:e5:76:37:a1:29: 9f:80:79:aa:75 Exponent: 65537 (0x10001) X509v3 extensions: X509v3 Subject Alternative Name: DNS:lersek-test.redhat.com, IP Address:192.168.124.2, IP Address:FD33:EB1B:9B36:0:0:0:0:2 Signature Algorithm: sha256WithRSAEncryption 37:8c:17:6c:4d:5f:05:b6:70:b9:96:49:0a:e3:f6:3c:bd:3b: d0:fe:56:ee:ad:58:15:6e:a6:79:a8:3b:d4:fa:09:f9:7d:85: 8a:8b:14:7b:e4:db:bf:2d:8d:32:28:26:d6:37:a5:51:90:e9: 75:25:b9:9d:63:db:35:29:8a:58:61:56:b2:2a:5a:d3:80:b7: 1d:4c:05:0b:49:da:6f:ec:67:f5:3d:09:f2:58:92:43:8d:39: d7:f4:f3:3c:bd:9b:16:a2:c9:c0:63:5d:c9:1a:c3:a7:24:fa: 31:8c:7c:3e:98:98:87:8f:5b:fb:00:f5:41:15:16:89:c6:e3: c4:63:3a:3d:3e:b8:b5:b7:af:cb:11:1a:13:f4:b2:df:c4:f4: a1:a2:9c:d1:05:20:84:65:70:91:41:be:f4:26:c2:63:07:46: d0:63:bf:27:3f:42:9c:69:22:e1:d6:6a:41:dc:97:51:2d:ef: a1:11:20:ed:89:57:d6:d2:ad:6c:7f:88:69:ae:31:51:e8:cb: 9e:3a:e1:49:48:01:5b:d5:ab:93:53:5e:cb:2f:72:6e:84:af: d0:c2:91:41:29:6f:3c:0b:df:c6:9c:77:14:fd:29:fc:65:0b: 2d:6c:61:69:a6:72:19:38:5f:a1:83:fd:6c:22:02:d7:b6:81: 9e:05:7c:58:2c:c9:eb:c0:09:aa:07:d1:b7:15:a1:e3:ea:27: b1:f7:70:87:fe:d6:16:57:67:70:fe:65:9a:0f:1b:11:be:22: 08:2f:21:50:30:a4:35:99:d3:fb:4d:40:22:39:2c:f3 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49016): https://edk2.groups.io/g/devel/message/49016 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/15/19 17:57, David Woodhouse wrote: > On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote: >> (I can't test it easily myself, as I don't even know how to create a >> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) > > I had to look it up again, but here goes... > > $ cat v3.ext > subjectAltName = @alt_names > [alt_names] > DNS.1 = lersek-test.redhat.com > IP.2 = 192.168.124.2 > IP.3 = fd33:eb1b:9b36::2 > $ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr > ... > $ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem -extfile v3.ext I'm not familiar with this x509 invocation ("-signkey"). Thus far I've used x509 to sign self-signed certificate requests with a CA key: openssl x509 -req -in request.csr -out signedcert.pem \ -CA ca-cert.pem -CAkey ca-key.pem [-CAcreateserial] I guess "-signkey ca-key.pem" is a shorthand for the (-CA, -CAkey) pair? (I've tried to look at the manual; I couldn't say I'm wiser now.) Either way: why do we add the subject alternative names when the CA signs the request? Shouldn't the *original* certificate request state what alternative names can stand for the same subject? (I don't even understand how a CA can usefully insert such an extension; after all, it cannot be signed by the original certificate requestor!) The "openssl req" command too seems to accept "-extensions" -- why are we not required to use that? To me it seems like the only acceptable place, to add alternative names. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49025): https://edk2.groups.io/g/devel/message/49025 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > I have not tested this, but I started looking when there was a message > on the edk2 list from someone who was reporting that it didn't work for > IPv6 URIs, IIRC. > > You are using SSL_set1_host(), and I believe you're just passing in the > bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]". Here, I want to highlight is that UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2: "...TLS session hostname for validation which is used to verify whether the name within the peer certificate matches a given host name..." In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature by following the standard TLS HostName verification capability. > > That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the > SSL connection. Yes. > > In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the > code simply iterates over the members of that list, calling > X509_check_host() for each one. It never calls X509_check_ip(). Yes. > > If you look in openssl/crypto/x509/v3_utl.c you can see the > X509_check_host() really does only check hostnames. Yes. > You'd need to call X509_check_ip_asc() to check hostnames. And something would need to > have stripped the [] which surround an IPv6 literal. > Disagree, why need check the IP here since we only focus on the hostname verification? For HttpDxe driver, it's the implementation choice to treat the IP in URI as hostname string format. As I said before in the email, if the CN or SAN (Seems only in X509 V3) in the certificate are set correctly, it should be OK to pass the verification. Laszlo and I already have verified that. > I can't see how this can work. Have you tested it since the report on > the list that it wasn't working? > Sorry, I can't remember there is any failure of Ipv6 URI reported from edk2. If you can find it, that will be better. > cf. https://github.com/openssl/openssl/pull/9201 which is being ignored > by the OpenSSL developers — OpenSSL really doesn't make life easy for > you here, which is a shame. > > > > For the series patches here, we are intending to support the host > > name validation, I think we can commit the series patches since we > > pass the verification of IPV6 URL, what do you think? > > If it passes the verification of IPv6 literals, then all my analysis is > broken and so was the report on the list that prompted me to start > looking (or I'm misremembering that report). In that case, sure, go > ahead and commit. > > > Thanks, > > Jiaxin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48683): https://edk2.groups.io/g/devel/message/48683 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 10/01/19 01:21, Laszlo Ersek wrote: > On 09/29/19 08:09, Wang, Jian J wrote: >> For this patch series, >> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more. >> Remove it at push time and no need to send a v2. >> 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b >> required. Please raise it asap if any objections. >> 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > > > * Can you please confirm that these patches match those that we > discussed here: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 To answer my own question, I've now compared the patches from those BZ comments linked above, with the present series. Here's a list of differences. (1) The subject lines now include the reference "(CVE-2019-14553)". This is great, *but* please be sure to insert a space character before the opening parenthesis! (In every patch.) (2) The commit messages reference both the BZ and the CVE number. Good. (3) In the commit messages, the line Contributed-under: TianoCore Contribution Agreement 1.0 has been upgraded to Contributed-under: TianoCore Contribution Agreement 1.1 I think this is wrong. The lines should have been removed, due to the SPDX adoption. Please update all the commit messages. (4) Copyright notice updates are gone from the patches. That's fine: the reason is that the underlying files have seen their copyright notices updated, meanwhile. Otherwise, the patches (code, commit messages, and feedback tags) are identical. Before you push the patches (or post a v2), please fix issues (1) and (3). Now, regarding the other set of questions: > * In the BZ, David and Bret raised some questions: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > and > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > The latest comment in the bug is c#41. I'm not under the impression that > all concerns raised by David and Bret have been addressed (or > abandoned). I'd like David and Bret to ACK the patches. I'll first have to process the new comments down-thread. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48664): https://edk2.groups.io/g/devel/message/48664 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo, Thanks the comments. Best Regards! Jiaxin > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Wednesday, October 9, 2019 11:55 PM > To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Jiaxin > <jiaxin.wu@intel.com>; David Woodhouse <dwmw2@infradead.org>; Bret > Barkelew <Bret.Barkelew@microsoft.com> > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On 10/01/19 01:21, Laszlo Ersek wrote: > > On 09/29/19 08:09, Wang, Jian J wrote: > >> For this patch series, > >> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not > needed any more. > >> Remove it at push time and no need to send a v2. > >> 2. Since it's security patch which had been reviewed separately, I see no > reason for new r-b > >> required. Please raise it asap if any objections. > >> 3. Acked-by: Jian J Wang <jian.j.wang@intel.com> > > > > > > * Can you please confirm that these patches match those that we > > discussed here: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > To answer my own question, I've now compared the patches from those BZ > comments linked above, with the present series. Here's a list of > differences. > > (1) The subject lines now include the reference "(CVE-2019-14553)". > > This is great, *but* please be sure to insert a space character before > the opening parenthesis! (In every patch.) > > (2) The commit messages reference both the BZ and the CVE number. > > Good. > > (3) In the commit messages, the line > > Contributed-under: TianoCore Contribution Agreement 1.0 > > has been upgraded to > > Contributed-under: TianoCore Contribution Agreement 1.1 > > I think this is wrong. The lines should have been removed, due to the > SPDX adoption. Please update all the commit messages. > > (4) Copyright notice updates are gone from the patches. > > That's fine: the reason is that the underlying files have seen their > copyright notices updated, meanwhile. > > > Otherwise, the patches (code, commit messages, and feedback tags) are > identical. > > Before you push the patches (or post a v2), please fix issues (1) and (3). > > Now, regarding the other set of questions: > > > * In the BZ, David and Bret raised some questions: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > and > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > The latest comment in the bug is c#41. I'm not under the impression that > > all concerns raised by David and Bret have been addressed (or > > abandoned). I'd like David and Bret to ACK the patches. > > I'll first have to process the new comments down-thread. > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48684): https://edk2.groups.io/g/devel/message/48684 Mute This Topic: https://groups.io/mt/34307578/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.