[PATCH] [PATCH 2 of 4] SSL: add support for PSK cipher suites

Maxim Dounin mdounin at mdounin.ru
Thu Aug 31 14:43:45 UTC 2017


Hello!

On Wed, Aug 23, 2017 at 09:20:50PM -0500, Nate Karstens wrote:

> # HG changeset patch
> # User Nate Karstens <nate.karstens at garmin.com>
> # Date 1503540059 18000
> #      Wed Aug 23 21:00:59 2017 -0500
> # Node ID 97953fe374455a04973268c4b2fbadd7ced91ffe
> # Parent  17c038b56051f922ec440d40e23e8d1b23d835df
> [PATCH 2 of 4] SSL: add support for PSK cipher suites.

There is no need to include "[PATCH 2 of 4] " into patch summary 
line.

[...]

> @@ -1163,6 +1177,211 @@ ngx_ssl_ecdh_curve(ngx_conf_t *cf, ngx_s
> 
> 
>  ngx_int_t
> +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file)
> +{
> +#ifdef PSK_MAX_IDENTITY_LEN
> +
> +    ngx_fd_t            fd;
> +    ngx_int_t           err;
> +    ngx_file_info_t     fi;

Style: there should be two spaces between the longest type and a 
variable.  I've already explained details in one of the previous 
reviews here:

http://mailman.nginx.org/pipermail/nginx-devel/2017-June/010247.html 

> +
> +    err = NGX_OK;
> +
> +    if (ngx_conf_full_name(cf->cycle, file, 1) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +
> +    fd = ngx_open_file(file->data, NGX_FILE_RDONLY, NGX_FILE_OPEN, 0);
> +    if (fd == NGX_INVALID_FILE) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno,
> +                      ngx_open_file_n " \"%V\" failed", file);
> +        return NGX_ERROR;
> +    }
> +
> +    if (ngx_fd_info(fd, &fi) == NGX_FILE_ERROR) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno,
> +                      ngx_fd_info_n " \"%V\" failed", file);
> +        err = NGX_ERROR;
> +        goto failed;
> +    }
> +
> +    if (ngx_file_size(&fi) == 0) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                      "PSK file \"%V\" is empty", file);
> +        err = NGX_ERROR;
> +        goto failed;
> +    }

What's the point in opening the file here?  As long the file is 
being read at run-time, it doesn't seem to make sense.  The file 
can be legitimately created later.

Checking ngx_file_size() looks completely wrong as well.  It will 
prevent configurations with no currently configured PSK keys from 
running if the file size is 0 - but won't do this as long as there 
is a comment and/or some garbage in the file.

> +
> +failed:
> +
> +    if (ngx_close_file(fd) == NGX_FILE_ERROR) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, ngx_errno,
> +                      ngx_close_file_n " %V failed", file);
> +        err = NGX_ERROR;
> +    }
> +
> +    if (err != NGX_OK) {
> +        return err;
> +    }
> +
> +    if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) {
> +        ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
> +                      "SSL_CTX_set_ex_data() failed");
> +        return NGX_ERROR;
> +    }
> +
> +    SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback);
> +
> +#endif
> +
> +    return NGX_OK;
> +}
> +
> +
> +#ifdef PSK_MAX_IDENTITY_LEN
> +
> +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, *colon;
> +    size_t              len;
> +    ssize_t             n;
> +    SSL_CTX            *ssl_ctx;
> +    ngx_fd_t            fd;
> +    ngx_str_t          *file;
> +    unsigned int        psk_len;
> +    ngx_connection_t   *c;
> +    u_char              buf[NGX_SSL_PSK_BUFFER_SIZE];

Style: there should be two spaces between "ngx_connection_t" and 
"*c".

> +
> +    psk_len = 0;

This looks to early for the psk_len initialization.  Rather, I 
would move the initialization to somewhere near "len = 0; last = 
buf" below where it will look more logical.

