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

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

On 10/11/19 13:16, David Woodhouse wrote:
> 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.

Indeed that is the case; I'm aware of it. Thanks for pointing it out.

This functional regression does not translate to an actual end-user
regression, under my circumstances. From my perspective, UEFI HTTPS boot
is a feature that cannot be enabled *at all* in a production
environment, due to the possible MITM. In other words, I cannot build
OVMF with

  -D NETWORK_HTTP_BOOT_ENABLE -D NETWORK_TLS_ENABLE

and give the binary to users with a clear conscience. They'd see UEFI
HTTPS boot, say "hey this is secure!", and use it. That would be a false
sense of security.

With the patches applied, yes, some valid certificates would be
rejected. That would be a normal bug report, or a known limitation, or
even a "layered" feature request. Not a security bug. It would not
prevent the basic -- or let's say, "somewhat restricted" -- enablement
of UEFI HTTPS boot. Not a deal-breaker.

It would be possible for us to announce, "and now we're giving you UEFI
HTTPS boot that is secure to the best of our knowledge, only it does not
*yet* support GEN_IP in SAN. Stay tuned for that."

This would not be a regression, from an end-user perspective.

Again, I'm not attempting to "override" or "suppress" your (implied)
NACK. If the agreement turns out to be that we should *first* add
support for GEN_IP in the SAN, I will not campaign for merging the v1
patches *upstream*, right now. I respect your feedback. No patch should
be merged while there are outstanding questions, let alone well-founded
NACKs.


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


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

More precisely, I'd be happy to see patches committed that plug the
security hole. Which has been *completely* preventing us from enabling
this feature.

I agree that, while working towards a feature or a bugfix, already
functional things should *not* be regressed -- even temporarily, even if
the regression lasts only from patch#n to patch#(n+1) in a single
series. This is a very strong guideline, minimally for bisectability.

But to me, this feature has never existed in practice. I found and
reported the security issue (BZ#960) while working on the downstream
enablement of UEFI HTTPS boot. The problem caused me to postpone the
downstream enablement. So for me there's nothing to regress.

Similarly, knowledge of this issue prevented me from porting
NETWORK_TLS_ENABLE from OvmfPkg to ArmVirtPkg, up-stream:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1007
  https://bugzilla.tianocore.org/show_bug.cgi?id=1009

For a long time, I couldn't publicly tell the truth about splitting
BZ#1009 off of BZ#1007. My *real* incentive was to save at least
ArmVirtPkg users from exposure. I called Ard on the phone, and we agreed
to postpone TLS in ArmVirPkg.

(I couldn't do the same for OVMF users, as I would have had to back out
TLS settings from OvmfPkg, and that would have drawn stares.)

I went on to intentionally ignore BZ#1009, hoping no-one would ask me
about it. Then Guillaume did. See my answer here:
<https://bugzilla.tianocore.org/show_bug.cgi?id=1009#c4>.

Therefore, UEFI HTTPS boot has not existed for me, in practice, even in
the *upstream* project, to the extent that I was able to block the
"spreading" of the feature.


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

David: it *is* hard! It is hard for me. I wouldn't know where to begin.

I assume it may be hard for Jiaxin too.

If it's not hard for you, can you please post the patches? Feel free to
pick up this series as a starting point, rework it, and then post an
updated variant.

If it takes two more weeks and you can solve the problem to everyone's
satisfaction (including your own!), I'll be overjoyed. I'll be happy to
wait!

If it takes four more months... then I might apply the present patchset
down-stream only.

As always, I strongly favor "upstream first". Show us the code, please?


> Ultimately, though, do whatever you like. I am not the final arbiter of
> engineering sanity and common sense for the EDK2 project.

No well-founded NACK or pending question should be ignored in patch review.

> 
> If I were, the world would be a very different place.

No doubt! ;)

Laszlo