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
