[PATCH] Proxy remote server SSL certificate verification
Aviram Cohen
aviram at adallom.com
Wed Oct 9 16:32:52 UTC 2013
Hello!,
I've made the necessary fixes. A few comments about those:
- Name validation
- Unlike Apache, in this patch, the configuration must contain the name
to verify. In most cases, this should be set to the $host variable (and
this is the default value). I've encountered certificates that validate
names differently (some of Microsoft's certificates), and this is useful
for such cases.
- Many certificates use wildcards in the names on which they sign (i.e.
"*.google.com"). Though wildcards can appear anywhere according to the
standard, I've added support only for a wildcard in the beginning of the
name. This is the normal case, and it is easier to implement. This is
is how Apache implements the wildcard name validation as well. Note also
that the function X509_check_host will be introduced in future versions
of OpenSSL that will provide the entire feature.
- CRL verification - I've added CRL validation, but note that OpenSSL doesn't
download CRL files from the servers, so the CRL file that is used should
contain the revocation lists of all the proxied hosts. This also
means that it
is out of Nginx's scope to update this file. Apache does the same thing.
- The patch was made for v1.4.1.
The patch itself is in the end of this mail, and also in:
https://gist.github.com/aviramc/6903821
Best regards,
Aviram
# HG changeset patch
# User Aviram Cohen <aviram at adallom.com>
# Date 1381334949 -7200
# Branch stable-1.4
# Node ID 9a6e20bf72f8cf4d17653e4fdfcbac48c4de03aa
# Parent 0702de638a4c51123d7b97801d393e8e25eb48de
Added remote end SSL certificate verification in the proxy module.
This patch adds the following directives to the proxy module:
- proxy_ssl_verify - whether or not to verify the remote end's
certificate. Default is off.
- proxy_ssl_verify_name - the remote end's name to verify. Default is
$host, can be set to "" in order to avoid name verification.
- proxy_ssl_verify_depth - how deep the ssl verification should be
done. Default is 1.
- proxy_ssl_trusted_certificate - the path of the certificate file
that is used for verification. This must be provided when
proxy_ssl_verify is on.
- proxy_ssl_crl - the path a file that contains the CRLs of the hosts
to which we proxy. Default is empty, and CRL verification is not done.
diff -r 0702de638a4c -r 9a6e20bf72f8 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c Mon May 06 14:20:27 2013 +0400
+++ b/src/event/ngx_event_openssl.c Wed Oct 09 18:09:09 2013 +0200
@@ -42,6 +42,11 @@
static char *ngx_openssl_engine(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
static void ngx_openssl_exit(ngx_cycle_t *cycle);
+static int ngx_openssl_host_wildcard_match(ASN1_STRING *match_pattern,
+ ngx_str_t *hostname);
+static int ngx_openssl_host_exact_match(ASN1_STRING *match_pattern,
+ ngx_str_t *hostname);
+
static ngx_command_t ngx_openssl_commands[] = {
@@ -2562,3 +2567,163 @@
EVP_cleanup();
ENGINE_cleanup();
}
+
+
+static int
+ngx_openssl_host_wildcard_match(ASN1_STRING *match_pattern,
+ ngx_str_t *hostname)
+{
+ int host_top_domain_length;
+ u_char *host_top_domain;
+ u_char *wildcard_pattern;
+
+ /* sanity check */
+ if (!match_pattern
+ || match_pattern->length <= 0
+ || !hostname
+ || !hostname->len)
+ {
+ return 0;
+ }
+
+ /* trivial case */
+ if (ngx_strncasecmp((u_char *) match_pattern->data,
+ hostname->data,
+ hostname->len)
+ == 0)
+ {
+ return 1;
+ }
+
+ /* simple wildcard matching - only in the beginning of the string. */
+ if (match_pattern->length > 2
+ && match_pattern->data[0] == '*'
+ && match_pattern->data[1] == '.')
+ {
+
+ wildcard_pattern = (u_char *) (match_pattern->data + 1);
+
+ host_top_domain = ngx_strlchr(hostname->data,
+ hostname->data + hostname->len,
+ '.');
+
+ /*
+ * If the pattern begings with "*." and the hostname consists of
+ * a top level domain, compare the pattern to the top level domain.
+ */
+ if (host_top_domain != NULL) {
+ host_top_domain_length =
+ hostname->len - (int) (host_top_domain - hostname->data);
+
+ if (host_top_domain_length == match_pattern->length - 1
+ && ngx_strncasecmp(wildcard_pattern,
+ host_top_domain,
+ match_pattern->length - 1)
+ == 0)
+ {
+ return 1;
+ }
+ }
+ }
+
+ return 0;
+}
+
+
+static int
+ngx_openssl_host_exact_match(ASN1_STRING *match_pattern,
+ ngx_str_t *hostname)
+{
+ /* sanity check */
+ if (!match_pattern
+ || match_pattern->length <= 0
+ || !hostname
+ || !hostname->len
+ || match_pattern->length != (int) hostname->len)
+ {
+ return 0;
+ }
+
+ if (ngx_strncmp((u_char *) match_pattern->data,
+ hostname->data,
+ hostname->len)
+ == 0)
+ {
+ return 1;
+ }
+
+ return 0;
+}
+
+
+int
+ngx_openssl_verify_name(X509 *cert, ngx_str_t *expected_name)
+{
+ GENERAL_NAMES *gens = NULL;
+ GENERAL_NAME *gen;
+ X509_NAME *name = NULL;
+ ASN1_STRING *cstr = NULL;
+ X509_NAME_ENTRY *ne;
+ int i;
+ int rc = 0;
+
+ /* based on OpenSSL's do_x509_check */
+
+ gens = X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
+
+ if (gens) {
+
+ rc = 0;
+
+ for (i = 0; i < sk_GENERAL_NAME_num(gens); i++) {
+
+ gen = sk_GENERAL_NAME_value(gens, i);
+
+ /* we only check for either name or IP */
+ switch (gen->type) {
+
+ case GEN_DNS:
+ cstr = gen->d.dNSName;
+ rc = ngx_openssl_host_wildcard_match(cstr,
+ expected_name);
+ break;
+
+ case GEN_IPADD:
+ cstr = gen->d.iPAddress;
+ rc = ngx_openssl_host_exact_match(cstr,
+ expected_name);
+ break;
+
+ default:
+ cstr = NULL;
+ rc = 0;
+ }
+
+ if (rc) {
+ break;
+ }
+
+ }
+
+ GENERAL_NAMES_free(gens);
+
+ if (rc) {
+ return 1;
+ }
+ }
+
+ name = X509_get_subject_name(cert);
+ i = X509_NAME_get_index_by_NID(name, NID_commonName, -1);
+ while (i >= 0) {
+ ne = X509_NAME_get_entry(name, i);
+ cstr = X509_NAME_ENTRY_get_data(ne);
+
+ if (ngx_openssl_host_exact_match(cstr, expected_name)) {
+ return 1;
+ }
+
+ i = X509_NAME_get_index_by_NID(name, NID_commonName, i);
+ }
+
+ return 0;
+}
diff -r 0702de638a4c -r 9a6e20bf72f8 src/event/ngx_event_openssl.h
--- a/src/event/ngx_event_openssl.h Mon May 06 14:20:27 2013 +0400
+++ b/src/event/ngx_event_openssl.h Wed Oct 09 18:09:09 2013 +0200
@@ -167,6 +167,8 @@
char *fmt, ...);
void ngx_ssl_cleanup_ctx(void *data);
+int
+ngx_openssl_verify_name(X509 *cert, ngx_str_t *expected_name);
extern int ngx_ssl_connection_index;
extern int ngx_ssl_server_conf_index;
diff -r 0702de638a4c -r 9a6e20bf72f8 src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c Mon May 06 14:20:27 2013 +0400
+++ b/src/http/modules/ngx_http_proxy_module.c Wed Oct 09 18:09:09 2013 +0200
@@ -74,6 +74,15 @@
ngx_uint_t http_version;
+#if (NGX_HTTP_SSL)
+ ngx_uint_t ssl_verify_depth;
+ ngx_str_t ssl_trusted_certificate;
+ ngx_str_t ssl_crl;
+ ngx_str_t ssl_verify_name_source;
+ ngx_array_t *ssl_verify_name_lengths;
+ ngx_array_t *ssl_verify_name_values;
+#endif
+
ngx_uint_t headers_hash_max_size;
ngx_uint_t headers_hash_bucket_size;
} ngx_http_proxy_loc_conf_t;
@@ -510,6 +519,41 @@
NGX_HTTP_LOC_CONF_OFFSET,
offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_session_reuse),
NULL },
+
+ { ngx_string("proxy_ssl_verify"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+ ngx_conf_set_flag_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_verify),
+ NULL },
+
+ { ngx_string("proxy_ssl_verify_name"),
+ 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, ssl_verify_name_source),
+ NULL },
+
+ { ngx_string("proxy_ssl_verify_depth"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
+ ngx_conf_set_num_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_proxy_loc_conf_t, ssl_verify_depth),
+ NULL },
+
+ { ngx_string("proxy_ssl_trusted_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, ssl_trusted_certificate),
+ NULL },
+
+ { ngx_string("proxy_ssl_crl"),
+ 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, ssl_crl),
+ NULL },
#endif
@@ -633,6 +677,7 @@
ngx_http_upstream_t *u;
ngx_http_proxy_ctx_t *ctx;
ngx_http_proxy_loc_conf_t *plcf;
+ ngx_str_t host;
if (ngx_http_upstream_create(r) != NGX_OK) {
return NGX_HTTP_INTERNAL_SERVER_ERROR;
@@ -662,6 +707,24 @@
}
}
+#if (NGX_HTTP_SSL)
+
+ if (plcf->ssl_verify_name_lengths == NULL) {
+ u->ssl_verify_name.data = plcf->ssl_verify_name_source.data;
+ u->ssl_verify_name.len = plcf->ssl_verify_name_source.len;
+ } else {
+ if (ngx_http_script_run(r, &host, plcf->ssl_verify_name_lengths->elts,
+ 0, plcf->ssl_verify_name_values->elts)
+ == NULL)
+ {
+ return NGX_HTTP_INTERNAL_SERVER_ERROR;
+ }
+ u->ssl_verify_name.data = host.data;
+ u->ssl_verify_name.len = host.len;
+ }
+
+#endif
+
u->output.tag = (ngx_buf_tag_t) &ngx_http_proxy_module;
u->conf = &plcf->upstream;
@@ -2382,6 +2445,11 @@
* conf->body_set = NULL;
* conf->body_source = { 0, NULL };
* conf->redirects = NULL;
+ * conf->ssl_trusted_certificate = { 0, NULL };
+ * conf->ssl_crl = { 0, NULL };
+ * conf->ssl_verify_name_source = { 0, NULL };
+ * conf->ssl_verify_name_lengths = NULL;
+ * conf->ssl_verify_name_values = NULL;
*/
conf->upstream.store = NGX_CONF_UNSET;
@@ -2419,8 +2487,11 @@
conf->upstream.pass_headers = NGX_CONF_UNSET_PTR;
conf->upstream.intercept_errors = NGX_CONF_UNSET;
+
#if (NGX_HTTP_SSL)
conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
+ conf->upstream.ssl_verify = NGX_CONF_UNSET;
+ conf->ssl_verify_depth = NGX_CONF_UNSET_UINT;
#endif
/* "proxy_cyclic_temp_file" is disabled */
@@ -2455,6 +2526,7 @@
ngx_http_core_loc_conf_t *clcf;
ngx_http_proxy_rewrite_t *pr;
ngx_http_script_compile_t sc;
+ ngx_int_t n;
if (conf->upstream.store != 0) {
ngx_conf_merge_value(conf->upstream.store,
@@ -2695,8 +2767,62 @@
prev->upstream.intercept_errors, 0);
#if (NGX_HTTP_SSL)
+
ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
prev->upstream.ssl_session_reuse, 1);
+ ngx_conf_merge_value(conf->upstream.ssl_verify,
+ prev->upstream.ssl_verify, 0);
+ ngx_conf_merge_str_value(conf->ssl_verify_name_source,
+ prev->ssl_verify_name_source, "$host");
+ ngx_conf_merge_uint_value(conf->ssl_verify_depth,
+ prev->ssl_verify_depth, 1);
+ ngx_conf_merge_str_value(conf->ssl_trusted_certificate,
+ prev->ssl_trusted_certificate, "");
+ ngx_conf_merge_str_value(conf->ssl_crl,
+ prev->ssl_crl, "");
+
+ if (conf->upstream.ssl && conf->upstream.ssl_verify) {
+ if (conf->ssl_trusted_certificate.len == 0) {
+ ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
+ "no \"proxy_ssl_trusted_certificate\" is "
+ " defined for the \"proxy_ssl_verify\" "
+ "directive");
+
+ return NGX_CONF_ERROR;
+ }
+
+ if (ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
+ &conf->ssl_trusted_certificate,
+ conf->ssl_verify_depth)
+ != NGX_OK)
+ {
+ return NGX_CONF_ERROR;
+ }
+
+ if (ngx_ssl_crl(cf, conf->upstream.ssl, &conf->ssl_crl) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
+ n = ngx_http_script_variables_count(&conf->ssl_verify_name_source);
+
+ if (n) {
+ ngx_memzero(&sc, sizeof(ngx_http_script_compile_t));
+
+ sc.cf = cf;
+ sc.source = &conf->ssl_verify_name_source;
+ sc.variables = n;
+ sc.lengths = &conf->ssl_verify_name_lengths;
+ sc.values = &conf->ssl_verify_name_values;
+ sc.complete_lengths = 1;
+ sc.complete_values = 1;
+
+ if (ngx_http_script_compile(&sc) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
+ }
+ }
+
#endif
ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
diff -r 0702de638a4c -r 9a6e20bf72f8 src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c Mon May 06 14:20:27 2013 +0400
+++ b/src/http/ngx_http_upstream.c Wed Oct 09 18:09:09 2013 +0200
@@ -1319,12 +1319,43 @@
{
ngx_http_request_t *r;
ngx_http_upstream_t *u;
-
+ X509 *cert;
+ long rc;
+
r = c->data;
u = r->upstream;
if (c->ssl->handshaked) {
-
+ if (u->conf->ssl_verify) {
+ rc = SSL_get_verify_result(c->ssl->connection);
+ if (rc != X509_V_OK) {
+ ngx_log_error(NGX_LOG_ERR, c->log, 0,
+ "upstream SSL certificate verify error: (%l:%s)",
+ rc, X509_verify_cert_error_string(rc));
+ goto fail;
+ }
+
+ cert = SSL_get_peer_certificate(c->ssl->connection);
+
+ if (cert == NULL) {
+ ngx_log_error(NGX_LOG_ERR, c->log, 0,
+ "upstream sent no SSL certificate");
+ goto fail;
+ }
+
+ if (u->ssl_verify_name.len
+ && ngx_openssl_verify_name(cert, &u->ssl_verify_name)
+ == 0)
+ {
+ X509_free(cert);
+ ngx_log_error(NGX_LOG_ERR, c->log, 0,
+ "upstream SSL certificate name
validation error");
+ goto fail;
+ }
+
+ X509_free(cert);
+ }
+
if (u->conf->ssl_session_reuse) {
u->peer.save_session(&u->peer, u->peer.data);
}
@@ -1332,13 +1363,22 @@
c->write->handler = ngx_http_upstream_handler;
c->read->handler = ngx_http_upstream_handler;
+ c = r->connection;
+
ngx_http_upstream_send_request(r, u);
+ ngx_http_run_posted_requests(c);
+
return;
}
+fail:
+
+ c = r->connection;
+
ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
+ ngx_http_run_posted_requests(c);
}
#endif
diff -r 0702de638a4c -r 9a6e20bf72f8 src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h Mon May 06 14:20:27 2013 +0400
+++ b/src/http/ngx_http_upstream.h Wed Oct 09 18:09:09 2013 +0200
@@ -191,6 +191,7 @@
#if (NGX_HTTP_SSL)
ngx_ssl_t *ssl;
ngx_flag_t ssl_session_reuse;
+ ngx_flag_t ssl_verify;
#endif
ngx_str_t module;
@@ -297,6 +298,10 @@
ngx_int_t (*input_filter)(void *data, ssize_t bytes);
void *input_filter_ctx;
+#if (NGX_HTTP_SSL)
+ ngx_str_t ssl_verify_name;
+#endif
+
#if (NGX_HTTP_CACHE)
ngx_int_t (*create_key)(ngx_http_request_t *r);
#endif
On Tue, Sep 3, 2013 at 4:21 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> Hello!
>
> On Tue, Sep 03, 2013 at 03:32:58PM +0300, Aviram Cohen wrote:
>
>> Thanks for the comments. The new version with all the fixes is
>> attached (and also pasted in this mail).
>
> See below for comments.
>
> [...]
>
>> @@ -2382,6 +2408,7 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
>> * conf->body_set = NULL;
>> * conf->body_source = { 0, NULL };
>> * conf->redirects = NULL;
>> + * conf->ssl_trusted_certificate = NULL;
>> */
>>
>> conf->upstream.store = NGX_CONF_UNSET;
>
> Nitpicking: ssl_trusted_certificate is ngx_str_t, so it's set
> to { 0, NULL } (much like body_source above).
>
> [...]
>
>> + if (conf->upstream.ssl && conf->upstream.ssl_verify) {
>> + if (conf->ssl_trusted_certificate.len == 0) {
>> + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
>> + "no \"proxy_ssl_trusted_certificate\" is "
>> + " defined for the \"proxy_ssl_verify\" "
>> + "directive");
>> +
>> + return NGX_CONF_ERROR;
>> + }
>> + }
>> +
>> + if (conf->upstream.ssl
>> + && ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
>> + &conf->ssl_trusted_certificate,
>> + conf->ssl_verify_depth)
>> + != NGX_OK)
>> + {
>> + return NGX_CONF_ERROR;
>> + }
>
> Nitpicking: if (conf->upstream.ssl) seems to be common condition,
> and probably conf->upstream.ssl_verify too. Merging the two under
> something like
>
> if (conf->upstream.ssl && conf->upstream.ssl_verify) {
>
> if (conf->ssl_trusted_certificate.len == 0) {
> ...
> }
>
> if (ngx_ssl_trusted_certificate(...)
> != NGX_OK)
> {
> ...
> }
> }
>
> should be better.
>
> [...]
>
>> @@ -1319,12 +1319,30 @@ ngx_http_upstream_ssl_handshake(ngx_conn
>> {
>> ngx_http_request_t *r;
>> ngx_http_upstream_t *u;
>> -
>> + X509 *cert;
>> +
>> r = c->data;
>> u = r->upstream;
>>
>> if (c->ssl->handshaked) {
>> -
>> + if (u->conf->ssl_verify) {
>> + if (SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
>> + ngx_log_error(NGX_LOG_ERR, c->log, 0,
>> + "upstream ssl certificate validation failed");
>
> This seems to lack detailsed error reporting. Logging at least
> error returned and X509_verify_cert_error_string() as it's done in
> ngx_http_process_request() is really good idea.
>
>> + goto fail;
>> + }
>> +
>> + cert = SSL_get_peer_certificate(c->ssl->connection);
>> +
>> + if (cert == NULL) {
>> + ngx_log_error(NGX_LOG_INFO, c->log, 0,
>> + "upstream sent no required SSL certificate");
>
> The "required" is probably not needed. And NGX_LOG_INFO logging
> level looks very wrong - while it's used by
> ngx_http_process_request(), it's used in a situation which may be
> easily triggered by broken/malicious clients, so the "info" level
> is appropiate. No certificate from an upstream server deserves at
> least "error" level (which is already used in your patch by
> previous error message).
>
>> + goto fail;
>> + }
>> +
>> + X509_free(cert);
>> + }
>> +
>> if (u->conf->ssl_session_reuse) {
>> u->peer.save_session(&u->peer, u->peer.data);
>> }
>
> One more relatively major point: certificate checking seems to
> lack any peer name validation. Without it, any certificate issued
> by a trusted certificate authority can be used, making it
> impossible to use certificate verification to prevent MITM if you
> don't control trusted CAs.
>
> I tend to think it's required for initial proxy certificate
> verification. Though probably there should be a directive to
> switch the off, like in Apache, see
> http://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslproxycheckpeername.
>
> Additionally, it might be good idea to introduce an ssl_crl
> counterpart for proxy.
>
>> @@ -1332,13 +1350,21 @@ ngx_http_upstream_ssl_handshake(ngx_conn
>> c->write->handler = ngx_http_upstream_handler;
>> c->read->handler = ngx_http_upstream_handler;
>>
>> + c = r->connection;
>> +
>> ngx_http_upstream_send_request(r, u);
>>
>> + ngx_http_run_posted_requests(c);
>> +
>> return;
>> }
>>
>> +fail:
>> + c = r->connection;
>> +
>> ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
>>
>> + ngx_http_run_posted_requests(c);
>> }
>
> Nitpicking: I think there should be empty line after "fail:".
>
> BTW, you may want to build patches against mercurial repo, it
> already has the posted requests code here. See
>
> http://nginx.org/en/docs/contributing_changes.html
>
> for basic tips.
>
> [...]
>
> --
> Maxim Dounin
> http://nginx.org/en/donation.html
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Aviram Cohen, R&D
Adallom, 1 Ha'Barzel st., Tel-Aviv, Israel
Mobile: +972 (54) 5833508
aviram at adallom.com, www.adallom.com
More information about the nginx-devel
mailing list