[PATCH] SSL: fix order of checks during SSL certificate verification

[Maxim Dounin]

Hello!

On Tue, Aug 09, 2016 at 12:51:38PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > This behaviour is explicitly documented for years.  The BUGS
> > section outlines that the API is not intuitive and requires use of
> > SSL_get_peer_certificate() in addition ot SSL_get_verify_result().
> 
> Exactly.
> 
> > And this is what nginx does.  I don't see compelling reasons to
> > change the order of the calls here.
> 
> No, it doesn't, which is the reason for this patch in the first
> place... NGINX uses the result of SSL_get_verify_result() without
> calling SSL_get_peer_certificate(), which is what the BUGS section
> requires.

Calling SSL_get_peer_certificate() is only needed if 
SSL_get_verify_result() returns X509_V_OK, to distinguish two of 
its different meanings:

       X509_V_OK
           The verification succeeded or no peer certificate was presented.

And that's what nginx does.

> Also, this breaks implementations (i.e. BoringSSL) that are a bit more
> strict and don't use X509_V_OK as the initial value.

Ok, this looks like the real reason for the patch.  This looks 
like an API change in BoringSSL, and should be threated 
accordingly.

Given the number of various API changes BoringSSL introduces here 
and there - we probably don't want to follow, at least till some 
version is actually released.

> > This looks like a separate patch, or two patches.
> 
> Fair enough.
> 
> > Though I'm
> > somewhat sceptical about the use of "upstream" and "client" in
> > error messages introduced, this looks like a wrong approach for a
> > generic SSL code.
> 
> What you would prefer, then?
> 
> 1. Returning error string back the the caller?
> 2. Returning "rc" back to the caller and adding another function to
> abstract X509_verify_cert_error_string()?
> 3. Adding the caller string ("client" or "upstream") as the parameter
> and writing error from within the function?

I can't say I like either of the variants.  (1) will require 
memory allocations, (2) looks hardly portable (what if another 
library will have different rc values? or will have more than one 
error string to print?), and (3) looks strange.

May be something like "peer" in error messages instead would be a 
better solution, assuming context is already known from the 
c->log->action == "SSL handshaking to upstream".  Alternatively, 
we can consider abstracting printing of verification results 
errors with something similar to ngx_ssl_error().

> > As well as magic values in the "verify" argument,
> 
> Good call.
> 
> > and the change of the ngx_ssl_check_host() semantics.
> 
> You mean the introduction of error logs? If so, then it seems that
> returning "rc" and adding another function to retrieve the error
> string would be the best way to keep the behavior as close to the
> existing one as possible.

There are two changes in the ngx_ssl_check_host() function in the 
patch: 

- an API change which moves error printing to the function;

- a semantics change which adds certificate verification check to 
  the function.

Both has problems.  API changes tend to break 3rd party modules, 
and so it's better to avoid them unless really needed.  The 
semantics change in question may not be adequate for various 
non-trivial cases when one want to check verification results but 
not host or vice versa.

-- 
Maxim Dounin
http://nginx.org/