[edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Wu, Jiaxin posted 4 patches 2 weeks ago
Failed in applying to current master (apply log)
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(-)

[edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 2 weeks ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wang, Jian J 2 weeks ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 2 weeks ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 2 weeks ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 1 week ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 1 week ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 6 days ago
> 
> 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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 22 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 21 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 4 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 4 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 21 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 21 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 5 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 4 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 23 hours ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Laszlo Ersek 1 day ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by David Woodhouse 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 5 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Posted by Wu, Jiaxin 6 days ago
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]
-=-=-=-=-=-=-=-=-=-=-=-