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.