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

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

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