[PATCH 2 of 2] SSL: add connection support

Maxim Dounin mdounin at mdounin.ru
Mon Aug 21 23:40:54 UTC 2017


Hello!

On Fri, Jul 28, 2017 at 01:45:37PM -0500, Nate Karstens wrote:

> # HG changeset patch
> # User Nate Karstens <nate.karstens at garmin.com>
> # Date 1501265849 18000
> #      Fri Jul 28 13:17:29 2017 -0500
> # Node ID 9537b7d299131e41a3f5993257000d328e28b117
> # Parent  7e10e1dc1fae0f538a0a74fd6783f9b33a905933
> SSL: add connection support.

It is not clear what this summary line is expected to mean.

> 
> Adds support for TLS connections using PSK cipher suites. A new
> configuration directive, ssl_psk_file, specifies the file that
> contains a list of identities and associated PSKs. Each line of
> the file begins with the identity, followed by a colon character
> (':'), and ending with the PSK. As required by RFC 4279 section
> 5.4, PSKs may be entered either as plain text or using hexadecimal
> encoding. Hexadecimal PSKs must begin with "{HEX}". PSKs without
> this prefix are assumed to be plain text, but they may optionally
> begin with "{PLAIN}" to denote this. Some examples:
> 
> gary:plain_text_password
> min:{PLAIN}another_text_password
> cliff:{HEX}ab0123CD

Looks good now, thanks.

> 
> PSK functionality can be easily tested with the OpenSSL s_client
> using the "-psk" and "-psk_identity" options.

Note that this stops working with OpenSSL 1.1.1 when using TLS 
1.3.  As of now, using PSK with TLS 1.3 in OpenSSL requires using 
a different callback (SSL_CTX_set_psk_find_session_callback()) 
with slightly different logic, and keys must match sha256 or 
sha384 length exactly.  An example code can be found in 
OpenSSL's apps/s_server.c.  This is probably something ok to 
postpone for later though.

> 
> Signed-off-by: Nate Karstens <nate.karstens at garmin.com>
> 
> diff -r 7e10e1dc1fae -r 9537b7d29913 contrib/vim/syntax/nginx.vim
> --- a/contrib/vim/syntax/nginx.vim      Fri Jul 28 13:16:27 2017 -0500
> +++ b/contrib/vim/syntax/nginx.vim      Fri Jul 28 13:17:29 2017 -0500
> @@ -550,6 +550,7 @@ syn keyword ngxDirective contained ssl_p
>  syn keyword ngxDirective contained ssl_prefer_server_ciphers
>  syn keyword ngxDirective contained ssl_preread
>  syn keyword ngxDirective contained ssl_protocols
> +syn keyword ngxDirective contained ssl_psk_file
>  syn keyword ngxDirective contained ssl_session_cache
>  syn keyword ngxDirective contained ssl_session_ticket_key
>  syn keyword ngxDirective contained ssl_session_tickets
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c     Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/event/ngx_event_openssl.c     Fri Jul 28 13:17:29 2017 -0500
> @@ -11,6 +11,7 @@
> 
> 
>  #define NGX_SSL_PASSWORD_BUFFER_SIZE  4096
> +#define NGX_SSL_PSK_BUFFER_SIZE       4096
> 
> 
>  typedef struct {
> @@ -23,6 +24,8 @@ static int ngx_ssl_password_callback(cha
>  static int ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
>  static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
>      int ret);
> +static unsigned int ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn,
> +    const char *identity, unsigned char *psk, unsigned int max_psk_len);

This callback probably should be placed after the 
ngx_ssl_psk_file() function which uses it.  And see below about 
ngx_ssl_psk_file() position.

>  static void ngx_ssl_passwords_cleanup(void *data);
>  static void ngx_ssl_handshake_handler(ngx_event_t *ev);
>  static ngx_int_t ngx_ssl_handle_recv(ngx_connection_t *c, int n);
> @@ -114,6 +117,7 @@ int  ngx_ssl_certificate_index;
>  int  ngx_ssl_next_certificate_index;
>  int  ngx_ssl_certificate_name_index;
>  int  ngx_ssl_stapling_index;
> +int  ngx_ssl_psk_index;
> 
> 
>  ngx_int_t
> @@ -225,6 +229,13 @@ ngx_ssl_init(ngx_log_t *log)
>          return NGX_ERROR;
>      }
> 
> +    ngx_ssl_psk_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);

You are allocating connection-specific index (for SSL structures), but use it
as context-specific one (in SSL_CTX structures).  This is not going to work.

Also, please note that indexes are orderd based on their type.  
This particular position is wrong for both the type you've used 
and the type it should be.

