> In the previous review I've pointed out that the patch needs to adjust > severity levels in ngx_ssl_connection_error():
Sorry, I think I got confused with something else. Nate -----Original Message----- From: nginx-devel [mailto:[email protected]] On Behalf Of Maxim Dounin Sent: Thursday, August 31, 2017 9:44 AM To: [email protected] Subject: Re: [PATCH] [PATCH 2 of 4] SSL: add support for PSK cipher suites Hello! On Wed, Aug 23, 2017 at 09:20:50PM -0500, Nate Karstens wrote: > # HG changeset patch > # User Nate Karstens <[email protected]> # 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 [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel ________________________________ 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 [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
