[PATCH] Proxy remote server SSL certificate verification

[Maxim Dounin]

Hello!

On Wed, Aug 21, 2013 at 02:45:55PM +0300, Aviram Cohen wrote:

> Hello!
> 
> Thank you for the useful feedback!
> I think it's better not to use ssl_trusted_certificate (for a more
> extensible solution).
> The following is the fixed patch. I've also attached it in case Gmail
> corrupts it.

[...]

>  ngx_int_t
> +ngx_ssl_set_verify_options(ngx_ssl_t *ssl, ngx_str_t *cert,
> +    ngx_int_t depth)
> +{
> +    if (cert->len == 0) {
> +        return NGX_OK;
> +    }
> +
> +    SSL_CTX_set_verify(ssl->ctx, SSL_VERIFY_PEER,
> ngx_http_ssl_verify_callback);
> +
> +    SSL_CTX_set_verify_depth(ssl->ctx, depth);
> +
> +    if (SSL_CTX_load_verify_locations(ssl->ctx, (char *) cert->data, NULL)
> +        == 0)
> +    {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                      "SSL_CTX_load_verify_locations(\"%s\") failed",
> +                      cert->data);
> +        return NGX_ERROR;
> +    }
> +
> +    return NGX_OK;
> +}

Even if you don't want to reuse ssl_trusted_certificate value, 
reusing ngx_ssl_trusted_certificate() function might be a good 
idea.  In particular, it would have saved you from a bug with 
relative certificate names handling.

> +
> +
> +ngx_int_t
>  ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
>      ngx_int_t depth)
>  {
> @@ -710,6 +735,17 @@ ngx_ssl_set_session(ngx_connection_t *c,
>      return NGX_OK;
>  }
> 
> +
> +ngx_int_t
> +ngx_ssl_verify_result(ngx_connection_t *c)
> +{
> +    if (SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
> +        ngx_ssl_error(NGX_LOG_EMERG, c->log, 0,
> "SSL_get_verify_result failed");
> +        return NGX_ERROR;
> +    }
> +    return NGX_OK;
> +}
> +

Note: the SSL_get_verify_result() is currently called directly by 
ngx_http_request.c code, and introducing a wrapper function for 
proxy code might not be a good idea.

[...]

> +    { ngx_string("proxy_ssl_certificate"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_str_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_certificate),
> +      NULL },
>  #endif

Still bad name.  Such name will easily cause confusion with a 
client certificate proxy may in theory use as well.  Something 
like "proxy_ssl_trusted_certificate" is probably good enough.

[...]

> @@ -3748,6 +3786,13 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, n
>          != NGX_OK)
>      {
>          return NGX_ERROR;
> +    }
> +
> +    if (ngx_ssl_set_verify_options(plcf->upstream.ssl,
> +          &plcf->upstream.ssl_certificate, plcf->upstream.ssl_verify_depth)
> +        != NGX_OK)
> +    {
> +        return NGX_ERROR;
>      }

This is called before options used are correctly set.

(There is also a style problem here, but it doesn't really matter 
as you'll have to rewrite the code anyway.)

[...]

> @@ -1334,6 +1341,11 @@ ngx_http_upstream_ssl_handshake(ngx_conn
> 
>          ngx_http_upstream_send_request(r, u);
> 
> +        c = r->connection;
> +
> +fail:
> +        ngx_http_run_posted_requests(c);
> +
>          return;
>      }
> 

You probably missed my previous comment.  You have a use after 
free problem here.  Try triggering an error in 
ngx_http_upstream_send_request() with NGX_DEBUG_MALLOC defined, it 
should segfault.
 
[...]

-- 
Maxim Dounin
http://nginx.org/en/donation.html