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

[Maxim Dounin]

Hello!

On Thu, Aug 18, 2016 at 07:46:29PM -0700, Piotr Sikora wrote:

> Hey Maxim,
> 
> > 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.
> 
> That's not what the documentation says, though...

The above quote is from the documentation.

> At this point, NGINX is just relying on a known bug, nothing else.

At this point nginx just uses the interface provided by the 
OpenSSL library, exactly as documented.  The fact that the 
interface is flawed (and documented to be flawed to make sure 
people will use it properly) has nothing to do with nginx use of 
it.

> > Ok, this looks like the real reason for the patch.  This looks
> > like an API change in BoringSSL, and should be threated
> > accordingly.
> 
> Nope, that's not the real reason... BoringSSL actually added a
> workaround for this [1], just to play nicely with NGINX and
> netty-tcnative, because virtually everything else is using OpenSSL API
> correctly, i.e. checking if peer sent the certificate before checking
> if that certificate passed verification.
> 
> The real reason for this patch is that existing code is misusing OpenSSL API.
> 
> [1] https://boringssl.googlesource.com/boringssl/+/37646838e9bb62a0d9d506b117193611c4c46012

So the change was inspired by an attempt to change the API in 
BoringSSL.  As for "misusing", see above.

[...]

> > (2) looks hardly portable (what if another
> > library will have different rc values? or will have more than one
> > error string to print?),
> 
> Well, it's portable assuming that the library also converts the "rc"
> value back to the error string (like in the patchset I sent
> yesterday).

Your patches assume that there are no conflicts between nginx 
error codes (NGX_OK, NGX_DECLINED) and SSL_get_verify_result() 
error codes.  While this is currently true, this is not something 
I would rely on, even assuming OpenSSL only.

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