Welcome! Log In Create A New Profile

Advanced

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

Karstens, Nate
September 01, 2017 09:18AM
> 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@nginx.org] On Behalf Of Maxim Dounin
Sent: Thursday, August 31, 2017 9:44 AM
To: nginx-devel@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@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

________________________________

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
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

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

Nate Karstens 682 August 23, 2017 10:22PM

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

Maxim Dounin 290 August 31, 2017 10:44AM

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

Karstens, Nate 391 September 01, 2017 09:18AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 308
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready