Re: [Openvpn-devel] [RFC PATCH v1 05/15] OpenSSL: don't use direct access to the internal of X509

[Emmanuel D. <lo...@fr...>]

Hi,

I'm not sure why but it seems this mail (that I send yesterday) never found
its way to the ML. So I re-send it.

Sorry for the inconvenience.

BR,

-- Emmanuel Deloget

On Mon, Mar 27, 2017 at 5:49 PM, Emmanuel Deloget <lo...@fr...> wrote:

> Hi everyone,
>
> I got some time to try to fix all that stuff.
>
> First,
>
> On Sat, Mar 4, 2017 at 11:38 PM, David Sommerseth <op...@sf...sts.
> topphemmelig.net> wrote:
>
>> On 04/03/17 16:13, Steffan Karger wrote:
>> > As a last resort, we could consider keeping the old code inside #if
>> > OSSL_VER < 1.1.0 in release/2.4, but that might just create more
>> > confusion...
>>
>
>
> ​I think I found a solution -- see later in the email.​
>
> I had a closer look to the corresponding code to see what are the
> differences between the old check and the new one -- and indeed,
> the discrepancies might only happen if the tested certificate is
> kind-of-weird. For exemple, a client certificate:
>
> * has the client bit set
> * has no key, or has a key but the key usage is neither for
>   signature nor for key agreement
> * is not used to auth a client
>
> ​If a user creates a Frankenstein certificate that look like this,
> it might be a good idea to warn him that such certificate is very
> likely to be refused in the near future (one can have a similar
> reasonning about server certificates).
>
>
>
>>
>> Just a very quick thought here ... I do dislike different behaviours
>> depending on which OpenSSL version being used.  But given that this
>> feature is already deprecated and even removed in OpenSSL-1.1, I think
>> that gives us some options.
>>
>> I agree with Gert that breaking --ns-cert-type isn't good at all.
>> However, consider when people upgrade from OpenSSL v1.0 to v1.1 - that
>> is most commonly when there is major distro update.  It is not something
>> which happens "mid-term", as OpenSSL is quite commonly used by lots of
>> base system packages these days.
>>
>> Regardless of OpenSSL version, I agree to loudly deprecate
>> --ns-cert-type, through documentation, --help and log files.
>>
>> But I think we need to carry the existing behaviour for --ns-cert-type
>> when built against OpenSSL v1.0.  And we can solve through some
>> compatibility wrapper when built against OpenSSL v1.1 - with even louder
>> warnings in the logs that this may break apart.
>>
>
>
> ​Fortunately, I think I found a solution that would help
> when using OpenSSL 1.1. Idealy, this should be a call
> to X509_check_purpose(.., X509_PURPOSE_SSL_*_WEAK, ...)​
> but I don't think the OpenSSL developers will accept such
> a change :)
>
> It might be possible to use the X509_get_ext_d2i() call
> like this:
>
> void *ns = X509_get_ext_d2i(x, NID_netscape_cert_type,
>                             NULL, NULL);
>
> ​In this case, the ​obtained ns object contains the data
> we're interested in:
>
> * if it exists, then EXFLAG_NSCERT is set
> * if not null, ns is of type ASN1_BIT_STRING, and this
>   type is not opaque (yep, sir).
>
> ​So the code would look like:
>
> ​ASN1_BIT_STRING *ns;
> result_t result = FAILURE;
> ns = X509_get_ext_d2i(x, NID_netscape_cert_type, NULL, NULL);
> if (ns)
> {
>     if (ns->length > 0)
>     {
>         int flags = ns->data[0];
>         if (flags & NS_SSL_CLIENT)
>         {
>             result = SUCCESS;
>         }
>     }
>     ASN1_BIT_STRING_free(ns);
> }
>
>
> ​For the record, it's the code ​that is used within function
> x509v3_cache_extensions() which builds the X509 flags we
> are using so I'm failry confident it's the right thing to
> do (but it's a bit convoluted and I don't like it much).
>
> ​Good news: the same code should work with nearly all the
> previous versions of OpenSSL.
>
>
>
>> ​<snip>
>>
>>
>> --
>> kind regards,
>>
>> David Sommerseth
>> OpenVPN Technologies, Inc
>>
>
>
> I'll post my new patches as soon as I get over every issues
> that have been talked on the ML (is that even a valid
> sentence?)
>
> ​Best regards,
>
> -- Emmanuel Deloget​
>