[PATCH] [PATCH 2 of 4] SSL: add support for PSK cipher suites
Karstens, Nate
Nate.Karstens at garmin.com
Fri Sep 1 13:17:15 UTC 2017
> 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:nginx-devel-bounces at nginx.org] On Behalf Of Maxim Dounin
Sent: Thursday, August 31, 2017 9:44 AM
To: nginx-devel at nginx.org
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 <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/
_______________________________________________
nginx-devel mailing list
nginx-devel at nginx.org
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.
More information about the nginx-devel
mailing list