> +
> +    if (ngx_ssl_psk_index == -1) {
> +        ngx_ssl_error(NGX_LOG_ALERT, log, 0, "SSL_get_ex_new_index() failed");
> +        return NGX_ERROR;
> +    }
> +
>      return NGX_OK;
>  }
> 
> @@ -875,6 +886,138 @@ ngx_ssl_info_callback(const ngx_ssl_conn
>  }
> 
> 
> +static unsigned int
> +ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn, const char *identity,
> +    unsigned char *psk, unsigned int max_psk_len)
> +{
> +    u_char             *p, *last, *end, *psk_ptr;
> +    size_t              len;
> +    ssize_t             n;
> +    SSL_CTX            *ssl_ctx;
> +    ngx_fd_t            fd;
> +    ngx_str_t          *psk_file;
> +    ngx_int_t           psk_len;
> +    ngx_connection_t   *c;
> +    u_char              buf[NGX_SSL_PSK_BUFFER_SIZE];
> +
> +    c = ngx_ssl_get_connection(ssl_conn);
> +
> +    ssl_ctx = c->ssl->session_ctx;

Any reasons to use session-specific context here?  This will 
preclude use of virtualhost-specific PSK files, which looks like a 
bad idea.  Not to mention that the mere existence of 
session-specific contexts in c->ssl is a workaround for the 
OpenSSL session resumption logic, see 
http://hg.nginx.org/nginx/rev/97f102a13f33.

Unless there are reasons, normal SSL context should be used 
instead, as available via SSL_get_SSL_CTX().  Note though that for 
proper virtualhost support additional changes in 
ngx_http_ssl_servername() might be also needed, especially if 
callbacks will be set conditionally (see below), as OpenSSL might 
not adjust callbacks properly on SSL_set_SSL_CTX().

> +    psk_file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index);

The "psk_file" name looks excessive.  Just "file" would be fine 
here.

> +
> +    if (psk_file == NULL) {
> +        return 0;
> +    }

As the code only configures callback with a PSK file, 
this should never happen.

> +
> +    fd = ngx_open_file(psk_file->data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0);
> +    if (fd == NGX_INVALID_FILE) {
> +        ngx_ssl_error(NGX_LOG_ERR, c->log, ngx_errno,
> +                      ngx_open_file_n " \"%V\" failed", psk_file);
> +        return 0;
> +    }
> +
> +    len = 0;
> +    last = buf;
> +
> +    do {
> +        n = ngx_read_fd(fd, last, NGX_SSL_PSK_BUFFER_SIZE - len);
> +
> +        if (n == -1) {
> +            ngx_ssl_error(NGX_LOG_ERR, c->log, ngx_errno,
> +                          ngx_read_fd_n " \"%V\" failed", psk_file);
> +            psk_len = 0;
> +            goto cleanup;
> +        }
> +
> +        end = last + n;
> +
> +        if (len && n == 0) {
> +            *end++ = LF;
> +        }
> +
> +        for (p = buf; /* void */; p = last) {
> +            last = ngx_strlchr(last, end, LF);
> +
> +            if (last == NULL) {
> +                break;
> +            }
> +
> +            len = last++ - p;
> +
> +            if (len && p[len - 1] == CR) {
> +                len--;
> +            }
> +
> +            if (len == 0) {
> +                continue;
> +            }
> +
> +            psk_ptr = (u_char *) ngx_strlchr(p, p + len, ':');

There are no reasons for the cast here, ngx_strlchr() returns 
u_char *.

The "psk_ptr" name looks excessive.  Instead, I would recommend 
using something like "colon" and moving the "p" pointer to 
colon + 1 after identity check.

> +
> +            if (psk_ptr == NULL) {
> +                continue;
> +            }
> +
> +            *psk_ptr = '\0';
> +            psk_ptr++;
> +
> +            if (ngx_strcmp(p, identity) != 0) {
> +                continue;
> +            }
> +
> +            if (ngx_strncmp(psk_ptr, "{HEX}", ngx_strlen("{HEX}")) == 0) {
> +                psk_ptr += ngx_strlen("{HEX}");

Using ngx_strlen() with static strings is a bad idea.  Instead, 
sizeof(...) - 1 should be used, much like in other parts of the 
code.

> +                psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr,
> +                                         p + len - psk_ptr);
> +                if (psk_len == NGX_ERROR) {
> +                    ngx_memzero(psk, max_psk_len);
> +                    psk_len = 0;
> +                }
> +                goto cleanup;

Style: there should be more empty lines here, including one before 
"} else".  I would recommend something like this:

            if (ngx_strncmp(psk_ptr, "{HEX}", sizeof("{HEX}") - 1) == 0) {
                psk_ptr += sizeof("{HEX}") - 1;

                psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr,
                                         p + len - psk_ptr);
                if (psk_len == NGX_ERROR) {
                    ngx_memzero(psk, max_psk_len);
                    psk_len = 0;
                }

                goto cleanup;

            } else ...

> +            } else if (ngx_strncmp(psk_ptr, "{PLAIN}",
> +                       ngx_strlen("{PLAIN}")) == 0) {

Style: indentation is wrong, ngx_strlen() should be aligned with 
ngx_strncmp() arguments, "== 0" should be on its own line and 
indented to the function checked, and "{" should be on its own 
line.

And since there is no need to wrap ngx_strcmp() arguments, the 
result should look like:
        
            } else if (ngx_strncmp(psk_ptr, "{PLAIN}", sizeof("{PLAIN}") - 1)
                       == 0)
            {


> +                psk_ptr += ngx_strlen("{PLAIN}");
> +            }
> +
> +            psk_len = last - 1 - psk_ptr;

