Proposed changeset to fix client cert from ngx_ssl_get_certificate passed as HTTP header value

[Sam McKelvie]

The ngx_ssl_get_certificate() changes “\n” to “\n\t” in the returned PEM string in an effort to make
the string usable as an HTTP header value with $ssl_client_cert. However, bare ‘\n’ (without a preceding ‘\r’) is passed
along as “\n\t". This causes some HTTP servers (including node/express) to hang up. This changeset
fixes the problem by replacing occurrences of ‘\n’ that have no preceding ‘\r’ with "\r\n\t".

Tested with node.js/express and nginx-tests.

I should note that a similar solution was proposed at https://forum.nginx.org/read.php?29,249804,249833 <https://forum.nginx.org/read.php?29,249804,249833>, but the thread never went anywhere.
This solution is slightly more paranoid with edge cases and does not insert extra ‘\r’ characters if they are already present.

Thank you,
Sam McKelvie


# HG changeset patch
# User Sam McKelvie <sam at surround.io <mailto:sam at surround.io>>
# Date 1454541255 28800
#      Wed Feb 03 15:14:15 2016 -0800
# Node ID d70dae44196b5625f10080b4b869f33adbb83f54
# Parent  0e0e2e522fa2cb439b636a2c19aca5bdfbe8704f
Fix ngx_ssl_get_certificate to properly format for HTTP header value.

Previously, the value could contain lines ending in '\n' rather than
'\r\n'. This caused some HTTP servers, including node/express, to
reject the request entirely.

Now, instances of bare '\n' are replaced with "\r\n\t". Instances of
"\r\n" are still replaced with "\r\n\t".

Tested with node.js/express and nginx-tests.

diff -r 0e0e2e522fa2 -r d70dae44196b src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c	Tue Feb 02 16:33:55 2016 +0300
+++ b/src/event/ngx_event_openssl.c	Wed Feb 03 15:14:15 2016 -0800
@@ -3212,7 +3212,10 @@
         goto failed;
     }
 
-    BIO_read(bio, s->data, len);
+    if (BIO_read(bio, s->data, len) != (int)len) {
+      ngx_ssl_error(NGX_LOG_ALERT, c->log, 0, "BIO_read() failed");
+      goto failed;
+    }
 
     BIO_free(bio);
     X509_free(cert);
@@ -3235,22 +3238,41 @@
     size_t       len;
     ngx_uint_t   i;
     ngx_str_t    cert;
+    int          prev_cr;
 
     if (ngx_ssl_get_raw_certificate(c, pool, &cert) != NGX_OK) {
         return NGX_ERROR;
     }
 
+    /* Strip off trailing whitespace (last newline) from the cert */
+    while (cert.len > 0 && (
+                cert.data[cert.len-1] == CR ||
+                cert.data[cert.len-1] == LF ||
+                cert.data[cert.len-1] == ' ')) {
+      --cert.len;
+    }
+
     if (cert.len == 0) {
         s->len = 0;
         return NGX_OK;
     }
 
-    len = cert.len - 1;
-
-    for (i = 0; i < cert.len - 1; i++) {
+    /* 
+     * To make PEM text compatible as an HTTP header value, convert
+     * "\r\n" to "\r\n\t", and convert "\n" to "\r\n\t". Bare '\n'
+     * is rejected by some HTTP servers including node/express. 
+     */ 
+    prev_cr = 0;
+    len = 0;
+    for (i = 0; i < cert.len; i++) {
         if (cert.data[i] == LF) {
-            len++;
+            if (!prev_cr) {
+              len++; /* Insert CR before LF*/
+            }
+            len++; /* Insert '\t' after LF */
         }
+        len++;
+        prev_cr = (cert.data[i] == CR);
     }
 
     s->len = len;
@@ -3261,11 +3283,18 @@
 
     p = s->data;
 
-    for (i = 0; i < cert.len - 1; i++) {
+    prev_cr = 0;
+    for (i = 0; i < cert.len; i++) {
+      if (cert.data[i] == LF) {
+        if (!prev_cr) {
+          *p++ = CR;
+        }
+        *p++ = LF;
+        *p++ = '\t';
+      } else {
         *p++ = cert.data[i];
-        if (cert.data[i] == LF) {
-            *p++ = '\t';
-        }
+      }
+      prev_cr = (cert.data[i] == CR);
     }
 
     return NGX_OK;


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20160203/0023dbf5/attachment.html>