Hello!

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

> # HG changeset patch
> # User Nate Karstens <nate.karstens@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel