[PATCH 2 of 2] SSL: add connection support
Maxim Dounin
mdounin at mdounin.ru
Mon Aug 21 23:40:54 UTC 2017
Hello!
On Fri, Jul 28, 2017 at 01:45:37PM -0500, Nate Karstens wrote:
> # HG changeset patch
> # User Nate Karstens <nate.karstens at garmin.com>
> # Date 1501265849 18000
> # Fri Jul 28 13:17:29 2017 -0500
> # Node ID 9537b7d299131e41a3f5993257000d328e28b117
> # Parent 7e10e1dc1fae0f538a0a74fd6783f9b33a905933
> SSL: add connection support.
It is not clear what this summary line is expected to mean.
>
> Adds support for TLS connections using PSK cipher suites. A new
> configuration directive, ssl_psk_file, specifies the file that
> contains a list of identities and associated PSKs. Each line of
> the file begins with the identity, followed by a colon character
> (':'), and ending with the PSK. As required by RFC 4279 section
> 5.4, PSKs may be entered either as plain text or using hexadecimal
> encoding. Hexadecimal PSKs must begin with "{HEX}". PSKs without
> this prefix are assumed to be plain text, but they may optionally
> begin with "{PLAIN}" to denote this. Some examples:
>
> gary:plain_text_password
> min:{PLAIN}another_text_password
> cliff:{HEX}ab0123CD
Looks good now, thanks.
>
> PSK functionality can be easily tested with the OpenSSL s_client
> using the "-psk" and "-psk_identity" options.
Note that this stops working with OpenSSL 1.1.1 when using TLS
1.3. As of now, using PSK with TLS 1.3 in OpenSSL requires using
a different callback (SSL_CTX_set_psk_find_session_callback())
with slightly different logic, and keys must match sha256 or
sha384 length exactly. An example code can be found in
OpenSSL's apps/s_server.c. This is probably something ok to
postpone for later though.
>
> Signed-off-by: Nate Karstens <nate.karstens at garmin.com>
>
> diff -r 7e10e1dc1fae -r 9537b7d29913 contrib/vim/syntax/nginx.vim
> --- a/contrib/vim/syntax/nginx.vim Fri Jul 28 13:16:27 2017 -0500
> +++ b/contrib/vim/syntax/nginx.vim Fri Jul 28 13:17:29 2017 -0500
> @@ -550,6 +550,7 @@ syn keyword ngxDirective contained ssl_p
> syn keyword ngxDirective contained ssl_prefer_server_ciphers
> syn keyword ngxDirective contained ssl_preread
> syn keyword ngxDirective contained ssl_protocols
> +syn keyword ngxDirective contained ssl_psk_file
> syn keyword ngxDirective contained ssl_session_cache
> syn keyword ngxDirective contained ssl_session_ticket_key
> syn keyword ngxDirective contained ssl_session_tickets
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/event/ngx_event_openssl.c Fri Jul 28 13:17:29 2017 -0500
> @@ -11,6 +11,7 @@
>
>
> #define NGX_SSL_PASSWORD_BUFFER_SIZE 4096
> +#define NGX_SSL_PSK_BUFFER_SIZE 4096
>
>
> typedef struct {
> @@ -23,6 +24,8 @@ static int ngx_ssl_password_callback(cha
> static int ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
> static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
> int ret);
> +static unsigned int ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn,
> + const char *identity, unsigned char *psk, unsigned int max_psk_len);
This callback probably should be placed after the
ngx_ssl_psk_file() function which uses it. And see below about
ngx_ssl_psk_file() position.
> static void ngx_ssl_passwords_cleanup(void *data);
> static void ngx_ssl_handshake_handler(ngx_event_t *ev);
> static ngx_int_t ngx_ssl_handle_recv(ngx_connection_t *c, int n);
> @@ -114,6 +117,7 @@ int ngx_ssl_certificate_index;
> int ngx_ssl_next_certificate_index;
> int ngx_ssl_certificate_name_index;
> int ngx_ssl_stapling_index;
> +int ngx_ssl_psk_index;
>
>
> ngx_int_t
> @@ -225,6 +229,13 @@ ngx_ssl_init(ngx_log_t *log)
> return NGX_ERROR;
> }
>
> + ngx_ssl_psk_index = SSL_get_ex_new_index(0, NULL, NULL, NULL, NULL);
You are allocating connection-specific index (for SSL structures), but use it
as context-specific one (in SSL_CTX structures). This is not going to work.
Also, please note that indexes are orderd based on their type.
This particular position is wrong for both the type you've used
and the type it should be.
> +
> + if (ngx_ssl_psk_index == -1) {
> + ngx_ssl_error(NGX_LOG_ALERT, log, 0, "SSL_get_ex_new_index() failed");
> + return NGX_ERROR;
> + }
> +
> return NGX_OK;
> }
>
> @@ -875,6 +886,138 @@ ngx_ssl_info_callback(const ngx_ssl_conn
> }
>
>
> +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, *psk_ptr;
> + size_t len;
> + ssize_t n;
> + SSL_CTX *ssl_ctx;
> + ngx_fd_t fd;
> + ngx_str_t *psk_file;
> + ngx_int_t psk_len;
> + ngx_connection_t *c;
> + u_char buf[NGX_SSL_PSK_BUFFER_SIZE];
> +
> + c = ngx_ssl_get_connection(ssl_conn);
> +
> + ssl_ctx = c->ssl->session_ctx;
Any reasons to use session-specific context here? This will
preclude use of virtualhost-specific PSK files, which looks like a
bad idea. Not to mention that the mere existence of
session-specific contexts in c->ssl is a workaround for the
OpenSSL session resumption logic, see
http://hg.nginx.org/nginx/rev/97f102a13f33.
Unless there are reasons, normal SSL context should be used
instead, as available via SSL_get_SSL_CTX(). Note though that for
proper virtualhost support additional changes in
ngx_http_ssl_servername() might be also needed, especially if
callbacks will be set conditionally (see below), as OpenSSL might
not adjust callbacks properly on SSL_set_SSL_CTX().
> + psk_file = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_psk_index);
The "psk_file" name looks excessive. Just "file" would be fine
here.
> +
> + if (psk_file == NULL) {
> + return 0;
> + }
As the code only configures callback with a PSK file,
this should never happen.
> +
> + fd = ngx_open_file(psk_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", psk_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", psk_file);
> + psk_len = 0;
> + 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;
> + }
> +
> + psk_ptr = (u_char *) ngx_strlchr(p, p + len, ':');
There are no reasons for the cast here, ngx_strlchr() returns
u_char *.
The "psk_ptr" name looks excessive. Instead, I would recommend
using something like "colon" and moving the "p" pointer to
colon + 1 after identity check.
> +
> + if (psk_ptr == NULL) {
> + continue;
> + }
> +
> + *psk_ptr = '\0';
> + psk_ptr++;
> +
> + if (ngx_strcmp(p, identity) != 0) {
> + continue;
> + }
> +
> + if (ngx_strncmp(psk_ptr, "{HEX}", ngx_strlen("{HEX}")) == 0) {
> + psk_ptr += ngx_strlen("{HEX}");
Using ngx_strlen() with static strings is a bad idea. Instead,
sizeof(...) - 1 should be used, much like in other parts of the
code.
> + psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr,
> + p + len - psk_ptr);
> + if (psk_len == NGX_ERROR) {
> + ngx_memzero(psk, max_psk_len);
> + psk_len = 0;
> + }
> + goto cleanup;
Style: there should be more empty lines here, including one before
"} else". I would recommend something like this:
if (ngx_strncmp(psk_ptr, "{HEX}", sizeof("{HEX}") - 1) == 0) {
psk_ptr += sizeof("{HEX}") - 1;
psk_len = ngx_hex_decode(psk, max_psk_len, psk_ptr,
p + len - psk_ptr);
if (psk_len == NGX_ERROR) {
ngx_memzero(psk, max_psk_len);
psk_len = 0;
}
goto cleanup;
} else ...
> + } else if (ngx_strncmp(psk_ptr, "{PLAIN}",
> + ngx_strlen("{PLAIN}")) == 0) {
Style: indentation is wrong, ngx_strlen() should be aligned with
ngx_strncmp() arguments, "== 0" should be on its own line and
indented to the function checked, and "{" should be on its own
line.
And since there is no need to wrap ngx_strcmp() arguments, the
result should look like:
} else if (ngx_strncmp(psk_ptr, "{PLAIN}", sizeof("{PLAIN}") - 1)
== 0)
{
> + psk_ptr += ngx_strlen("{PLAIN}");
> + }
> +
> + psk_len = last - 1 - psk_ptr;
This logic is hardly readable and wrong. Notably, it will include
trailing "CR" into the psk_len, despite the fact that the above
code tries to ignore it.
Instead, I would recommend to adjast "p" as suggested above, and
adjust "len" at the same time. This will allow to get rid of such
error-prone calculations, and simply use "p" and "len" here and in
other places.
> +
> + if (psk_len > max_psk_len) {
Compilation with at least clang fails here with the following
message:
src/event/ngx_event_openssl.c:984:25: error: comparison of integers of different
signs: 'ngx_int_t' (aka 'int') and 'unsigned int' [-Werror,-Wsign-compare]
if (psk_len > max_psk_len) {
~~~~~~~ ^ ~~~~~~~~~~~
Changing the psk_len type to "unsigned int" as it should be
resolves this (though introduces a different problem elsewhere, to
be resolved with ngx_hex_decode() API improvements as discussed in
the previous patch review).
> + psk_len = 0;
> + goto cleanup;
> + }
> +
> + ngx_memcpy(psk, psk_ptr, psk_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\"", psk_file);
> + psk_len = 0;
> + 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", psk_file);
> + psk_len = 0;
> + }
> +
> + ngx_memzero(buf, NGX_SSL_PSK_BUFFER_SIZE);
> +
> + return psk_len;
Compilation fails here with the following message:
src/event/ngx_event_openssl.c:1017:12: error: variable 'psk_len' may be
uninitialized when used here [-Werror,-Wconditional-uninitialized]
return psk_len;
^~~~~~~
This is indeed a bug in the code, as psk_len is never initialized
if the identity is not found in the PSK file.
> +}
> +
> +
> RSA *
> ngx_ssl_rsa512_key_callback(ngx_ssl_conn_t *ssl_conn, int is_export,
> int key_length)
> @@ -3137,6 +3280,23 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
> #endif
>
>
> +ngx_int_t
> +ngx_ssl_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file)
> +{
> +#if OPENSSL_VERSION_NUMBER >= 0x1000000fL
This is not enough to compile with older OpenSSL versions, as
there will be an unused ngx_ssl_psk_callback() function:
src/event/ngx_event_openssl.c:890:1: error: unused function
'ngx_ssl_psk_callback' [-Werror,-Wunused-function]
ngx_ssl_psk_callback(ngx_ssl_conn_t *ssl_conn, const char *identity,
^
This also breaks builds with at least LibreSSL 2.5.5, as it does
not have SSL_CTX_set_ex_data() defined, yet provides large enough
OPENSSL_VERSION_NUMBER. Checking for some related define
(PSK_MAX_IDENTITY_LEN?) might be a better option.
Also, there should be empty lines after #if and before #endif, as
the code inside #if/#endif is large enough and contains empty
lines.
> + if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_psk_index, file) == 0) {
This uses "file" as written in the configuration. Instead, it
should resolve the name using the ngx_conf_full_name() function,
to avoid dependencies on the working directory. See
ngx_ssl_dhparam() for an example.
> + ngx_ssl_error(NGX_LOG_ALERT, ssl->log, 0,
> + "SSL_CTX_set_ex_data() failed");
The NGX_LOG_ALERT logging level is not appropriate here. As the
error is fatal and will prevent nginx from starting, it should be
NGX_LOG_EMERG.
> + return NGX_ERROR;
> + }
> +
> + SSL_CTX_set_psk_server_callback(ssl->ctx, ngx_ssl_psk_callback);
Always configuring a callback is probably not a good idea.
Instead, there should be a check if file is not empty.
In particular, this will result in cryptic messages like:
... [error] ... open() "" failed (SSL:) (2: No such file or directory) while SSL handshaking
on servers without PSK file configured if a client tries to
use PSK.
It also results in a critictal error like:
... [crit] ... SSL_do_handshake() failed (SSL: error:1419E0DF:SSL routines:tls_process_cke_psk_preamble:psk identity not found) while SSL handshaking
which suggests that the patch needs to adjust severity levels in
ngx_ssl_connection_error().
> +#endif
> +
> + return NGX_OK;
> +}
> +
> +
> void
> ngx_ssl_cleanup_ctx(void *data)
> {
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/event/ngx_event_openssl.h Fri Jul 28 13:17:29 2017 -0500
> @@ -171,6 +171,7 @@ ngx_int_t ngx_ssl_session_cache(ngx_ssl_
> ssize_t builtin_session_cache, ngx_shm_zone_t *shm_zone, time_t timeout);
> 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_psk_file(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *file);
> ngx_int_t ngx_ssl_session_cache_init(ngx_shm_zone_t *shm_zone, void *data);
> ngx_int_t ngx_ssl_create_connection(ngx_ssl_t *ssl, ngx_connection_t *c,
> ngx_uint_t flags);
The position chosen for for the function looks suboptimal, as it
is placed between functions related to session caching.
> @@ -255,6 +256,7 @@ extern int ngx_ssl_certificate_index;
> extern int ngx_ssl_next_certificate_index;
> extern int ngx_ssl_certificate_name_index;
> extern int ngx_ssl_stapling_index;
> +extern int ngx_ssl_psk_index;
>
>
> #endif /* _NGX_EVENT_OPENSSL_H_INCLUDED_ */
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/http/modules/ngx_http_ssl_module.c Fri Jul 28 13:17:29 2017 -0500
> @@ -234,6 +234,13 @@ static ngx_command_t ngx_http_ssl_comma
> offsetof(ngx_http_ssl_srv_conf_t, stapling_verify),
> NULL },
>
> + { ngx_string("ssl_psk_file"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_TAKE1,
> + ngx_conf_set_str_slot,
> + NGX_HTTP_SRV_CONF_OFFSET,
> + offsetof(ngx_http_ssl_srv_conf_t, psk_file),
> + NULL },
> +
> ngx_null_command
> };
>
> @@ -539,6 +546,7 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
> * sscf->shm_zone = NULL;
> * sscf->stapling_file = { 0, NULL };
> * sscf->stapling_responder = { 0, NULL };
> + * sscf->psk_file = { 0, NULL };
> */
>
> sscf->enable = NGX_CONF_UNSET;
> @@ -620,6 +628,8 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> ngx_conf_merge_str_value(conf->stapling_responder,
> prev->stapling_responder, "");
>
> + ngx_conf_merge_str_value(conf->psk_file, prev->psk_file, "");
> +
> conf->ssl.log = cf->log;
>
> if (conf->enable) {
> @@ -800,6 +810,12 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
>
> }
>
> + if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file)
> + != NGX_OK)
> + {
> + return NGX_CONF_ERROR;
> + }
> +
Style: there is no need to wrap lines here, just using
if (ngx_ssl_psk_file(cf, &conf->ssl, &conf->psk_file) != NGX_OK) {
return NGX_CONF_ERROR;
}
would be fine.
> return NGX_CONF_OK;
> }
>
> diff -r 7e10e1dc1fae -r 9537b7d29913 src/http/modules/ngx_http_ssl_module.h
> --- a/src/http/modules/ngx_http_ssl_module.h Fri Jul 28 13:16:27 2017 -0500
> +++ b/src/http/modules/ngx_http_ssl_module.h Fri Jul 28 13:17:29 2017 -0500
> @@ -55,6 +55,8 @@ typedef struct {
> ngx_str_t stapling_file;
> ngx_str_t stapling_responder;
>
> + ngx_str_t psk_file;
> +
> u_char *file;
> ngx_uint_t line;
> } ngx_http_ssl_srv_conf_t;
>
> ________________________________
>
> 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list