[PATCH] SSL: consistent certificate verification depth

[Maxim Dounin]

Hello!

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

> > On 20 Jul 2022, at 22:04, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > 
> > On Wed, Jul 20, 2022 at 05:58:26PM +0400, Sergey Kandaurov wrote:
> > 
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet at 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/