This logic is hardly readable and wrong.  Notably, it will include 
trailing "CR" into the psk_len, despite the fact that the above 
code tries to ignore it.

Instead, I would recommend to adjast "p" as suggested above, and 
adjust "len" at the same time.  This will allow to get rid of such 
error-prone calculations, and simply use "p" and "len" here and in 
other places.

> +
> +            if (psk_len > max_psk_len) {

Compilation with at least clang fails here with the following 
message:

src/event/ngx_event_openssl.c:984:25: error: comparison of integers of different
      signs: 'ngx_int_t' (aka 'int') and 'unsigned int' [-Werror,-Wsign-compare]
            if (psk_len > max_psk_len) {
                ~~~~~~~ ^ ~~~~~~~~~~~

Changing the psk_len type to "unsigned int" as it should be 
resolves this (though introduces a different problem elsewhere, to 
be resolved with ngx_hex_decode() API improvements as discussed in 
the previous patch review).

> +                psk_len = 0;
> +                goto cleanup;
> +            }
> +
> +            ngx_memcpy(psk, psk_ptr, psk_len);
> +            goto cleanup;
> +        }
> +
> +        len = end - p;
> +
> +        if (len == NGX_SSL_PSK_BUFFER_SIZE) {
> +            ngx_ssl_error(NGX_LOG_ERR, c->log, 0,
> +                          "too long line in \"%V\"", psk_file);
> +            psk_len = 0;
> +            goto cleanup;
> +        }
> +
> +        ngx_memmove(buf, p, len);
> +        last = buf + len;
> +
> +    } while (n != 0);
> +
> +cleanup:
> +
> +    if (ngx_close_file(fd) == NGX_FILE_ERROR) {
> +        ngx_ssl_error(NGX_LOG_ALERT, c->log, ngx_errno,
> +                      ngx_close_file_n " %V failed", psk_file);
> +        psk_len = 0;
> +    }
> +
> +    ngx_memzero(buf, NGX_SSL_PSK_BUFFER_SIZE);
> +
> +    return psk_len;

Compilation fails here with the following message:

src/event/ngx_event_openssl.c:1017:12: error: variable 'psk_len' may be
      uninitialized when used here [-Werror,-Wconditional-uninitialized]
    return psk_len;
           ^~~~~~~

This is indeed a bug in the code, as psk_len is never initialized 
if the identity is not found in the PSK file.

> +}
> +
> +
>  RSA *
>  ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export,
>      int key_length)
> @@ -3137,6 +3280,23 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>  #endif
> 
> 
> +ngx_int_t
> +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file)
> +{
> +#if OPENSSL_VERSION_NUMBER >= 0x1000000fL

This is not enough to compile with older OpenSSL versions, as 
there will be an unused ngx_ssl_psk_callback() function:

src/event/ngx_event_openssl.c:890:1: error: unused function
      'ngx_ssl_psk_callback' [-Werror,-Wunused-function]
ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn, const char *identity,
^

This also breaks builds with at least LibreSSL 2.5.5, as it does 
not have SSL_CTX_set_ex_data() defined, yet provides large enough 
OPENSSL_VERSION_NUMBER.  Checking for some related define 
(PSK_MAX_IDENTITY_LEN?) might be a better option.

Also, there should be empty lines after #if and before #endif, as 
the code inside #if/#endif is large enough and contains empty 
lines.

> +    if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) {

This uses "file" as written in the configuration.  Instead, it 
should resolve the name using the ngx_conf_full_name() function, 
to avoid dependencies on the working directory.  See 
ngx_ssl_dhparam() for an example.

> +        ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0,
> +                      "SSL_CTX_set_ex_data() failed");

