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

["David Woodhouse" <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 4232 bytes --]

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.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]