> +    c = ngx_ssl_get_connection(ssl_conn);
> +
> +    ssl_ctx = SSL_get_SSL_CTX(ssl_conn);
> +    file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index);
> +
> +    fd = ngx_open_file(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", 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", file);
> +            psk_len = 0;

There is no need to set psk_len to 0 here, as it is already 
initialized to 0 and never set to anything else except before 
"goto cleanup".

> +            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;
> +            }
> +
> +            colon = ngx_strlchr(p, p + len, ':');
> +
> +            if (colon == NULL) {
> +                continue;
> +            }
> +
> +            *colon = '\0';
> +
> +            if (ngx_strcmp(p, identity) != 0) {
> +                continue;
> +            }
> +
> +            len -= colon + 1 - p;
> +            p = colon + 1;
> +
> +            if (ngx_strncmp(p, "{HEX}", sizeof("{HEX}") - 1) == 0) {
> +
> +                p += sizeof("{HEX}") - 1;
> +                len -= sizeof("{HEX}") - 1;
> +
> +                if (len / 2 > max_psk_len) {
> +                    goto cleanup;
> +                }
> +
> +                if (ngx_hex_decode(psk, p, len) != NGX_OK) {
> +                    ngx_memzero(psk, len / 2);
> +                    goto cleanup;
> +                }
> +
> +                psk_len = len / 2;
> +
> +                goto cleanup;
> +
> +            } else if (ngx_strncmp(p, "{PLAIN}", sizeof("{PLAIN}") - 1) == 0) {
> +                p += sizeof("{PLAIN}") - 1;
> +                len -= sizeof("{PLAIN}") - 1;
> +            }
> +
> +            if (len > max_psk_len) {
> +                goto cleanup;
> +            }
> +
> +            ngx_memcpy(psk, p, len);
> +            psk_len = 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\"", file);
> +            psk_len = 0;

There is no need to set psk_len here, see above.

> +            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", file);
> +        psk_len = 0;

I don't think we should reset psk_len here, this error is not 
related to the PSK key we already either have at this point, or 
psk_len is already set to 0.

> +    }
> +
> +    ngx_memzero(buf, NGX_SSL_PSK_BUFFER_SIZE);
> +
> +    return psk_len;
> +}
> +
> +#endif
> +
> +
> +ngx_int_t
>  ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c, ngx_uint_t flags)
>  {
>      ngx_ssl_connection_t  *sc;

In the previous review I've pointed out that the patch needs to 
adjust severity levels in ngx_ssl_connection_error():

http://mailman.nginx.org/pipermail/nginx-devel/2017-August/010419.html

Are there any specific reasons why you've ignored this comment?  
Just in case, here is the relevant change:

@@ -2249,6 +2249,9 @@ ngx_ssl_connection_error(ngx_connection_
             || n == SSL_R_NO_COMPRESSION_SPECIFIED                   /*  187 */
             || n == SSL_R_NO_SHARED_CIPHER                           /*  193 */
             || n == SSL_R_RECORD_LENGTH_MISMATCH                     /*  213 */
+#ifdef SSL_R_PSK_IDENTITY_NOT_FOUND
+            || n == SSL_R_PSK_IDENTITY_NOT_FOUND                     /*  223 */
+#endif
 #ifdef SSL_R_PARSE_TLSEXT
             || n == SSL_R_PARSE_TLSEXT                               /*  227 */
 #endif


> diff -r 17c038b56051 -r 97953fe37445 src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h     Wed Aug 23 21:00:18 2017 -0500
> +++ b/src/event/ngx_event_openssl.h     Wed Aug 23 21:00:59 2017 -0500
> @@ -172,6 +172,7 @@ ngx_int_t ngx_ssl_session_cache(ngx_ssl_
>  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_session_cache_init(ngx_shm_zone_t *shm_zone, void *data);
> +ngx_int_t ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file);
>  ngx_int_t ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c,
>      ngx_uint_t flags);
> 

I would rather place it after the ngx_ssl_ecdh_curve() function, 
similar to how it is placed in the C file.

[...]

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


More information about the nginx-devel mailing list