The NGX_LOG_ALERT logging level is not appropriate here.  As the 
error is fatal and will prevent nginx from starting, it should be
NGX_LOG_EMERG.

> +        return NGX_ERROR;
> +    }
> +
> +    SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback);

Always configuring a callback is probably not a good idea.  
Instead, there should be a check if file is not empty.

In particular, this will result in cryptic messages like:

... [error] ... open() "" failed (SSL:) (2: No such file or directory) while SSL handshaking

on servers without PSK file configured if a client tries to 
use PSK.

It also results in a critictal error like:

... [crit] ... SSL_do_handshake() failed (SSL: error:1419E0DF:SSL routines:tls_process_cke_psk_preamble:psk identity not found) while SSL handshaking

which suggests that the patch needs to adjust severity levels in 
ngx_ssl_connection_error().

> +#endif
> +
> +    return NGX_OK;
> +}
> +
> +
>  void
>  ngx_ssl_cleanup_ctx(void *data)
>  {
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h     Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/event/ngx_event_openssl.h     Fri Jul 28 13:17:29 2017 -0500
> @@ -171,6 +171,7 @@ ngx_int_t ngx_ssl_session_cache(ngx_ssl_
>      ssize_t builtin_session_cache, ngx_shm_zone_t *shm_zone, time_t timeout);
>  ngx_int_t ngx_ssl_session_ticket_keys(ngx_conf_t *cf, ngx_ssl_t *ssl,
>      ngx_array_t *paths);
> +ngx_int_t ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file);
>  ngx_int_t ngx_ssl_session_cache_init(ngx_shm_zone_t *shm_zone, void *data);
>  ngx_int_t ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c,
>      ngx_uint_t flags);

The position chosen for for the function looks suboptimal, as it 
is placed between functions related to session caching.  

> @@ -255,6 +256,7 @@ extern int  ngx_ssl_certificate_index;
>  extern int  ngx_ssl_next_certificate_index;
>  extern int  ngx_ssl_certificate_name_index;
>  extern int  ngx_ssl_stapling_index;
> +extern int  ngx_ssl_psk_index;
> 
> 
>  #endif /* _NGX_EVENT_OPENSSL_H_INCLUDED_ */
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c    Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/http/modules/ngx_http_ssl_module.c    Fri Jul 28 13:17:29 2017 -0500
> @@ -234,6 +234,13 @@ static ngx_command_t  ngx_http_ssl_comma
>        offsetof(ngx_http_ssl_srv_conf_t, stapling_verify),
>        NULL },
> 
> +    { ngx_string("ssl_psk_file"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_str_slot,
> +      NGX_HTTP_SRV_CONF_OFFSET,
> +      offsetof(ngx_http_ssl_srv_conf_t, psk_file),
> +      NULL },
> +
>        ngx_null_command
>  };
> 
> @@ -539,6 +546,7 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
>       *     sscf->shm_zone = NULL;
>       *     sscf->stapling_file = { 0, NULL };
>       *     sscf->stapling_responder = { 0, NULL };
> +     *     sscf->psk_file = { 0, NULL };
>       */
> 
>      sscf->enable = NGX_CONF_UNSET;
> @@ -620,6 +628,8 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>      ngx_conf_merge_str_value(conf->stapling_responder,
>                           prev->stapling_responder, "");
> 
> +    ngx_conf_merge_str_value(conf->psk_file, prev->psk_file, "");
> +
>      conf->ssl.log = cf->log;
> 
>      if (conf->enable) {
> @@ -800,6 +810,12 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> 
>      }
> 
> +    if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file)
> +        != NGX_OK)
> +    {
> +        return NGX_CONF_ERROR;
> +    }
> +

Style: there is no need to wrap lines here, just using

    if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file) != NGX_OK) {
        return NGX_CONF_ERROR;
    }

would be fine.

>      return NGX_CONF_OK;
>  }
> 
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.h
> --- a/src/http/modules/ngx_http_ssl_module.h    Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/http/modules/ngx_http_ssl_module.h    Fri Jul 28 13:17:29 2017 -0500
> @@ -55,6 +55,8 @@ typedef struct {
>      ngx_str_t                       stapling_file;
>      ngx_str_t                       stapling_responder;
> 
> +    ngx_str_t                       psk_file;
> +
>      u_char                         *file;
>      ngx_uint_t                      line;
>  } ngx_http_ssl_srv_conf_t;
> 
> ________________________________
> 
> CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

-- 
Maxim Dounin
http://nginx.org/


More information about the nginx-devel mailing list