Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

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

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

On Wed, 2019-10-16 at 09:36 +0200, Laszlo Ersek wrote:
> On 10/16/19 07:18, Wu, Jiaxin wrote:
> >    In some cases, the URI is specified as an IP address rather than a
> >    Hostname .  In this case, the iPAddress subjectAltName must be
> >    present in the certificate and must exactly match the IP in the
> >    URI.
> 
> Wow!
> 
> This seems to prove David right, and it suggests that symptom (2)
> encountered with my patch is actually *valid* behavior -- the
> certificate I generated with "genkey" is *not* valid for a URI that
> specifies an IP address!
> 
> This is not good news: the "curl" utility also accepts such a
> certificate as valid (IP address in URL, but the certificate only
> contains a CN,  matching the IP address in textual form). Is that a bug
> in "curl" then?

Yes, I believe so.
Please report it at https://github.com/curl/curl/issues


> > To achieve that, we have two methods:
> > 1) TLS provides the separate interfaces to upper driver to set the
> > iPAddress & dNSName (even email address), which means HTTPS driver
> > need follow RFC6125. But that will need spec changes.
> 
> I think this is not a good approach.

> David's pull request for OpenSSL, namely
> <https://github.com/openssl/openssl/pull/9201>, makes the argument that
> recognizing and prioritizing an IP address over a DNS hostname should be
> *internal* to OpenSSL itself.
> 
>   https://github.com/openssl/openssl/pull/9201/commits/da280b92520f5a0e41efb777a4d39bc907c42ecf


Indeed; I think we all agree here. I believe it was presented as a
straw man option just for completeness and even Jiaxin didn't seriously
intend that we'd do it.

But for the record, let me make it abundantly clear how wrong this
option would be...

I will *always* make the argument that crypto libraries should make it
trivial for users to get things like this right. I'm dismayed that we
even have to have this discussion. This stuff should Just Work™.

That applies just as much to the API that you present from the TlsLib.

If you give your users a function that takes a hostname and silently
does a completely bogus wrong thing when it's passed an IP address
instead, that's a *bad* API design.

Forcing users to work it out for themselves and call a *different*
function if they happen to have an IP address, and still doing that
bogus wrong thing if they call the original function, is just
compounding the error.

And yes, I'll argue quite strongly that OpenSSL shouldn't do that.

OpenSSL did it because of the way its API evolved, and I will continue
to have the discussion with them upstream about not designing APIs that
set their users up for failure. It is an ongoing theme.

But for UEFI? To even *propose* that we make such an appalling design
choice in TlsLib?

Just NO. Don't even contemplate it.

As noted above, I choose to believe that it was only being suggested as
a straw man, and only for completeness. I just wanted to make it
*absolutely* clear that it was entirely unacceptable.

We should apply that "don't set your users up for failure" principle
everywhere.

I will strongly assert, for example, that TlsLib shouldn't make the
same mistake in its API design that led to CVE-2019-14553 in the first
place. If a caller "forgets" to specify the hostname (or IP) that
they're connecting to, the connection should FAIL. It shouldn't default
to just connecting to anything. That's just batshit insane.

If you have to fix the UEFI spec to make setting the hostname
mandatory, then do so.

Don't set your users up for failure by design.


> > Comment2: do we really need the app_verify_callback function setting?
> > Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param,
> > HostName) in TlsSetVerifyHost directly? anything I missed in the
> > discussion?
> 
> I don't think client code should access "Ssl->param" directly. SSL
> should be treated as an opaque data structure.
> 
> However, I think you may have a point. Formally, the SSL_get0_param()
> function could be called to retrieve X509_VERIFY_PARAM.
> 
>   https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html
> 
> And then we could call X509_VERIFY_PARAM_set1_ip_asc() on that, perhaps.
> This would make both the ExData stuff and the custom certificate
> verification procedure unnecessary.

Haha, I had *looked* for that but somehow failed to find it. I can't
for the life of me work out how I missed it, now.

Yes, that would be a whole lot easier than the callback and ex_data
nonsense. It's really just as simple as

 X509_VERIFY_PARAM *vpm = SSL_get0_param(ssl);

 if (!X509_VERIFY_PARAM_set_ip_asc(vpm, hostname))
     /* Didn't like that, must be an actual hostname then */
     X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);


So what did we say about false modesty, Laszlo?

In the end you did actually solve it all for yourself — based on the
pointer I'd given in bugzilla, and then ignoring my subsequent
misdirection about callbacks and my overly complex attempt at doing it
myself :)





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