Hello!

On Thu, Jul 21, 2022 at 06:18:24PM +0400, Sergey Kandaurov wrote:

> > On 20 Jul 2022, at 22:04, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > On Wed, Jul 20, 2022 at 05:58:26PM +0400, Sergey Kandaurov wrote:
> >
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet@nginx.com>
> >> # Date 1658325446 -14400
> >> # Wed Jul 20 17:57:26 2022 +0400
> >> # Node ID 5a9cc2a846c9ea4c0af03109ab186af1ac28e222
> >> # Parent 069a4813e8d6d7ec662d282a10f5f7062ebd817f
> >> SSL: consistent certificate verification depth.
> >>
> >> Originally, certificate verification depth was used to limit the number
> >> of signatures to validate, that is, to limit chains with intermediate
> >> certificates one less. The semantics was changed in OpenSSL 1.1.0, and
> >> instead it limits now the number of intermediate certificates allowed.
> >> This makes it not possible to limit certificate checking to self-signed
> >> certificates with verify depth 0 in OpenSSL 1.1.0+, and is inconsistent
> >> compared with former behaviour in BoringSSL and older OpenSSL versions.
> >>
> >> This change restores former verification logic when using OpenSSL 1.1.0+.
> >> The verify callback is adjusted to emit the "certificate chain too long"
> >> error when the certificate chain exceeds the verification depth. It has
> >> no effect to other SSL libraries, where this is limited by other means.
> >> Also, this fixes verification checks when using LibreSSL 3.4.0+, where
> >> a chain depth is not limited except by X509_VERIFY_MAX_CHAIN_CERTS (32).
> >
> > This (highly questionable) OpenSSL behaviour seems to be status
> > quo for a while, also recorded in tests (see ssl_verify_depth.t).
> > Any specific reasons for the nginx-side workaround?
>
> As explained in the commit message, main motivation is to eliminate
> annoying difference in behaviour among various SSL libraries
> (aside from working around the arguably broken LibreSSL verifier).
> Nothing specific behind that.
> Disambiguating ssl_verify_depth.t is a good demo of net result.
> The downside is that this can potentially break previously working
> configurations when using with modern OpenSSL versions.
> So the patch is to seek feedback on whether this makes sense.

Apart from potentially breaking some existing configurations
(especially with LibreSSL), I have several basic concerns here:

1. As of now, it's OpenSSL responsibility to apply verify depth
limit, and "openssl verify -verify_depth <num>" can be used to
test the particular verification. Switching to our implementation
of the depth limit means that it will be our responsibility to
apply and document the limit.

2. The verify callback behaviour is complex enough, and I expect
portability issues.

3. Further, the main reason why the depth limit exists is to limit
maximum resource consumption of the whole certificate verification
process. Raising an error within the verify callback without
returning failure won't stop OpenSSL from checking additional
certificates, and hence is not going to limit the resource usage.
That is, such a limit would be misleading: it will prevent
certificates from being accepted, but won't stop DoS attacks.

I don't think that there is a way to implement the limit via
verify callback in a way which will prevent excessive signature
checks yet preserve nginx existing behaviour with not closing the
connection in case of verification failures.

Summing the above, I tend to think that it's not worth the effort.

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org