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

["Laszlo Ersek" <lersek@redhat.com>]

Hi Jiaxin,

On 10/16/19 07:18, Wu, Jiaxin wrote:
> These days I'm too busy with other things. Just take the time to
> review the email discussion & researched the correct behavior of HTTPS
> cert verification.
>
> I did never though my patch caused the function regression, and I'm
> also not loop in the long ago email discussion reported from AMI.
>
> Now, I understand the testing *gap* between us is that we set the IP
> address string in the CN or dNSName filed of SAN when creating the
> certificate, while you don't prefer an IP address string setting in
> dNSName but setting the OCTET string to iPAddress of SAN. That's why
> the failure happen with different certificates setting (I've explained
> it many times).

You have misunderstood.

It's not my *preference* to set an "OCTET string in the SAN iPAddress".

It is so much *not* my preference that I didn't even know how to
generate such a server certificate until David showed me!

I gave Tested-by for your patch set because my personal validation
mapped cleanly to your validation. In fact I think your validation was
more extensive than mine, and so my testing was a *proper subset* of
yours.

But, that's not the whole story. When these patches were first exposed
to the infosec group in TianoCore#960, in June, AMI reported a practical
regression with them, to the list. Unfortunately, at that time, that
regression report didn't reach you (it should have been made in the BZ,
not on the list, because the patches were also attached to the BZ -- at
that time, anyway).

Therefore, we have to figure out what to do about the regression
encountered by AMI. We have two options:

- We can call their use case "invalid", assuming we can prove this from
  the UEFI spec.

- We can attempt to accommodate their use case.

The present patch is an *attempt* to accommodate their use case, based
on David's guidance. Unfortunately, it does not work. (See the "Notes"
section in the patch for the symptoms.) So we can decide to stop
pursuing this use case, or we can continue researching it. We can *also*
call AMI's use case invalid still, if we can prove that from the UEFI
spec.

To me, AMI's use case looked valid (= covered under the UEFI spec), but
that's just me. We can contact the USWG for help with the spec
interpretation.


> With my patches to UEFI part, I believe you can pass the test if
> setting the IP in CN or dNSName of SAN,

Yes, I fully agree about that. That's why I gave Tested-by earlier in
this thread, for your patches 1-4. Please see:

  http://mid.mail-archive.com/6939ba4e-6c77-0769-4ac2-c3ba1ea9a0b7@redhat.com
  https://edk2.groups.io/g/devel/message/48813


> but no one shared me any significant evidence to indicate it's the
> mistake idea if URI is specified as an IP address rather than a
> Hostname.

I have not shared such evidence because I do not *believe* that storing
the IP address in the Common Name of the certificate is a mistake.

The "genkey" utility that Red Hat recommends (in the official docs) for
setting up mod_ssl with Apache places the IP address in the Common Name,
if the user enters an IP address. David argues that this is wrong, but
I'm not convinced just yet.

Therefore, I agree that your v1 patches 1-4 cover a valid use case.

The issue is that they break a *different* use case, which *may* also be
valid. (Dependent on how we interpret the UEFI spec.)


> On the contrary, my explains to my thought/viewpoint when creating the
> patch was treated as poor excuse:(.

Not by me, for sure.


> Now, I post my own investigation here to correct my mistake thought:
>
> According my previous experiment and RFC5280, the dNSName of
> subjectAltName is IA5String, which means the string of an IPv4 or IPv6
> address is also legal to be included in the dNSName:
>    SubjectAltName ::= GeneralNames
>    GeneralName ::= CHOICE {
>         otherName                       [0]     OtherName,
>         rfc822Name                      [1]     IA5String,
>         dNSName                         [2]     IA5String,
>         x400Address                     [3]     ORAddress,
>         directoryName                   [4]     Name,
>         ediPartyName                    [5]     EDIPartyName,
>         uniformResourceIdentifier       [6]     IA5String,
>         iPAddress                       [7]     OCTET STRING,
>         registeredID                    [8]     OBJECT IDENTIFIER }
>
> If so, I thought it should be doable and make sense to treat the IP in
> URI as hostname in CN or dNSName of SAN. Besides, it can also pass the
> verification if the certificate only has the CN (NO SAN case, I think
> it's also the failure case Laszlo met in Note(2)). I believed it's the
> implementation choice before if no below finding in RFC 6125. But with
> the series discussion here, I'm starting to wonder whether there is
> any RFC or document to restrict the TLS & certificate checking roles ,
> for example:
> 1) IP in the URI can be treated as Hostname or it only can be used for
> the iPAddress of SAN verification?
> 2) Certificate must have the iPAddress or dNSName of SAN?
>
> Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 :
>
>    If a subjectAltName extension of type dNSName is present, that MUST
>    be used as the identity.  Otherwise, the (most specific) Common
>    Name field in the Subject field of the certificate MUST be used.
>    Although the use of the Common Name is existing practice, it is
>    deprecated and Certification Authorities are encouraged to use the
>    dNSName instead.
>    ...
>    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?


