Hello! On Thu, Jun 04, 2015 at 04:06:38PM -0700, Andrew Ayer wrote:
> # HG changeset patch > # User Andrew Ayer <a...@andrewayer.name> > # Date 1433458802 25200 > # Node ID f231e66871bcac1aa6db81e6153f818b417f8338 > # Parent d811f22033ad2eb5c70dc4bbdda31c949a127ddb > OCSP stapling: never return an expired OCSP response (ticket #425) > > Previously, once an OCSP response was cached, it would be sent to > clients until a new response replaced it, even if the response expired. > This is bad, since Firefox displays an interstitial SSL error when it > receives an expired stapled OCSP response, and Chrome is planning to do > the same as well. > > With this change, nginx now caches the expiration time of the OCSP > response, and re-checks it before sending the response to the client. > If the response is expired, it's immediately cleared from the cache, > which prevents it from being returned, and triggers an immediate update > from the OCSP responder. > > diff -r d811f22033ad -r f231e66871bc src/event/ngx_event_openssl_stapling.c > --- a/src/event/ngx_event_openssl_stapling.c Wed Jun 03 19:12:26 2015 +0300 > +++ b/src/event/ngx_event_openssl_stapling.c Thu Jun 04 16:00:02 2015 -0700 > @@ -18,6 +18,9 @@ > ngx_str_t staple; > ngx_msec_t timeout; > > + ASN1_GENERALIZEDTIME *thisupdate; > + ASN1_GENERALIZEDTIME *nextupdate; > + > ngx_resolver_t *resolver; > ngx_msec_t resolver_timeout; > > @@ -462,6 +465,27 @@ > staple = data; > rc = SSL_TLSEXT_ERR_NOACK; > > + if (staple->thisupdate && staple->nextupdate > + && OCSP_check_validity(staple->thisupdate, > + staple->nextupdate, 300, -1) != 1) > + { I can't say I like this approach. This way, we preserve strings and do complex string operations on each handshake. And half of these operations are not needed at all, as we already checked thisupdate. The nextupdate time should be really converted to time_t and compared normally instead. I understand that OpenSSL's API is rather limited here, and doesn't provide an easy way to do the conversion. Looks like the only correct way to do it is to print a time using the ASN1_GENERALIZEDTIME_print() function, and then parse the result. Below are patches to do this. First two are preparatory: they remove unused ngx_http_get_time() declaration and move ngx_http_parse_time() to core level. Last one introduce actual changes to stapling code. # HG changeset patch # User Maxim Dounin <mdou...@mdounin.ru> # Date 1433793592 -10800 # Mon Jun 08 22:59:52 2015 +0300 # Node ID d66d1233dc74c3ca8561cdeb24ac9a618802f813 # Parent f654addf0eea52247dd6a1646ea2255b47e0e84f Removed unused ngx_http_get_time() declaration. diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -149,8 +149,6 @@ void ngx_http_clean_header(ngx_http_requ time_t ngx_http_parse_time(u_char *value, size_t len); -size_t ngx_http_get_time(char *buf, time_t t); - ngx_int_t ngx_http_discard_request_body(ngx_http_request_t *r); # HG changeset patch # User Maxim Dounin <mdou...@mdounin.ru> # Date 1433794645 -10800 # Mon Jun 08 23:17:25 2015 +0300 # Node ID c098aa9b73fe4713092be070d39b565c1610c568 # Parent d66d1233dc74c3ca8561cdeb24ac9a618802f813 Moved ngx_http_parse_time() to core, renamed accordingly. The function is now called ngx_parse_http_time(), and can be used by any code to parse HTTP-style date and time. In particular, it will be used for OCSP Stapling. For compatibility, a macro ngx_http_parse_time() provided for a while. diff --git a/auto/sources b/auto/sources --- a/auto/sources +++ b/auto/sources @@ -19,6 +19,7 @@ CORE_DEPS="src/core/nginx.h \ src/core/ngx_queue.h \ src/core/ngx_string.h \ src/core/ngx_parse.h \ + src/core/ngx_parse_time.h \ src/core/ngx_inet.h \ src/core/ngx_file.h \ src/core/ngx_crc.h \ @@ -53,6 +54,7 @@ CORE_SRCS="src/core/nginx.c \ src/core/ngx_output_chain.c \ src/core/ngx_string.c \ src/core/ngx_parse.c \ + src/core/ngx_parse_time.c \ src/core/ngx_inet.c \ src/core/ngx_file.c \ src/core/ngx_crc32.c \ @@ -303,7 +305,6 @@ HTTP_SRCS="src/http/ngx_http.c \ src/http/ngx_http_script.c \ src/http/ngx_http_upstream.c \ src/http/ngx_http_upstream_round_robin.c \ - src/http/ngx_http_parse_time.c \ src/http/modules/ngx_http_static_module.c \ src/http/modules/ngx_http_index_module.c \ src/http/modules/ngx_http_chunked_filter_module.c \ diff --git a/src/core/ngx_core.h b/src/core/ngx_core.h --- a/src/core/ngx_core.h +++ b/src/core/ngx_core.h @@ -54,6 +54,7 @@ typedef void (*ngx_connection_handler_pt #include <ngx_process.h> #include <ngx_user.h> #include <ngx_parse.h> +#include <ngx_parse_time.h> #include <ngx_log.h> #include <ngx_alloc.h> #include <ngx_palloc.h> diff --git a/src/http/ngx_http_parse_time.c b/src/core/ngx_parse_time.c rename from src/http/ngx_http_parse_time.c rename to src/core/ngx_parse_time.c --- a/src/http/ngx_http_parse_time.c +++ b/src/core/ngx_parse_time.c @@ -7,13 +7,12 @@ #include <ngx_config.h> #include <ngx_core.h> -#include <ngx_http.h> static ngx_uint_t mday[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; time_t -ngx_http_parse_time(u_char *value, size_t len) +ngx_parse_http_time(u_char *value, size_t len) { u_char *p, *end; ngx_int_t month; diff --git a/src/core/ngx_parse_time.h b/src/core/ngx_parse_time.h new file mode 100644 --- /dev/null +++ b/src/core/ngx_parse_time.h @@ -0,0 +1,22 @@ + +/* + * Copyright (C) Igor Sysoev + * Copyright (C) Nginx, Inc. + */ + + +#ifndef _NGX_PARSE_TIME_H_INCLUDED_ +#define _NGX_PARSE_TIME_H_INCLUDED_ + + +#include <ngx_config.h> +#include <ngx_core.h> + + +time_t ngx_parse_http_time(u_char *value, size_t len); + +/* compatibility */ +#define ngx_http_parse_time(value, len) ngx_parse_http_time(value, len) + + +#endif /* _NGX_PARSE_TIME_H_INCLUDED_ */ diff --git a/src/http/modules/ngx_http_dav_module.c b/src/http/modules/ngx_http_dav_module.c --- a/src/http/modules/ngx_http_dav_module.c +++ b/src/http/modules/ngx_http_dav_module.c @@ -255,7 +255,7 @@ ngx_http_dav_put_handler(ngx_http_reques ext.log = r->connection->log; if (r->headers_in.date) { - date = ngx_http_parse_time(r->headers_in.date->value.data, + date = ngx_parse_http_time(r->headers_in.date->value.data, r->headers_in.date->value.len); if (date != NGX_ERROR) { diff --git a/src/http/modules/ngx_http_headers_filter_module.c b/src/http/modules/ngx_http_headers_filter_module.c --- a/src/http/modules/ngx_http_headers_filter_module.c +++ b/src/http/modules/ngx_http_headers_filter_module.c @@ -498,7 +498,7 @@ ngx_http_set_last_modified(ngx_http_requ } r->headers_out.last_modified_time = - (value->len) ? ngx_http_parse_time(value->data, value->len) : -1; + (value->len) ? ngx_parse_http_time(value->data, value->len) : -1; return NGX_OK; } diff --git a/src/http/modules/ngx_http_not_modified_filter_module.c b/src/http/modules/ngx_http_not_modified_filter_module.c --- a/src/http/modules/ngx_http_not_modified_filter_module.c +++ b/src/http/modules/ngx_http_not_modified_filter_module.c @@ -118,7 +118,7 @@ ngx_http_test_if_unmodified(ngx_http_req return 0; } - iums = ngx_http_parse_time(r->headers_in.if_unmodified_since->value.data, + iums = ngx_parse_http_time(r->headers_in.if_unmodified_since->value.data, r->headers_in.if_unmodified_since->value.len); ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, @@ -148,7 +148,7 @@ ngx_http_test_if_modified(ngx_http_reque return 1; } - ims = ngx_http_parse_time(r->headers_in.if_modified_since->value.data, + ims = ngx_parse_http_time(r->headers_in.if_modified_since->value.data, r->headers_in.if_modified_since->value.len); ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, diff --git a/src/http/modules/ngx_http_range_filter_module.c b/src/http/modules/ngx_http_range_filter_module.c --- a/src/http/modules/ngx_http_range_filter_module.c +++ b/src/http/modules/ngx_http_range_filter_module.c @@ -204,7 +204,7 @@ ngx_http_range_header_filter(ngx_http_re goto next_filter; } - if_range_time = ngx_http_parse_time(if_range->data, if_range->len); + if_range_time = ngx_parse_http_time(if_range->data, if_range->len); ngx_log_debug2(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "http ir:%d lm:%d", diff --git a/src/http/ngx_http.h b/src/http/ngx_http.h --- a/src/http/ngx_http.h +++ b/src/http/ngx_http.h @@ -148,9 +148,6 @@ ngx_int_t ngx_http_filter_finalize_reque void ngx_http_clean_header(ngx_http_request_t *r); -time_t ngx_http_parse_time(u_char *value, size_t len); - - ngx_int_t ngx_http_discard_request_body(ngx_http_request_t *r); void ngx_http_discarded_request_body_handler(ngx_http_request_t *r); void ngx_http_block_reading(ngx_http_request_t *r); diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c --- a/src/http/ngx_http_core_module.c +++ b/src/http/ngx_http_core_module.c @@ -2195,7 +2195,7 @@ ngx_http_gzip_ok(ngx_http_request_t *r) return NGX_DECLINED; } - expires = ngx_http_parse_time(e->value.data, e->value.len); + expires = ngx_parse_http_time(e->value.data, e->value.len); if (expires == NGX_ERROR) { return NGX_DECLINED; } @@ -2203,7 +2203,7 @@ ngx_http_gzip_ok(ngx_http_request_t *r) d = r->headers_out.date; if (d) { - date = ngx_http_parse_time(d->value.data, d->value.len); + date = ngx_parse_http_time(d->value.data, d->value.len); if (date == NGX_ERROR) { return NGX_DECLINED; } diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c --- a/src/http/ngx_http_upstream.c +++ b/src/http/ngx_http_upstream.c @@ -3731,7 +3731,7 @@ ngx_http_upstream_store(ngx_http_request if (u->headers_in.last_modified) { - lm = ngx_http_parse_time(u->headers_in.last_modified->value.data, + lm = ngx_parse_http_time(u->headers_in.last_modified->value.data, u->headers_in.last_modified->value.len); if (lm != NGX_ERROR) { @@ -4128,7 +4128,7 @@ ngx_http_upstream_process_last_modified( #if (NGX_HTTP_CACHE) if (u->cacheable) { - u->headers_in.last_modified_time = ngx_http_parse_time(h->value.data, + u->headers_in.last_modified_time = ngx_parse_http_time(h->value.data, h->value.len); } @@ -4292,7 +4292,7 @@ ngx_http_upstream_process_expires(ngx_ht return NGX_OK; } - expires = ngx_http_parse_time(h->value.data, h->value.len); + expires = ngx_parse_http_time(h->value.data, h->value.len); if (expires == NGX_ERROR || expires < ngx_time()) { u->cacheable = 0; # HG changeset patch # User Maxim Dounin <mdou...@mdounin.ru> # Date 1433794648 -10800 # Mon Jun 08 23:17:28 2015 +0300 # Node ID 0dd6345e0e89ace4fca58230f75e4db4a09951bf # Parent c098aa9b73fe4713092be070d39b565c1610c568 OCSP stapling: avoid sending expired responses (ticket #425). diff --git a/src/event/ngx_event_openssl_stapling.c b/src/event/ngx_event_openssl_stapling.c --- a/src/event/ngx_event_openssl_stapling.c +++ b/src/event/ngx_event_openssl_stapling.c @@ -32,6 +32,7 @@ typedef struct { X509 *issuer; time_t valid; + time_t refresh; unsigned verify:1; unsigned loading:1; @@ -93,6 +94,8 @@ static int ngx_ssl_certificate_status_ca static void ngx_ssl_stapling_update(ngx_ssl_stapling_t *staple); static void ngx_ssl_stapling_ocsp_handler(ngx_ssl_ocsp_ctx_t *ctx); +static time_t ngx_ssl_stapling_time(ASN1_GENERALIZEDTIME *asn1time); + static void ngx_ssl_stapling_cleanup(void *data); static ngx_ssl_ocsp_ctx_t *ngx_ssl_ocsp_start(void); @@ -462,7 +465,9 @@ ngx_ssl_certificate_status_callback(ngx_ staple = data; rc = SSL_TLSEXT_ERR_NOACK; - if (staple->staple.len) { + if (staple->staple.len + && staple->valid >= ngx_time()) + { /* we have to copy ocsp response as OpenSSL will free it by itself */ p = OPENSSL_malloc(staple->staple.len); @@ -490,7 +495,7 @@ ngx_ssl_stapling_update(ngx_ssl_stapling ngx_ssl_ocsp_ctx_t *ctx; if (staple->host.len == 0 - || staple->loading || staple->valid >= ngx_time()) + || staple->loading || staple->refresh >= ngx_time()) { return; } @@ -532,6 +537,7 @@ ngx_ssl_stapling_ocsp_handler(ngx_ssl_oc u_char *p; int n; size_t len; + time_t now, valid; ngx_str_t response; X509_STORE *store; STACK_OF(X509) *chain; @@ -542,6 +548,7 @@ ngx_ssl_stapling_ocsp_handler(ngx_ssl_oc ASN1_GENERALIZEDTIME *thisupdate, *nextupdate; staple = ctx->data; + now = ngx_time(); ocsp = NULL; basic = NULL; id = NULL; @@ -629,17 +636,28 @@ ngx_ssl_stapling_ocsp_handler(ngx_ssl_oc goto error; } + valid = ngx_ssl_stapling_time(nextupdate); + if (valid == (time_t) NGX_ERROR) { + ngx_log_error(NGX_LOG_ERR, ctx->log, 0, + "invalid nextUpdate time in certificate status"); + goto error; + } + OCSP_CERTID_free(id); OCSP_BASICRESP_free(basic); OCSP_RESPONSE_free(ocsp); + id = NULL; + basic = NULL; + ocsp = NULL; + /* copy the response to memory not in ctx->pool */ response.len = len; response.data = ngx_alloc(response.len, ctx->log); if (response.data == NULL) { - goto done; + goto error; } ngx_memcpy(response.data, ctx->response->pos, response.len); @@ -653,11 +671,10 @@ ngx_ssl_stapling_ocsp_handler(ngx_ssl_oc } staple->staple = response; - -done: + staple->valid = valid; staple->loading = 0; - staple->valid = ngx_time() + 3600; /* ssl_stapling_valid */ + staple->refresh = ngx_max(ngx_min(valid, now + 3600), now + 300); ngx_ssl_ocsp_done(ctx); return; @@ -665,7 +682,7 @@ done: error: staple->loading = 0; - staple->valid = ngx_time() + 300; /* ssl_stapling_err_valid */ + staple->refresh = now + 300; if (id) { OCSP_CERTID_free(id); @@ -683,6 +700,40 @@ error: } +static time_t +ngx_ssl_stapling_time(ASN1_GENERALIZEDTIME *asn1time) +{ + u_char *value; + size_t len; + time_t time; + BIO *bio; + + /* + * OpenSSL doesn't provide a way to convert ASN1_GENERALIZEDTIME + * into time_t. To do this, we use ASN1_GENERALIZEDTIME_print(), + * which uses the "MMM DD HH:MM:SS YYYY [GMT]" format (e.g., + * "Feb 3 00:55:52 2015 GMT"), and parse the result. + */ + + bio = BIO_new(BIO_s_mem()); + if (bio == NULL) { + return NGX_ERROR; + } + + /* fake weekday prepended to match C asctime() format */ + + BIO_write(bio, "Tue ", sizeof("Tue ") - 1); + ASN1_GENERALIZEDTIME_print(bio, asn1time); + len = BIO_get_mem_data(bio, &value); + + time = ngx_parse_http_time(value, len); + + BIO_free(bio); + + return time; +} + + static void ngx_ssl_stapling_cleanup(void *data) { -- Maxim Dounin http://nginx.org/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel