[PATCH] OCSP stapling: never return an expired OCSP response (ticket #425)

Maxim Dounin mdounin at mdounin.ru
Mon Jun 8 20:35:29 UTC 2015


Hello!

On Thu, Jun 04, 2015 at 04:06:38PM -0700, Andrew Ayer wrote:

> # HG changeset patch
> # User Andrew Ayer <agwa at 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 <mdounin at 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 <mdounin at 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 <mdounin at 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/



More information about the nginx-devel mailing list