> So, the question is more clear here:
> 1) IP in the URI is used for the iPAddress of SAN verification, but
> *not say no* if upper driver set to dNSName of SAN.

I'm not sure what you mean by "not say no if upper driver set to dNSName
of SAN".

Do you mean that we can ignore the IP address in the SAN if the URL
contains a hostname? (And then the hostname matches either the dNSName
in the SAN (high prio), or the CN (low prio)?) I agree about that.


> 2) iPAddress of SAN MUST be present in the certificate and must
> exactly match the IP in the URI. If not, we can treat as failure.

Yes, I'm compelled to agree.


> Ok, with above declaim in RFC, I agree there is the function issue in
> my series patches to treat IP address in URI as string setting to
> hostname,  which breaks the above 2 rules defined in RFC 6125.
>
> Now, to resolve the problem, I think the best compatibility can
> actually be reached by setting the IP address both as iPAddress and
> dNSName in SAN.

Wait, I don't understand the word "setting". "Setting" is an activity
that the server admin performs. The UEFI TLS code is on the client side,
and has no control over the elements of the certificate.

Do you mean: the best compatibility can be reached by *matching* the IP
address (from the URL) in both SAN.iPAddress and SAN.dNSName?

I think I disagree with that. The part you quoted above from the RFC
states that an IP-based URL can only be accepted via SAN.iPAddress.


> 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

> diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
> index c481e292fc88..393e1a704d2e 100644
> --- a/ssl/ssl_lib.c
> +++ b/ssl/ssl_lib.c
> @@ -965,11 +965,21 @@ int SSL_set_trust(SSL *s, int trust)
>
>  int SSL_set1_host(SSL *s, const char *hostname)
>  {
> +    /* If a hostname is provided and parses as an IP address,
> +     * treat it as such. */
> +    if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1)
> +        return 1;
> +
>      return X509_VERIFY_PARAM_set1_host(s->param, hostname, 0);
>  }
>
>  int SSL_add1_host(SSL *s, const char *hostname)
>  {
> +    /* If a hostname is provided and parses as an IP address,
> +     * treat it as such. */
> +    if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1)
> +        return 1;
> +
>      return X509_VERIFY_PARAM_add1_host(s->param, hostname, 0);
>  }
>

Therefore we should not burden the HTTPS driver in edk2 with this; and
we should even less complicate the UEFI spec with it.

Back to your email:


On 10/16/19 07:18, Wu, Jiaxin wrote:
> 2) Just as David's suggestion and the patch send out by Laszlo, fix it
> in UEFI TLS part. I reviewed the existing UEFI 2.8, looks we can treat
> the HostName in Spec as the widely name (can be IP, DNS or email
> address name).

I don't understand how email enters the picture, but otherwise I agree.


> To make things simple & easier, I also prefer 2). So, I reviewed the
> patch from Laszlo:
>
> Comment1: How about setting the IP address both as iPAddress and
> dNSName in SAN for the compatibility, if so, I think below note2
> failure will be gone.

See above -- failure (2) appears valid, based on the part you quoted
from the RFC. I'll have to generate a better certificate.

Also, curl appears wrong to accept the certificate. (I'm stunned by
this!)


> 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.

TBH, I'm quite confused by the OpenSSL API: after all, just *what* is
the object that X509_VERIFY_PARAM is associated with? Does it belong to
the SSL object, or to the peer certificate (X509_STORE_CTX)?

Thank you Jiaxin for following up!
Laszlo