Skip to content

Commit

Permalink
Tls server name indication support (#310)
Browse files Browse the repository at this point in the history 
* Added a mailstream_ssl_set_server_name() function

This allows clients to enable Server Name Indication on a TLS session by
supplying the server name to be used, via a call within the session open
callback.
  • Loading branch information
@MadAlexUK @dinhvh
MadAlexUK authored and dinhvh committed Nov 3, 2018
1 parent 7edab14 commit ee87746
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 2 deletions.
16 changes: 16 additions & 0 deletions doc/API.sgml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1213,6 +1213,22 @@ mailstream * mailstream_ssl_open(int fd);
<command>mailstream_ssl_open()</command> will open a
TLS/SSL socket.
</para>

<programlisting role="C">
int mailstream_ssl_set_server_name(struct mailstream_ssl_context * ssl_context,
char * hostname)
</programlisting>

<para>
<command>mailstream_ssl_set_server_name()</command> allows the client
to enable the use of the Server Name Indication TLS extension upon
opening a TLS stream, by providing the domain name to be indicated
to server as the desired destination. <command>ssl_context</command>
is the context pointer passed to the client-supplied callback
function by <command>mailstream_ssl_open_with_callback()</command>
etc. Note that <command>hostname</command> must be a domain name, not
a string representation of an IP address.
</para>
</sect2>
</sect1>

Expand Down
65 changes: 63 additions & 2 deletions src/data-types/mailstream_ssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ struct mailstream_ssl_context
SSL_CTX * openssl_ssl_ctx;
X509* client_x509;
EVP_PKEY *client_pkey;
# if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
char * server_name;
# endif /* (OPENSSL_VERSION_NUMBER >= 0x10000000L) */
#else
gnutls_session session;
gnutls_x509_crt client_x509;
Expand Down Expand Up @@ -463,7 +466,15 @@ static struct mailstream_ssl_data * ssl_data_new_full(int fd, time_t timeout,

if (ssl_conn == NULL)
goto free_ctx;


#if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
if (ssl_context->server_name != NULL) {
SSL_set_tlsext_host_name(ssl_conn, ssl_context->server_name);
free(ssl_context->server_name);
ssl_context->server_name = NULL;
}
#endif /* (OPENSSL_VERSION_NUMBER >= 0x10000000L) */

if (SSL_set_fd(ssl_conn, fd) == 0)
goto free_ssl_conn;

Expand Down Expand Up @@ -1361,6 +1372,47 @@ int mailstream_ssl_set_server_certicate(struct mailstream_ssl_context * ssl_cont
#endif /* USE_SSL */
}

LIBETPAN_EXPORT
int mailstream_ssl_set_server_name(struct mailstream_ssl_context * ssl_context,
char * hostname)
{
int r = -1;

#ifdef USE_SSL
# ifdef USE_GNUTLS
if (hostname != NULL) {
r = gnutls_server_name_set(ssl_context->session, GNUTLS_NAME_DNS, hostname, strlen(hostname));
}
else {
r = gnutls_server_name_set(ssl_context->session, GNUTLS_NAME_DNS, "", 0U);
}
# else /* !USE_GNUTLS */
# if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
if (hostname != NULL) {
/* Unfortunately we can't set this in the openssl session yet since it
* hasn't been created yet; we only have the openssl context at this point.
* We will set it in the openssl session when we create it, soon after the
* client callback that we expect to be calling us (since it is the way the
* client gets our mailstream_ssl_context) returns (see
* ssl_data_new_full()) but we cannot rely on the client persisting it. We
* must therefore take a temporary copy here, which we free once we've set
* it in the openssl session. */
ssl_context->server_name = strdup(hostname);
}
else {
if (ssl_context->server_name != NULL) {
free(ssl_context->server_name);
}
ssl_context->server_name = NULL;
}
r = 0;
# endif /* (OPENSSL_VERSION_NUMBER >= 0x10000000L) */
# endif /* !USE_GNUTLS */
#endif /* USE_SSL */

return r;
}

#ifdef USE_SSL
#ifndef USE_GNUTLS
static struct mailstream_ssl_context * mailstream_ssl_context_new(SSL_CTX * open_ssl_ctx, int fd)
Expand All @@ -1374,15 +1426,24 @@ static struct mailstream_ssl_context * mailstream_ssl_context_new(SSL_CTX * open
ssl_ctx->openssl_ssl_ctx = open_ssl_ctx;
ssl_ctx->client_x509 = NULL;
ssl_ctx->client_pkey = NULL;
#if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
ssl_ctx->server_name = NULL;
#endif /* (OPENSSL_VERSION_NUMBER >= 0x10000000L) */
ssl_ctx->fd = fd;

return ssl_ctx;
}

static void mailstream_ssl_context_free(struct mailstream_ssl_context * ssl_ctx)
{
if (ssl_ctx)
if (ssl_ctx != NULL) {
#if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
if (ssl_ctx->server_name != NULL) {
free(ssl_ctx->server_name);
}
#endif /* (OPENSSL_VERSION_NUMBER >= 0x10000000L) */
free(ssl_ctx);
}
}
#else
static struct mailstream_ssl_context * mailstream_ssl_context_new(gnutls_session session, int fd)
Expand Down
4 changes: 4 additions & 0 deletions src/data-types/mailstream_ssl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,10 @@ LIBETPAN_EXPORT
int mailstream_ssl_set_server_certicate(struct mailstream_ssl_context * ssl_context,
char * CAfile, char * CApath);

LIBETPAN_EXPORT
int mailstream_ssl_set_server_name(struct mailstream_ssl_context * ssl_context,
char * hostname);

LIBETPAN_EXPORT
void * mailstream_ssl_get_openssl_ssl_ctx(struct mailstream_ssl_context * ssl_context);

Expand Down

4 comments on commit ee87746

@loomy
Copy link
Contributor

@loomy loomy commented on ee87746 Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this commit produces a segfault for me on a basic SSL connection without SNI

segfault at 20 ip 00000034e4433999 sp 00007fffcbe811d0 error 4 in libetpan.so.20.2.0[34e4400000+c0000]

OpenSSL> version
OpenSSL 1.0.2m 2 Nov 2017
OpenSSL>

mailstream debug:

<<<<<<< read <<<<<<
2 OK Begin TLS negotiation now.

<<<<<<< end read <<<<<<

@pmetzger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should open an issue; commenting on the commit might result in this being lost.

@loomy
Copy link
Contributor

@loomy loomy commented on ee87746 Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should open an issue; commenting on the commit might result in this being lost.

yup. just wanted to get the commiter notified not just the maintainer. I will open one if there is no response

I valgrinded the process today for some additional information

==3035== Invalid read of size 8
==3035==    at 0x3DCB233999: ssl_data_new_full (mailstream_ssl.c:471)
==3035==    by 0x3DCB233999: ssl_data_new (mailstream_ssl.c:537)
==3035==    by 0x3DCB233999: mailstream_low_ssl_open_full.isra.3 (mailstream_ssl.c:735)
==3035==    by 0x3DCB233D1A: mailstream_ssl_open_with_callback_timeout (mailstream_ssl.c:1145)
==3035==    by 0x3DCB28A0AF: mailstorage_generic_connect_with_local_address (mailstorage_tools.c:293)
==3035==    by 0x3DCB27A331: imap_connect.isra.0 (imapstorage.c:370)
==3035==    by 0x3DCB27A456: imap_mailstorage_connect (imapstorage.c:421)
==3035==  Address 0x20 is not stack'd, malloc'd or (recently) free'd
.....

==3035== 
==3035== 
==3035== Process terminating with default action of signal 11 (SIGSEGV)
==3035==  Access not within mapped region at address 0x20
==3035==    at 0x3DCB233999: ssl_data_new_full (mailstream_ssl.c:471)
==3035==    by 0x3DCB233999: ssl_data_new (mailstream_ssl.c:537)
==3035==    by 0x3DCB233999: mailstream_low_ssl_open_full.isra.3 (mailstream_ssl.c:735)
==3035==    by 0x3DCB233D1A: mailstream_ssl_open_with_callback_timeout (mailstream_ssl.c:1145)
==3035==    by 0x3DCB28A0AF: mailstorage_generic_connect_with_local_address (mailstorage_tools.c:293)
==3035==    by 0x3DCB27A331: imap_connect.isra.0 (imapstorage.c:370)
==3035==    by 0x3DCB27A456: imap_mailstorage_connect (imapstorage.c:421)
....

@pmetzger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also try doing a full build with clang using the various sanitize flags. They're amazingly good at tracking down problems.

Please sign in to comment.