Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/16/19 11:40, David Woodhouse wrote: > On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote: >> Ehh, I failed to ask the actual question. >> >> Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, >> every time just before we call X509_verify_cert()? >> >> My concern is not with the crypto functionality, but whether we could be >> leaking memory allocations. > > You had to ask yourself that before approving the original version of > TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed > any restriction on calling TlsSetVerifyHost() more than once... You are correct, of course. I seem to recall that I hand-waved that question away, seeing that TlsSetVerifyHost() simply passed the hostname (the pointer to the char array) into an OpenSSL API. I guess when I first looked at that call with any kind of focus, I wasn't *that* concerned about the life-cycle yet... > > The answer is yes, btw — it's fine. Thanks! > > Note also my observation that we should insist on TlsSetVerifyHost > being called at *least* once, or the connection should fail. > I wonder if we could make this an implementation detail in edk2 *first*, while a matching USWG Mantis ticket were in progress. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49087): https://edk2.groups.io/g/devel/message/49087 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote: > Ehh, I failed to ask the actual question. > > Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, > every time just before we call X509_verify_cert()? > > My concern is not with the crypto functionality, but whether we could be > leaking memory allocations. You had to ask yourself that before approving the original version of TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed any restriction on calling TlsSetVerifyHost() more than once... The answer is yes, btw — it's fine. Note also my observation that we should insist on TlsSetVerifyHost being called at *least* once, or the connection should fail. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49085): https://edk2.groups.io/g/devel/message/49085 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/15/19 18:56, Laszlo Ersek wrote:
> On 10/15/19 15:54, Laszlo Ersek wrote:
>> On 10/15/19 13:03, David Woodhouse wrote:
>
>>> The "app callback" in my OpenConnect example is set on the SSL_CTX not
>>> the SSL object, and is called from the top-level
>>> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
>>>
>>> It is X509_verify_cert() which can do the hostname/IP checks for us, if
>>> we can only tell it that we want it to. But the X509_VERIFY_PARAM
>>> object is private to the SSL.
>>>
>>> As discussed, we have the SSL_set1_host() accessor function which lets
>>> us set the hostname. The implementation really is a simple one-liner,
>>> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
>>> for use to set the IP address from the outside, without an equivalent
>>> accessor function for that (and without SSL_set1_host() spotting that
>>> the string it's given is an IP address, and doing so).
>>>
>>> But what we can do is stash the target string in some ex_data hanging
>>> off the SSL object, then have an app callback — which *can* reach the
>>> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
>>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
>>> normal X509_verify_cert() function that it has overridden.
>>>
>>> Something like this... and instead of calling SSL_set1_host(ssl, host)
>>> your own code now has to call
>>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
>>>
>>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c
>>> b/CryptoPkg/Library/TlsLib/TlsInit.c
>>> index f9ad6f6b946c..add5810cc4bd 100644
>>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
>>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
>>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>> #include "InternalTlsLib.h"
>>>
>>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
>>> + * alike. All we really wanted to do was call SSL_set1_host() and
>>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
>>> + * for us. But until we update OpenSSL, that doesn't work. And we
>>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
>>> + * for ourselves.
>>> + *
>>> + * So we install an app_verify_callback in the SSL_CTX (which is
>>> + * different to the per-SSL callback wae can use, because it happens
>>> + * sooner. All our callback does it set the hostname or IP address in
>>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
>>> + * then call X509_verify_param() which is the default function.
>>> + *
>>> + * How does it find the hostname/IP string? It's attached to the SSL
>>> + * as ex_data, using this index:
>>> + */
>>> +static int ssl_target_idx;
>>> +
>>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
>>> + int idx, long argl, void *argp)
>>> +{
>>> + /* Free it */
>>> +}
>>> +
>>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
>>> + void *from_d, int idx, long argl, void *argp)
>>> +{
>>> + /* strdup it */
>>> + return 0;
>>> +}
>>> +
>>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
>>> +{
>>> + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
>>> SSL_get_ex_data_X509_STORE_CTX_idx());
>>> + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
>>> + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
>>> +
>>> + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
>>> + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
>>> +
>>> + return X509_verify_cert(ctx);
>>> +}
>>> +
>>> /**
>>>Initializes the OpenSSL library.
>>>
>>> @@ -40,6 +83,9 @@ TlsInitialize (
>>> return FALSE;
>>>}
>>>
>>> + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
>>> + ssl_target_dup, ssl_target_free);
>>> +
>>>//
>>>// Initialize the pseudorandom number generator.
>>>//
>>> @@ -106,6 +152,10 @@ TlsCtxNew (
>>>//
>>>SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
>>>
>>> + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
>>> + * we could have done as SSL_set_verify(). Twisty maze, remember? */
>>> + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
>>> +
>>>return (VOID *) TlsCtx;
>>> }
>
>> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
>> already been stored for the same index?
>>
>> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
>> it be automatically freed with "free_func"?
>>
>> (Note: I think that, if we used a "new_func" for allocating anything,
>> this question could be relevant the very first time SSL_set_ex_data()
>> were called.)
>
> A similar question:
>
> is it possible that app_verify_callback() is called more frequently than
> SSL_set_ex_data() (in TlsSetVerify())?
>
> Because that means that the frequency of SSL_set1_host() calls changes.
> Previously we'd call
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/15/19 17:57, David Woodhouse wrote:
> On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote:
>> (I can't test it easily myself, as I don't even know how to create a
>> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)
>
> I had to look it up again, but here goes...
>
> $ cat v3.ext
> subjectAltName = @alt_names
> [alt_names]
> DNS.1 = lersek-test.redhat.com
> IP.2 = 192.168.124.2
> IP.3 = fd33:eb1b:9b36::2
> $ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr
> ...
> $ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem
> -extfile v3.ext
I'm not familiar with this x509 invocation ("-signkey").
Thus far I've used x509 to sign self-signed certificate requests with a
CA key:
openssl x509 -req -in request.csr -out signedcert.pem \
-CA ca-cert.pem -CAkey ca-key.pem [-CAcreateserial]
I guess "-signkey ca-key.pem" is a shorthand for the (-CA, -CAkey) pair?
(I've tried to look at the manual; I couldn't say I'm wiser now.)
Either way: why do we add the subject alternative names when the CA
signs the request? Shouldn't the *original* certificate request state
what alternative names can stand for the same subject?
(I don't even understand how a CA can usefully insert such an extension;
after all, it cannot be signed by the original certificate requestor!)
The "openssl req" command too seems to accept "-extensions" -- why are
we not required to use that? To me it seems like the only acceptable
place, to add alternative names.
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49025): https://edk2.groups.io/g/devel/message/49025
Mute This Topic: https://groups.io/mt/34307578/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/15/19 15:54, Laszlo Ersek wrote:
> On 10/15/19 13:03, David Woodhouse wrote:
>> The "app callback" in my OpenConnect example is set on the SSL_CTX not
>> the SSL object, and is called from the top-level
>> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
>>
>> It is X509_verify_cert() which can do the hostname/IP checks for us, if
>> we can only tell it that we want it to. But the X509_VERIFY_PARAM
>> object is private to the SSL.
>>
>> As discussed, we have the SSL_set1_host() accessor function which lets
>> us set the hostname. The implementation really is a simple one-liner,
>> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
>> for use to set the IP address from the outside, without an equivalent
>> accessor function for that (and without SSL_set1_host() spotting that
>> the string it's given is an IP address, and doing so).
>>
>> But what we can do is stash the target string in some ex_data hanging
>> off the SSL object, then have an app callback — which *can* reach the
>> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
>> normal X509_verify_cert() function that it has overridden.
>>
>> Something like this... and instead of calling SSL_set1_host(ssl, host)
>> your own code now has to call
>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
>>
>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c
>> b/CryptoPkg/Library/TlsLib/TlsInit.c
>> index f9ad6f6b946c..add5810cc4bd 100644
>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> #include "InternalTlsLib.h"
>>
>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
>> + * alike. All we really wanted to do was call SSL_set1_host() and
>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
>> + * for us. But until we update OpenSSL, that doesn't work. And we
>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
>> + * for ourselves.
>> + *
>> + * So we install an app_verify_callback in the SSL_CTX (which is
>> + * different to the per-SSL callback wae can use, because it happens
>> + * sooner. All our callback does it set the hostname or IP address in
>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
>> + * then call X509_verify_param() which is the default function.
>> + *
>> + * How does it find the hostname/IP string? It's attached to the SSL
>> + * as ex_data, using this index:
>> + */
>> +static int ssl_target_idx;
>> +
>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
>> +int idx, long argl, void *argp)
>> +{
>> +/* Free it */
>> +}
>> +
>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
>> + void *from_d, int idx, long argl, void *argp)
>> +{
>> +/* strdup it */
>> +return 0;
>> +}
>> +
>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
>> +{
>> +SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
>> SSL_get_ex_data_X509_STORE_CTX_idx());
>> +char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
>> +X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
>> +
>> +if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
>> +X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
>> +
>> +return X509_verify_cert(ctx);
>> +}
>> +
>> /**
>>Initializes the OpenSSL library.
>>
>> @@ -40,6 +83,9 @@ TlsInitialize (
>> return FALSE;
>>}
>>
>> + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
>> +ssl_target_dup, ssl_target_free);
>> +
>>//
>>// Initialize the pseudorandom number generator.
>>//
>> @@ -106,6 +152,10 @@ TlsCtxNew (
>>//
>>SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
>>
>> + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
>> + * we could have done as SSL_set_verify(). Twisty maze, remember? */
>> + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
>> +
>>return (VOID *) TlsCtx;
>> }
> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
> already been stored for the same index?
>
> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
> it be automatically freed with "free_func"?
>
> (Note: I think that, if we used a "new_func" for allocating anything,
> this question could be relevant the very first time SSL_set_ex_data()
> were called.)
A similar question:
is it possible that app_verify_callback() is called more frequently than
SSL_set_ex_data() (in TlsSetVerify())?
Because that means that the frequency of SSL_set1_host() calls changes.
Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it
could be called multiple times per TlsSetVerify(). Is that the case?
If it is, is it OK?
To me the ownership
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote: > (I can't test it easily myself, as I don't even know how to create a > server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) I had to look it up again, but here goes... $ cat v3.ext subjectAltName = @alt_names [alt_names] DNS.1 = lersek-test.redhat.com IP.2 = 192.168.124.2 IP.3 = fd33:eb1b:9b36::2 $ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr ... $ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem -extfile v3.ext Signature ok subject=C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Getting Private key $ openssl x509 -in cert.pem -noout -text Certificate: Data: Version: 3 (0x2) Serial Number: 56:c5:33:0f:b1:2d:e5:b5:1e:89:e5:a7:a2:45:a9:06:43:1f:4a:1e Signature Algorithm: sha256WithRSAEncryption Issuer: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Validity Not Before: Oct 15 15:56:11 2019 GMT Not After : Oct 12 15:56:11 2029 GMT Subject: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd Subject Public Key Info: Public Key Algorithm: rsaEncryption RSA Public-Key: (2432 bit) Modulus: 00:b4:6b:27:98:25:af:c1:ff:1e:ca:b0:7e:f4:d8: bc:ed:43:86:67:54:5d:da:b4:1e:c2:90:5f:83:3c: 02:11:fc:13:72:85:b2:88:a4:65:41:0b:76:5f:23: be:8a:9f:fe:79:4b:73:3b:2e:c7:4b:3c:bf:16:c9: 97:55:35:17:f3:a1:72:4b:30:c2:e0:27:94:12:f3: 56:00:e6:ce:82:4b:11:5d:a4:1e:9b:fa:fa:b9:1b: 2a:4d:18:b5:ba:a5:e6:0c:c7:a8:a8:a1:6d:aa:88: 84:dc:96:0e:b2:6c:1c:35:aa:e7:c7:94:3d:f9:d5: c7:c2:a2:0d:4b:b3:6e:7a:f7:08:5f:c5:09:cd:15: 93:1a:f7:98:df:2a:4c:66:89:24:ed:1f:d0:16:63: 81:65:a5:58:3b:a1:cd:25:62:9b:99:81:54:08:17: 18:ec:7c:2f:08:a2:3b:28:57:32:9d:17:47:0a:86: fb:62:b1:41:99:e6:fb:de:a8:ea:20:7e:f3:1b:ee: ba:ea:9a:21:64:29:92:f2:ad:73:e5:19:05:9d:37: 53:e2:11:9f:18:5f:22:ba:e2:8b:0d:00:8c:9e:2f: a7:87:3d:40:be:4a:a2:a5:92:08:0c:2e:61:c0:58: 7c:9a:99:e1:d6:ac:83:39:25:cf:3e:1b:ed:eb:a3: 6d:9d:cb:c5:38:de:c1:c7:6e:9b:34:14:be:30:3e: 82:90:1e:b9:4a:9a:76:e4:ef:33:0c:46:a2:31:72: f6:c3:61:0b:f8:aa:67:89:f4:a5:e5:76:37:a1:29: 9f:80:79:aa:75 Exponent: 65537 (0x10001) X509v3 extensions: X509v3 Subject Alternative Name: DNS:lersek-test.redhat.com, IP Address:192.168.124.2, IP Address:FD33:EB1B:9B36:0:0:0:0:2 Signature Algorithm: sha256WithRSAEncryption 37:8c:17:6c:4d:5f:05:b6:70:b9:96:49:0a:e3:f6:3c:bd:3b: d0:fe:56:ee:ad:58:15:6e:a6:79:a8:3b:d4:fa:09:f9:7d:85: 8a:8b:14:7b:e4:db:bf:2d:8d:32:28:26:d6:37:a5:51:90:e9: 75:25:b9:9d:63:db:35:29:8a:58:61:56:b2:2a:5a:d3:80:b7: 1d:4c:05:0b:49:da:6f:ec:67:f5:3d:09:f2:58:92:43:8d:39: d7:f4:f3:3c:bd:9b:16:a2:c9:c0:63:5d:c9:1a:c3:a7:24:fa: 31:8c:7c:3e:98:98:87:8f:5b:fb:00:f5:41:15:16:89:c6:e3: c4:63:3a:3d:3e:b8:b5:b7:af:cb:11:1a:13:f4:b2:df:c4:f4: a1:a2:9c:d1:05:20:84:65:70:91:41:be:f4:26:c2:63:07:46: d0:63:bf:27:3f:42:9c:69:22:e1:d6:6a:41:dc:97:51:2d:ef: a1:11:20:ed:89:57:d6:d2:ad:6c:7f:88:69:ae:31:51:e8:cb: 9e:3a:e1:49:48:01:5b:d5:ab:93:53:5e:cb:2f:72:6e:84:af: d0:c2:91:41:29:6f:3c:0b:df:c6:9c:77:14:fd:29:fc:65:0b: 2d:6c:61:69:a6:72:19:38:5f:a1:83:fd:6c:22:02:d7:b6:81: 9e:05:7c:58:2c:c9:eb:c0:09:aa:07:d1:b7:15:a1:e3:ea:27: b1:f7:70:87:fe:d6:16:57:67:70:fe:65:9a:0f:1b:11:be:22: 08:2f:21:50:30:a4:35:99:d3:fb:4d:40:22:39:2c:f3 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#49016): https://edk2.groups.io/g/devel/message/49016 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Tue, 2019-10-15 at 15:54 +0200, Laszlo Ersek wrote:
> On 10/15/19 13:03, David Woodhouse wrote:
> > On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> > > 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
> >
> >
> > Hm, you are lost in a twisty maze of verify callbacks, all alike.
>
> Definitely.
>
> > Actually the one you can set with SSL_set_verify() isn't the one you
> > want. That's a low-level one, called from within the generic
> > X509_verify_cert() function.
>
> Well, above I referred to SSL_set_verify() only because you had named
> that function in :
Yeah, I was wrong.
You were the one who called me an expert; I didn't :)
> Thank you very much for this code.
>
> To me it seems like this patch should be squashed into patch#2 (which
> modifies TlsLib), after adapting it to the edk2 coding style.
>
> I have some comments / questions.
>
>
> (1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be
> replaced with SSL_set_ex_data(). OK.
>
> In case the client does not use the TlsSetVerifyHost() function,
> SSL_set_ex_data() would not be called. However, app_verify_callback()
> would still be called.
>
> Is my understanding correct that "hostname" (from SSL_get_ex_data())
> would be NULL in that case, and therefore both
> X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would
> be omitted?
>
> (I mean this looks like the correct behavior to me; just asking if that
> is indeed the intent of the code.)
Yep. In the case where there's no such ex_data attached to the SSL
object, our own app_verify_callback() function would do nothing
special, just call through to the standard X509_verify_cert() function
that would have been called if we hadn't registered our own override.
You may call this "correct". I think you're right, in the context you
meant it.
But I suggest there is a conversation to be had about whether it's ever
"correct" for a crypto library to silently accept any bloody
certificate it sees, regardless of what the Subject of that certificate
is.
Or whether perhaps it ought to fail safe, and require you to jump
through extra hoops if you *really* want to accept any certificate.
In a world where crypto libraries made it hard for the users to Do The
Wrong Thing, CVE-2019-14553 would never have happened.
But we digress. Is it late enough in the day for me to start drinking
yet?
> (2) The documentation of the APIs seems a bit strange. I've found the
> following "directory" web pages:
>
> https://www.openssl.org/docs/man1.0.2/man3/
> https://www.openssl.org/docs/man1.1.0/man3/
> https://www.openssl.org/docs/man1.1.1/man3/
>
> edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only
> look at the last page. However, SSL_get_ex_new_index() is only
> documented in the first page.
>
> Is my understanding correct that each OpenSSL API should be looked up in
> the latest documentation directory that appears to describe it?
>
> In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1
> manual, and fail to find it, does that mean that the API is deprecated,
> or only that I should look at an earlier release of the manual?
I was just looking at the code. I happened to be looking at OpenSSL
HEAD but the sconnect demo I showed in my second email works with
1.1.1.
>
> (3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around
> CRYPTO_get_ex_new_index().
>
> https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html
> https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html
>
> Based on that, I think the second ("argp") parameter of our
> SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of
> the "free" and "dup" functions are unused.
>
> Do you agree?
Yeah, I had just cargo-culted that from elsewhere, and was assuming it
was performing some kind of de-duplication or alternative lookup
functionality. It does look like you can leave it NULL and it doesn't
matter.
>
> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
> already been stored for the same index?
>
> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
> it be automatically freed with "free_func"?
It will be automatically freed with free_func. Note that this
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/15/19 13:03, David Woodhouse wrote:
> On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
>> 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
>
>
> Hm, you are lost in a twisty maze of verify callbacks, all alike.
Definitely.
> Actually the one you can set with SSL_set_verify() isn't the one you
> want. That's a low-level one, called from within the generic
> X509_verify_cert() function.
Well, above I referred to SSL_set_verify() only because you had named
that function in :
> Note: the callback you're after is the final argument to
> SSL_set_verify(), which you are calling from TlsSetVerify() with a
> NULL argument. You need to point it at your own function of type
> SSL_verify_cb which does the checking correctly.
Back to the email:
On 10/15/19 13:03, David Woodhouse wrote:
> The "app callback" in my OpenConnect example is set on the SSL_CTX not
> the SSL object, and is called from the top-level
> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
>
> It is X509_verify_cert() which can do the hostname/IP checks for us, if
> we can only tell it that we want it to. But the X509_VERIFY_PARAM
> object is private to the SSL.
>
> As discussed, we have the SSL_set1_host() accessor function which lets
> us set the hostname. The implementation really is a simple one-liner,
> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
> for use to set the IP address from the outside, without an equivalent
> accessor function for that (and without SSL_set1_host() spotting that
> the string it's given is an IP address, and doing so).
>
> But what we can do is stash the target string in some ex_data hanging
> off the SSL object, then have an app callback — which *can* reach the
> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
> normal X509_verify_cert() function that it has overridden.
>
> Something like this... and instead of calling SSL_set1_host(ssl, host)
> your own code now has to call
> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
>
> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c
> b/CryptoPkg/Library/TlsLib/TlsInit.c
> index f9ad6f6b946c..add5810cc4bd 100644
> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #include "InternalTlsLib.h"
>
> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
> + * alike. All we really wanted to do was call SSL_set1_host() and
> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
> + * for us. But until we update OpenSSL, that doesn't work. And we
> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
> + * for ourselves.
> + *
> + * So we install an app_verify_callback in the SSL_CTX (which is
> + * different to the per-SSL callback wae can use, because it happens
> + * sooner. All our callback does it set the hostname or IP address in
> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
> + * then call X509_verify_param() which is the default function.
> + *
> + * How does it find the hostname/IP string? It's attached to the SSL
> + * as ex_data, using this index:
> + */
> +static int ssl_target_idx;
> +
> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
> + int idx, long argl, void *argp)
> +{
> + /* Free it */
> +}
> +
> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
> +void *from_d, int idx, long argl, void *argp)
> +{
> + /* strdup it */
> + return 0;
> +}
> +
> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
> +{
> + SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
> SSL_get_ex_data_X509_STORE_CTX_idx());
> + char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
> + X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
> +
> + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
> + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
> +
> + return X509_verify_cert(ctx);
> +}
> +
> /**
>Initializes the OpenSSL library.
>
> @@ -40,6 +83,9 @@ TlsInitialize (
> return FALSE;
>}
>
> + ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
> +
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Tue, 2019-10-15 at 12:03 +0100, David Woodhouse wrote:
>
> Something like this... and instead of calling SSL_set1_host(ssl, host)
> your own code now has to call
> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
Here's how I tested that in the OpenSSL tree in userspace, FWIW...
diff --git a/demos/bio/sconnect.c b/demos/bio/sconnect.c
index 7e46bf0ad8..1fd2829a79 100644
--- a/demos/bio/sconnect.c
+++ b/demos/bio/sconnect.c
@@ -25,11 +25,40 @@
#define HOSTPORT "localhost:4433"
#define CAFILE "root.pem"
+static int ssl_target_idx;
+
+void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
+ int idx, long argl, void *argp)
+{
+ printf("Freeing %s\n", ptr);
+ free(ptr);
+}
+
+int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
+ void *from_d, int idx, long argl, void *argp)
+{
+ /* strdup it */
+ return 0;
+}
+
+int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
+{
+ SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
+ char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
+ X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
+
+ printf("Setting %s\n", hostname);
+ if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
+ X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
+
+ return X509_verify_cert(ctx);
+}
+
int main(int argc, char *argv[])
{
const char *hostport = HOSTPORT;
const char *CAfile = CAFILE;
-char *hostname;
+const char *hostname;
char *cp;
BIO *out = NULL;
char buf[1024 * 10], *p;
@@ -43,10 +72,6 @@ int main(int argc, char *argv[])
if (argc > 2)
CAfile = argv[2];
-hostname = OPENSSL_strdup(hostport);
-if ((cp = strchr(hostname, ':')) != NULL)
-*cp = 0;
-
#ifdef WATT32
dbug_init();
sock_init();
@@ -54,6 +79,8 @@ int main(int argc, char *argv[])
ssl_ctx = SSL_CTX_new(TLS_client_method());
+SSL_CTX_set_cert_verify_callback(ssl_ctx, app_verify_callback, NULL);
+
/* Enable trust chain verification */
SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, NULL);
SSL_CTX_load_verify_locations(ssl_ctx, CAfile, NULL);
@@ -62,9 +89,6 @@ int main(int argc, char *argv[])
ssl = SSL_new(ssl_ctx);
SSL_set_connect_state(ssl);
-/* Enable peername verification */
-if (SSL_set1_host(ssl, hostname) <= 0)
-goto err;
/* Use it inside an SSL BIO */
ssl_bio = BIO_new(BIO_f_ssl());
@@ -73,6 +97,13 @@ int main(int argc, char *argv[])
/* Lets use a connect BIO under the SSL BIO */
out = BIO_new(BIO_s_connect());
BIO_set_conn_hostname(out, hostport);
+
+/* The BIO has parsed the host:port and even IPv6 literals in [] */
+hostname = BIO_get_conn_hostname(out);
+
+if (hostname)
+ SSL_set_ex_data(ssl, ssl_target_idx, strdup(hostname));
+
BIO_set_nbio(out, 1);
out = BIO_push(ssl_bio, out);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49004): https://edk2.groups.io/g/devel/message/49004
Mute This Topic: https://groups.io/mt/34307578/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
smime.p7s
Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> 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
Hm, you are lost in a twisty maze of verify callbacks, all alike.
Actually the one you can set with SSL_set_verify() isn't the one you
want. That's a low-level one, called from within the generic
X509_verify_cert() function.
The "app callback" in my OpenConnect example is set on the SSL_CTX not
the SSL object, and is called from the top-level
ssl_verify_cert_chain() function *instead* of X509_verify_cert().
It is X509_verify_cert() which can do the hostname/IP checks for us, if
we can only tell it that we want it to. But the X509_VERIFY_PARAM
object is private to the SSL.
As discussed, we have the SSL_set1_host() accessor function which lets
us set the hostname. The implementation really is a simple one-liner,
calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
for use to set the IP address from the outside, without an equivalent
accessor function for that (and without SSL_set1_host() spotting that
the string it's given is an IP address, and doing so).
But what we can do is stash the target string in some ex_data hanging
off the SSL object, then have an app callback — which *can* reach the
underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
normal X509_verify_cert() function that it has overridden.
Something like this... and instead of calling SSL_set1_host(ssl, host)
your own code now has to call
SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c
b/CryptoPkg/Library/TlsLib/TlsInit.c
index f9ad6f6b946c..add5810cc4bd 100644
--- a/CryptoPkg/Library/TlsLib/TlsInit.c
+++ b/CryptoPkg/Library/TlsLib/TlsInit.c
@@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "InternalTlsLib.h"
+/* You are lost in a twisty maze of SSL cert verify callbacks, all
+ * alike. All we really wanted to do was call SSL_set1_host() and
+ * have it work for IP addresses too, which OpenSSL PR#9201 will do
+ * for us. But until we update OpenSSL, that doesn't work. And we
+ * can't get at the underlying X509_VERIFY_PARAM to set the IP address
+ * for ourselves.
+ *
+ * So we install an app_verify_callback in the SSL_CTX (which is
+ * different to the per-SSL callback wae can use, because it happens
+ * sooner. All our callback does it set the hostname or IP address in
+ * the X509_VERIFY_PARAM like we wanted to in the first place, and
+ * then call X509_verify_param() which is the default function.
+ *
+ * How does it find the hostname/IP string? It's attached to the SSL
+ * as ex_data, using this index:
+ */
+static int ssl_target_idx;
+
+void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
+ int idx, long argl, void *argp)
+{
+ /* Free it */
+}
+
+int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
+ void *from_d, int idx, long argl, void *argp)
+{
+ /* strdup it */
+ return 0;
+}
+
+int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
+{
+ SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
+ char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
+ X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
+
+ if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
+ X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
+
+ return X509_verify_cert(ctx);
+}
+
/**
Initializes the OpenSSL library.
@@ -40,6 +83,9 @@ TlsInitialize (
return FALSE;
}
+ ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
+ ssl_target_dup, ssl_target_free);
+
//
// Initialize the pseudorandom number generator.
//
@@ -106,6 +152,10 @@ TlsCtxNew (
//
SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
+ /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
+ * we could have done as SSL_set_verify(). Twisty maze, remember? */
+ SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
+
return (VOID *) TlsCtx;
}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49002): https://edk2.groups.io/g/devel/message/49002
Mute This Topic:
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> My understanding is that a fix purely in edk2 -- that is, without
> advancing our openssl submodule reference at once
Haha, I love the fact that I am hoist by my own petard on patching
OpenSSL. I evidently did such a good job of upstreaming all the quirks
we need for EDK2, that we're now *incapable* of carrying any local
patches to OpenSSL.
I'll take that as a win, I suppose :)
> -- 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.
That's harder than it needs to be; it's the version for OpenSSL < 1.0.2
where they made the users jump through *lots* of hoops to validate
certs correctly. These days it's much easier; you only need the version
at
http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1369
which is called from the actual callback at
http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1507
I'll see if I can throw something together for you at least as an
example.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48916): https://edk2.groups.io/g/devel/message/48916
Mute This Topic: https://groups.io/mt/34307578/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
smime.p7s
Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/14/19 18:15, Laszlo Ersek wrote: > 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? ... to clarify, in this case, the upstream edk2 project should *not* claim to have fixed CVE-2019-14553, until the reminder BZ is also closed! The new BZ should actually block TianoCore#960. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48914): https://edk2.groups.io/g/devel/message/48914 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
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
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
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 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 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. 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. 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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48843): https://edk2.groups.io/g/devel/message/48843 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
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 >> >> 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
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
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 > > 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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48820): https://edk2.groups.io/g/devel/message/48820 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Fri, 2019-10-11 at 02: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(). I cannot coherently express how disappointed I am by this response. The current state is that EDK2 doesn't check the subject of the certificate at all. We're trying to fix that, and you have expended more effort typing in poor excuses for doing an incomplete job, than the typing it would have taken just to get it right in the first place. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48800): https://edk2.groups.io/g/devel/message/48800 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
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. Thanks, Jiaxin > -Original Message- > From: Laszlo Ersek > Sent: Friday, October 11, 2019 2:04 AM > To: David Woodhouse ; Wu, Jiaxin > ; devel@edk2.groups.io; Wang, Jian J > ; Bret Barkelew > Cc: Richard Levitte > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On 10/10/19 17:45, David Woodhouse wrote: > > On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: > >>> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, > OU=IPv6 cert, CN=fd33:eb1b:9b36::2 > > > > Yeah, you're not actually testing the case I'm talking about. You want > > a GEN_IP in the x509v3 Subject Alternative Name. > > > > Compare with... > > > > $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl > x509 -noout -text | grep -A1 Alternative > > X509v3 Subject Alternative Name: > > DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP > Address:134.191.232.101 > > > > $ curl https://134.191.232.101/ > > > > OK, thank you. > > I can imagine two failure modes, with the patches applied. > > (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server > certificate. > > (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid > (mismatched) server certificate. > > Can we tell which failure mode applies? > > (I can't test it easily myself, as I don't even know how to create a > server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) > > Case (2) is clearly bad, and it would be a sign that the patch series > does not fully fix the issue. > > Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is > pretty rare in practice. Thus regressing it (perhaps temporarily) should > be an acceptable trade-off for fixing the current gaping hole (= subject > name not checked at all). > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48777): https://edk2.groups.io/g/devel/message/48777 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/10/19 17:45, David Woodhouse wrote: > On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: >>> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, >>> OU=IPv6 cert, CN=fd33:eb1b:9b36::2 > > Yeah, you're not actually testing the case I'm talking about. You want > a GEN_IP in the x509v3 Subject Alternative Name. > > Compare with... > > $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl > x509 -noout -text | grep -A1 Alternative > X509v3 Subject Alternative Name: > DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP > Address:134.191.232.101 > > $ curl https://134.191.232.101/ > OK, thank you. I can imagine two failure modes, with the patches applied. (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server certificate. (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid (mismatched) server certificate. Can we tell which failure mode applies? (I can't test it easily myself, as I don't even know how to create a server certificate with a SAN -- any kind of SAN, let alone GEN_IP.) Case (2) is clearly bad, and it would be a sign that the patch series does not fully fix the issue. Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is pretty rare in practice. Thus regressing it (perhaps temporarily) should be an acceptable trade-off for fixing the current gaping hole (= subject name not checked at all). Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48751): https://edk2.groups.io/g/devel/message/48751 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote: > > Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, > > OU=IPv6 cert, CN=fd33:eb1b:9b36::2 Yeah, you're not actually testing the case I'm talking about. You want a GEN_IP in the x509v3 Subject Alternative Name. Compare with... $ openssl s_client -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl x509 -noout -text | grep -A1 Alternative X509v3 Subject Alternative Name: DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP Address:134.191.232.101 $ curl https://134.191.232.101/ -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48738): https://edk2.groups.io/g/devel/message/48738 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/09/19 22:34, David Woodhouse wrote:
> Can you show result of 'openssl x509 -noout -text -in xx.pem' on
> your certs please.
Sure. I had thought of that actually (I could have attached the
certificates at once), but I figured, let me not share crypto stuff
unless specifically asked for :)
> Would like to check if you really have a cert for the hostname string
> "192.168.124.2" or to the IP address. They are different things.
Very interesting! This makes me curious.
The *host* certificates were generated with the "genkey" utility ("Red
Hat Keypair Generation"), not with naked openssl tool invocations.
That's because Apache setup is not for the faint-hearted, and I had
decided up-front (when I first looked into setting up mod_ssl) that I'd
follow the official documentation, here:
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers
So I ran
genkey 'fd33:eb1b:9b36::2'
genkey 192.168.124.2
and went through the dialogs, which were already filled in with the
argument passed on the command line (which the genkey manual calls
"hostname").
The *CA* cert was generated with openssl directly, however. Also, for
signing the CSRs (certificate signing requests), produced by "genkey", I
also used openssl manually.
Please find the certificates below (including the CA certificate):
> Certificate:
> Data:
> Version: 3 (0x2)
> Serial Number:
> a7:b5:04:75:6a:2f:ee:7e
> Signature Algorithm: sha256WithRSAEncryption
> Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
> OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=ler...@redhat.com
> Validity
> Not Before: Oct 9 16:06:08 2019 GMT
> Not After : Nov 8 16:06:08 2019 GMT
> Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
> OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=ler...@redhat.com
> Subject Public Key Info:
> Public Key Algorithm: rsaEncryption
> Public-Key: (2048 bit)
> Modulus:
> 00:d4:db:3b:fa:98:bd:15:02:3a:32:ea:64:d5:1d:
> d8:80:03:fa:fb:4e:d8:47:45:3b:57:6a:36:80:83:
> e1:6d:c4:0b:f7:24:00:ad:54:63:77:dd:86:71:f3:
> fc:f4:e5:81:d4:6c:7d:23:b9:58:9e:cc:93:ee:93:
> ed:24:62:8b:94:8f:de:3a:6a:a9:cd:1a:38:f0:df:
> 17:91:7a:22:ca:35:94:3a:1f:cb:56:97:be:bb:69:
> 1f:12:3a:7c:a4:35:6c:15:0e:27:a0:0a:2b:3d:61:
> bd:ed:a6:84:29:bc:0a:d8:0c:98:4c:e9:7d:6b:36:
> da:29:1d:49:57:8d:89:2a:62:e7:4f:00:56:46:9b:
> a7:21:9f:b6:83:c7:dd:69:b0:8b:a3:bf:fa:ef:50:
> a4:05:6f:b1:87:83:87:93:c7:ba:0e:cb:50:28:05:
> f5:a7:0a:fe:be:13:71:1a:4b:6c:7b:f6:c9:cc:b2:
> 65:cd:62:29:e3:08:c7:da:5c:ca:4d:dd:74:a4:d1:
> 37:52:ea:ad:72:87:cf:48:f3:85:af:15:ab:e8:dc:
> 50:f2:f7:7a:59:ed:b7:69:4f:a2:55:39:e5:ae:09:
> 75:45:69:a5:3a:a0:cd:52:ee:f2:15:d5:3c:fc:c0:
> 00:b7:25:71:ba:00:ba:63:08:8a:fc:b6:af:94:a4:
> 5b:cb
> Exponent: 65537 (0x10001)
> X509v3 extensions:
> X509v3 Subject Key Identifier:
> 2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5
> X509v3 Authority Key Identifier:
>
> keyid:2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5
>
> X509v3 Basic Constraints:
> CA:TRUE
> Signature Algorithm: sha256WithRSAEncryption
> 5f:fb:ca:7c:52:c6:81:f3:3f:48:02:c6:31:64:dc:a5:2f:a6:
> 83:ef:b2:9d:68:7f:1a:e6:1d:8d:e0:50:08:ed:96:4f:56:8a:
> eb:cd:3a:ac:74:f6:e4:68:55:0d:73:b3:bf:45:cd:35:e0:4b:
> 70:bf:25:30:75:62:e1:5a:53:00:ff:ca:3c:c0:86:ad:44:73:
> 5b:64:d8:85:ea:32:38:3b:4b:60:85:95:e5:10:f3:92:19:0e:
> 55:67:26:c5:56:50:92:8c:d2:33:5b:5f:c6:27:c1:9a:6a:a5:
> ad:1b:21:04:47:ce:94:f0:2a:38:26:43:5f:9b:c0:f5:33:80:
> 59:72:33:e8:06:89:e1:7e:44:5c:cb:67:fa:f4:de:27:94:9c:
> 44:1d:81:40:6f:46:bc:2f:93:89:30:48:99:bc:53:29:44:a7:
> e1:9d:9c:05:98:14:3d:ab:e2:1c:3e:91:83:4a:74:9d:ed:14:
> d4:86:2d:c0:4e:4e:20:fc:29:31:60:b9:63:50:23:52:b1:2f:
> 9f:50:b4:d7:23:3e:08:c2:b3:bf:d3:b6:84:16:6d:71:fe:2a:
> b0:71:f0:94:67:84:1d:45:8d:22:3d:4a:f4:65:73:0c:81:8a:
> 4e:b7:52:b8:21:ab:ce:8d:50:e0:22:af:4f:2e:30:4f:95:8c:
> 9c:26:0d:fe
> Certificate:
> Data:
> Version: 1 (0x0)
> Serial Number:
> d1:ea:de:57:cb:4b:44:7b
> Signature Algorithm: sha256WithRSAEncryption
> Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
> OU=Certificate Authority, CN=Laszlo Ersek
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
Hi Laszlo, Thanks the comments. Best Regards! Jiaxin > -Original Message- > From: Laszlo Ersek > Sent: Wednesday, October 9, 2019 11:55 PM > To: devel@edk2.groups.io; Wang, Jian J ; Wu, Jiaxin > ; David Woodhouse ; Bret > Barkelew > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On 10/01/19 01:21, Laszlo Ersek wrote: > > On 09/29/19 08:09, Wang, Jian J wrote: > >> For this patch series, > >> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not > needed any more. > >> Remove it at push time and no need to send a v2. > >> 2. Since it's security patch which had been reviewed separately, I see no > reason for new r-b > >> required. Please raise it asap if any objections. > >> 3. Acked-by: Jian J Wang > > > > > > * Can you please confirm that these patches match those that we > > discussed here: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > To answer my own question, I've now compared the patches from those BZ > comments linked above, with the present series. Here's a list of > differences. > > (1) The subject lines now include the reference "(CVE-2019-14553)". > > This is great, *but* please be sure to insert a space character before > the opening parenthesis! (In every patch.) > > (2) The commit messages reference both the BZ and the CVE number. > > Good. > > (3) In the commit messages, the line > > Contributed-under: TianoCore Contribution Agreement 1.0 > > has been upgraded to > > Contributed-under: TianoCore Contribution Agreement 1.1 > > I think this is wrong. The lines should have been removed, due to the > SPDX adoption. Please update all the commit messages. > > (4) Copyright notice updates are gone from the patches. > > That's fine: the reason is that the underlying files have seen their > copyright notices updated, meanwhile. > > > Otherwise, the patches (code, commit messages, and feedback tags) are > identical. > > Before you push the patches (or post a v2), please fix issues (1) and (3). > > Now, regarding the other set of questions: > > > * In the BZ, David and Bret raised some questions: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > and > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > The latest comment in the bug is c#41. I'm not under the impression that > > all concerns raised by David and Bret have been addressed (or > > abandoned). I'd like David and Bret to ACK the patches. > > I'll first have to process the new comments down-thread. > > Thanks > Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48684): https://edk2.groups.io/g/devel/message/48684 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
> > I have not tested this, but I started looking when there was a message > on the edk2 list from someone who was reporting that it didn't work for > IPv6 URIs, IIRC. > > You are using SSL_set1_host(), and I believe you're just passing in the > bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]". Here, I want to highlight is that 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. > > That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the > SSL connection. Yes. > > In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the > code simply iterates over the members of that list, calling > X509_check_host() for each one. It never calls X509_check_ip(). Yes. > > If you look in openssl/crypto/x509/v3_utl.c you can see the > X509_check_host() really does only check hostnames. Yes. > You'd need to call X509_check_ip_asc() to check hostnames. And something > would need to > have stripped the [] which surround an IPv6 literal. > Disagree, why need check the IP here since we only focus on the hostname verification? For HttpDxe driver, it's the implementation choice to treat the IP in URI as hostname string format. As I said before in the email, if the CN or SAN (Seems only in X509 V3) in the certificate are set correctly, it should be OK to pass the verification. Laszlo and I already have verified that. > I can't see how this can work. Have you tested it since the report on > the list that it wasn't working? > Sorry, I can't remember there is any failure of Ipv6 URI reported from edk2. If you can find it, that will be better. > cf. https://github.com/openssl/openssl/pull/9201 which is being ignored > by the OpenSSL developers — OpenSSL really doesn't make life easy for > you here, which is a shame. > > > > For the series patches here, we are intending to support the host > > name validation, I think we can commit the series patches since we > > pass the verification of IPV6 URL, what do you think? > > If it passes the verification of IPv6 literals, then all my analysis is > broken and so was the report on the list that prompted me to start > looking (or I'm misremembering that report). In that case, sure, go > ahead and commit. > > > Thanks, > > Jiaxin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48683): https://edk2.groups.io/g/devel/message/48683 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
Can you show result of 'openssl x509 -noout -text -in xx.pem' on your certs please. Would like to check if you really have a cert for the hostname string "192.168.124.2" or to the IP address. They are different things. On 9 October 2019 21:24:34 BST, Laszlo Ersek wrote: >Hi All, > >(multi-hour composition ahead...) > >On 10/09/19 09:53, David Woodhouse wrote: >> On Tue, 2019-10-08 at 06:19 +, Wu, Jiaxin wrote: >>> Hi David, >>> >>> I just realized you have the comments on Bugzilla 960: >>> "...given that testing is failing and code inspection shows it would never have been expected to work." >>> >>> Do you mean you didn't pass the verification if URLs with IPv6 >>> literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me >>> where the code inspection indicated it would never have been >expected >>> to work? We do pass the testing for the URLs with IPv6 if the CN or >>> SAN in certificate has the corresponding IPv6 address (at least >>> working with openssl 1.1.0). >> >> I have not tested this, but I started looking when there was a >message >> on the edk2 list from someone who was reporting that it didn't work >> for IPv6 URIs, IIRC. >> >> You are using SSL_set1_host(), and I believe you're just passing in >> the bare hostname part of the URI, be it "1.2.3.4" or >> "[2001:8b0:10b::5]". >> >> That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for >the >> SSL connection. >> >> In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the >> code simply iterates over the members of that list, calling >> X509_check_host() for each one. It never calls X509_check_ip(). >> >> If you look in openssl/crypto/x509/v3_utl.c you can see the >> X509_check_host() really does only check hostnames. You'd need to >call >> X509_check_ip_asc() to check hostnames. And something would need to >> have stripped the [] which surround an IPv6 literal. >> >> I can't see how this can work. Have you tested it since the report on >> the list that it wasn't working? >> >> cf. https://github.com/openssl/openssl/pull/9201 which is being >> ignored by the OpenSSL developers OpenSSL really doesn't make >> life easy for you here, which is a shame. >> >> >>> For the series patches here, we are intending to support the host >>> name validation, I think we can commit the series patches since we >>> pass the verification of IPV6 URL, what do you think? >> >> If it passes the verification of IPv6 literals, then all my analysis >> is broken and so was the report on the list that prompted me to start >> looking (or I'm misremembering that report). In that case, sure, go >> ahead and commit. > >Here's a summary of my setup. > >* I've generated a brand new CA certificate, and two HTTP server > certificates, signed by the CA. > >* One HTTP server certificate is for Common Name = 192.168.124.2 > >* The other HTTP server certificate is for Common Name = > fd33:eb1b:9b36::2 > >* I have a "net-server" virtual machine that runs Apache on the above >IP > addresses (TCP port 443). > > - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. > > - The DHCP servers send the following boot file names: > >- "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso; > [IPv4] >- >"https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso; >[IPv6] > >* For sanity-checking the environment, I run the following two commands > on the *host* (connecting to the "net-server" virtual machine): > >- curl -I >'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' >- curl --globoff -I >'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' > > - The host is configured to trust the brand new test CA certificate >(see near the top). > > - When the certificates are assigned *correctly* to the IP addresses > in the Apache configuration, the above "curl" commands complete just >fine. If I add the "-v" option to "curl", it confirms the right >certificates are used, and it confirms the test CA as issuer too. > > - When the certificates are (intentionally) *cross-assigned* to the IP >addresses in the Apache configuration, then both "curl" commands >break with the following error message: > >> curl: (51) Unable to communicate securely with peer: requested domain >> name does not match the server's certificate. > > - If I add the "-v" option, I also see > >> NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) > > - As a side comment: Apache itself warns about the misconfig, in >"/var/log/httpd/ssl_error_log": > >> ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 >> does NOT include an ID which matches the server name > >* I have a "net-client" virtual machine, running OVMF. > > - The edk2 HTTPS/TLS client booting in this virtual machine is >configured to trust the exact same set of CA certificates that the >host trusts too. > > - In other words, HTTPS boot in the "net-client" VM accepts server >
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
Hi All, (multi-hour composition ahead...) On 10/09/19 09:53, David Woodhouse wrote: > On Tue, 2019-10-08 at 06:19 +, Wu, Jiaxin wrote: >> Hi David, >> >> I just realized you have the comments on Bugzilla 960: >> >>> "...given that testing is failing and code inspection shows it >>> would never have been expected to work." >> >> Do you mean you didn't pass the verification if URLs with IPv6 >> literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me >> where the code inspection indicated it would never have been expected >> to work? We do pass the testing for the URLs with IPv6 if the CN or >> SAN in certificate has the corresponding IPv6 address (at least >> working with openssl 1.1.0). > > I have not tested this, but I started looking when there was a message > on the edk2 list from someone who was reporting that it didn't work > for IPv6 URIs, IIRC. > > You are using SSL_set1_host(), and I believe you're just passing in > the bare hostname part of the URI, be it "1.2.3.4" or > "[2001:8b0:10b::5]". > > That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the > SSL connection. > > In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the > code simply iterates over the members of that list, calling > X509_check_host() for each one. It never calls X509_check_ip(). > > If you look in openssl/crypto/x509/v3_utl.c you can see the > X509_check_host() really does only check hostnames. You'd need to call > X509_check_ip_asc() to check hostnames. And something would need to > have stripped the [] which surround an IPv6 literal. > > I can't see how this can work. Have you tested it since the report on > the list that it wasn't working? > > cf. https://github.com/openssl/openssl/pull/9201 which is being > ignored by the OpenSSL developers OpenSSL really doesn't make > life easy for you here, which is a shame. > > >> For the series patches here, we are intending to support the host >> name validation, I think we can commit the series patches since we >> pass the verification of IPV6 URL, what do you think? > > If it passes the verification of IPv6 literals, then all my analysis > is broken and so was the report on the list that prompted me to start > looking (or I'm misremembering that report). In that case, sure, go > ahead and commit. Here's a summary of my setup. * I've generated a brand new CA certificate, and two HTTP server certificates, signed by the CA. * One HTTP server certificate is for Common Name = 192.168.124.2 * The other HTTP server certificate is for Common Name = fd33:eb1b:9b36::2 * I have a "net-server" virtual machine that runs Apache on the above IP addresses (TCP port 443). - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons. - The DHCP servers send the following boot file names: - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso; [IPv4] - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso; [IPv6] * For sanity-checking the environment, I run the following two commands on the *host* (connecting to the "net-server" virtual machine): - curl -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso' - The host is configured to trust the brand new test CA certificate (see near the top). - When the certificates are assigned *correctly* to the IP addresses in the Apache configuration, the above "curl" commands complete just fine. If I add the "-v" option to "curl", it confirms the right certificates are used, and it confirms the test CA as issuer too. - When the certificates are (intentionally) *cross-assigned* to the IP addresses in the Apache configuration, then both "curl" commands break with the following error message: > curl: (51) Unable to communicate securely with peer: requested domain > name does not match the server's certificate. - If I add the "-v" option, I also see > NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN) - As a side comment: Apache itself warns about the misconfig, in "/var/log/httpd/ssl_error_log": > ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443 > does NOT include an ID which matches the server name * I have a "net-client" virtual machine, running OVMF. - The edk2 HTTPS/TLS client booting in this virtual machine is configured to trust the exact same set of CA certificates that the host trusts too. - In other words, HTTPS boot in the "net-client" VM accepts server certificates signed by the new test CA. * The following is the test plan. 1. The patch set is *not* applied (that is, OVMF is built at current master, commit 976d0353a6ce). 1. Properly assigned certificates: 1. HTTPSv4 boot --> expect success (correct behavior, establishes baseline) 2. HTTPSv6 boot --> expect success (correct
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 10/01/19 01:21, Laszlo Ersek wrote: > On 09/29/19 08:09, Wang, Jian J wrote: >> For this patch series, >> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed >> any more. >> Remove it at push time and no need to send a v2. >> 2. Since it's security patch which had been reviewed separately, I see no >> reason for new r-b >> required. Please raise it asap if any objections. >> 3. Acked-by: Jian J Wang > > > * Can you please confirm that these patches match those that we > discussed here: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 To answer my own question, I've now compared the patches from those BZ comments linked above, with the present series. Here's a list of differences. (1) The subject lines now include the reference "(CVE-2019-14553)". This is great, *but* please be sure to insert a space character before the opening parenthesis! (In every patch.) (2) The commit messages reference both the BZ and the CVE number. Good. (3) In the commit messages, the line Contributed-under: TianoCore Contribution Agreement 1.0 has been upgraded to Contributed-under: TianoCore Contribution Agreement 1.1 I think this is wrong. The lines should have been removed, due to the SPDX adoption. Please update all the commit messages. (4) Copyright notice updates are gone from the patches. That's fine: the reason is that the underlying files have seen their copyright notices updated, meanwhile. Otherwise, the patches (code, commit messages, and feedback tags) are identical. Before you push the patches (or post a v2), please fix issues (1) and (3). Now, regarding the other set of questions: > * In the BZ, David and Bret raised some questions: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > and > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > The latest comment in the bug is c#41. I'm not under the impression that > all concerns raised by David and Bret have been addressed (or > abandoned). I'd like David and Bret to ACK the patches. I'll first have to process the new comments down-thread. Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48664): https://edk2.groups.io/g/devel/message/48664 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Tue, 2019-10-08 at 06:19 +, Wu, Jiaxin wrote: > Hi David, > > I just realized you have the comments on Bugzilla 960: > > > "...given that testing is failing and code inspection shows it > > would never have been expected to work." > > Do you mean you didn't pass the verification if URLs with IPv6 > literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me > where the code inspection indicated it would never have been expected > to work? We do pass the testing for the URLs with IPv6 if the CN or > SAN in certificate has the corresponding IPv6 address (at least > working with openssl 1.1.0). I have not tested this, but I started looking when there was a message on the edk2 list from someone who was reporting that it didn't work for IPv6 URIs, IIRC. You are using SSL_set1_host(), and I believe you're just passing in the bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]". That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the SSL connection. In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the code simply iterates over the members of that list, calling X509_check_host() for each one. It never calls X509_check_ip(). If you look in openssl/crypto/x509/v3_utl.c you can see the X509_check_host() really does only check hostnames. You'd need to call X509_check_ip_asc() to check hostnames. And something would need to have stripped the [] which surround an IPv6 literal. I can't see how this can work. Have you tested it since the report on the list that it wasn't working? cf. https://github.com/openssl/openssl/pull/9201 which is being ignored by the OpenSSL developers — OpenSSL really doesn't make life easy for you here, which is a shame. > For the series patches here, we are intending to support the host > name validation, I think we can commit the series patches since we > pass the verification of IPV6 URL, what do you think? If it passes the verification of IPv6 literals, then all my analysis is broken and so was the report on the list that prompted me to start looking (or I'm misremembering that report). In that case, sure, go ahead and commit. > Thanks, > Jiaxin > > > -Original Message- > > From: David Woodhouse > > Sent: Tuesday, October 1, 2019 5:02 PM > > To: Laszlo Ersek ; devel@edk2.groups.io; Wang, > > Jian J > > ; Wu, Jiaxin ; Bret > > Barkelew > > > > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > > validation feature(CVE-2019-14553) > > > > On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > > > On 09/29/19 08:09, Wang, Jian J wrote: > > > > For this patch series, > > > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" > > > > is not > > > > needed any more. > > > > Remove it at push time and no need to send a v2. > > > > 2. Since it's security patch which had been reviewed > > > > separately, I see no > > > > reason for new r-b > > > > required. Please raise it asap if any objections. > > > > 3. Acked-by: Jian J Wang > > > > > > > > > * Can you please confirm that these patches match those that we > > > discussed here: > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > > > > > > > * In the BZ, David and Bret raised some questions: > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > > > and > > > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > > > The latest comment in the bug is c#41. I'm not under the > > > impression that > > > all concerns raised by David and Bret have been addressed (or > > > abandoned). I'd like David and Bret to ACK the patches. > > > > I do not believe my comment #35 has been addressed, nor the > > requested > > testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48626): https://edk2.groups.io/g/devel/message/48626 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
Hi David, I just realized you have the comments on Bugzilla 960: >"...given that testing is failing and code inspection shows it would never >have been expected to work." Do you mean you didn't pass the verification if URLs with IPv6 literals (https://[2001:8b0:10b:1236::1]/)? Can you also show me where the code inspection indicated it would never have been expected to work? We do pass the testing for the URLs with IPv6 if the CN or SAN in certificate has the corresponding IPv6 address (at least working with openssl 1.1.0). For the series patches here, we are intending to support the host name validation, I think we can commit the series patches since we pass the verification of IPV6 URL, what do you think? Thanks, Jiaxin > -Original Message- > From: David Woodhouse > Sent: Tuesday, October 1, 2019 5:02 PM > To: Laszlo Ersek ; devel@edk2.groups.io; Wang, Jian J > ; Wu, Jiaxin ; Bret Barkelew > > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName > validation feature(CVE-2019-14553) > > On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > > On 09/29/19 08:09, Wang, Jian J wrote: > > > For this patch series, > > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not > needed any more. > > > Remove it at push time and no need to send a v2. > > > 2. Since it's security patch which had been reviewed separately, I see no > reason for new r-b > > > required. Please raise it asap if any objections. > > > 3. Acked-by: Jian J Wang > > > > > > * Can you please confirm that these patches match those that we > > discussed here: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > > > > * In the BZ, David and Bret raised some questions: > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > > > and > > > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > > > The latest comment in the bug is c#41. I'm not under the impression that > > all concerns raised by David and Bret have been addressed (or > > abandoned). I'd like David and Bret to ACK the patches. > > I do not believe my comment #35 has been addressed, nor the requested > testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48547): https://edk2.groups.io/g/devel/message/48547 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote: > On 09/29/19 08:09, Wang, Jian J wrote: > > For this patch series, > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed > > any more. > > Remove it at push time and no need to send a v2. > > 2. Since it's security patch which had been reviewed separately, I see no > > reason for new r-b > > required. Please raise it asap if any objections. > > 3. Acked-by: Jian J Wang > > > * Can you please confirm that these patches match those that we > discussed here: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 > > > * In the BZ, David and Bret raised some questions: > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 > > and > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 > > The latest comment in the bug is c#41. I'm not under the impression that > all concerns raised by David and Bret have been addressed (or > abandoned). I'd like David and Bret to ACK the patches. I do not believe my comment #35 has been addressed, nor the requested testing performed. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48340): https://edk2.groups.io/g/devel/message/48340 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=- smime.p7s Description: S/MIME cryptographic signature
Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)
On 09/29/19 08:09, Wang, Jian J wrote: > For this patch series, > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed > any more. > Remove it at push time and no need to send a v2. > 2. Since it's security patch which had been reviewed separately, I see no > reason for new r-b > required. Please raise it asap if any objections. > 3. Acked-by: Jian J Wang * Can you please confirm that these patches match those that we discussed here: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19 * In the BZ, David and Bret raised some questions: https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35 https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36 and https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40 The latest comment in the bug is c#41. I'm not under the impression that all concerns raised by David and Bret have been addressed (or abandoned). I'd like David and Bret to ACK the patches. Thanks, Laszlo >> -Original Message- >> From: devel@edk2.groups.io On Behalf Of Wu, Jiaxin >> Sent: Friday, September 27, 2019 11:45 AM >> To: devel@edk2.groups.io >> Cc: Wu, Jiaxin >> Subject: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation >> feature(CVE-2019-14553) >> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960 >> CVE: CVE-2019-14553 >> The series patches are to support HTTPS hostname validation feature. >> It fixes the issue exposed @ >> https://bugzilla.tianocore.org/show_bug.cgi?id=960. >> In the patches, we add the new data type named "EfiTlsVerifyHost" and >> the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to >> enable the host name check so as to avoid the potential >> Man-In-The-Middle attack. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Wu Jiaxin >> Reviewed-by: Ye Ting >> Reviewed-by: Long Qin >> Reviewed-by: Fu Siyuan >> Acked-by: Laszlo Ersek >> >> Jiaxin Wu (4): >> MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE- >> 2019-14553) >> CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553) >> NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE- >> 2019-14553) >> NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553) >> >> CryptoPkg/Include/Library/TlsLib.h | 20 >> CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++- >> MdePkg/Include/Protocol/Tls.h| 68 +++- >> NetworkPkg/HttpDxe/HttpProto.h | 1 + >> NetworkPkg/HttpDxe/HttpsSupport.c| 21 +++-- >> NetworkPkg/TlsDxe/TlsProtocol.c | 44 -- >> 6 files changed, 173 insertions(+), 19 deletions(-) >> >> -- >> 2.17.1.windows.2 >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48328): https://edk2.groups.io/g/devel/message/48328 Mute This Topic: https://groups.io/mt/34307578/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
