From 2745813367777f406d917b8a42f309f8c4da9953 Mon Sep 17 00:00:00 2001 From: Oswald Buddenhagen Date: Sat, 12 Jul 2014 20:35:55 +0200 Subject: [PATCH] re-design SSL/TLS configuration the combinations of the various options made quite a mess. additionally, 'RequireSSL no' is inherently insecure - "use SSL if available" is plain stupid. the old options are still accepted, but will elicit a warning. --- NEWS | 4 ++ src/config.c | 5 +- src/config.h | 5 ++ src/drv_imap.c | 151 ++++++++++++++++++++++++++++++++++--------------- src/mbsync.1 | 58 ++++++------------- src/socket.c | 10 ++-- src/socket.h | 12 +++- 7 files changed, 147 insertions(+), 98 deletions(-) diff --git a/NEWS b/NEWS index 63f9d78..a4ff5ec 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,10 @@ The 'isync' compatibility wrapper is now deprecated. +The SSL/TLS configuration has been re-designed. +SSL is now explicitly enabled or disabled - "use SSL if available" is gone. +Notice: Tunnels are assumed to be secure and thus default to no SSL. + [1.1.0] Support for hierarchical mailboxes in Patterns. diff --git a/src/config.c b/src/config.c index cf3b165..2ac3e70 100644 --- a/src/config.c +++ b/src/config.c @@ -36,10 +36,7 @@ store_conf_t *stores; -#define ARG_OPTIONAL 0 -#define ARG_REQUIRED 1 - -static char * +char * get_arg( conffile_t *cfile, int required, int *comment ) { char *ret, *p, *t; diff --git a/src/config.h b/src/config.h index b9b4bfe..e3bc72f 100644 --- a/src/config.h +++ b/src/config.h @@ -35,6 +35,11 @@ typedef struct conffile { char *cmd, *val, *rest; } conffile_t; +#define ARG_OPTIONAL 0 +#define ARG_REQUIRED 1 + +char *get_arg( conffile_t *cfile, int required, int *comment ); + int parse_bool( conffile_t *cfile ); int parse_int( conffile_t *cfile ); int parse_size( conffile_t *cfile ); diff --git a/src/drv_imap.c b/src/drv_imap.c index 299aa23..8981bb4 100644 --- a/src/drv_imap.c +++ b/src/drv_imap.c @@ -36,6 +36,10 @@ #include #include +#ifdef HAVE_LIBSSL +enum { SSL_None, SSL_STARTTLS, SSL_IMAPS }; +#endif + typedef struct imap_server_conf { struct imap_server_conf *next; char *name; @@ -45,9 +49,7 @@ typedef struct imap_server_conf { char *pass_cmd; int max_in_progress; #ifdef HAVE_LIBSSL - char use_ssl; - char use_imaps; - char require_ssl; + char ssl_type; char require_cram; #endif } imap_server_conf_t; @@ -1532,7 +1534,7 @@ imap_open_store_connected( int ok, void *aux ) if (!ok) imap_open_store_bail( ctx ); #ifdef HAVE_LIBSSL - else if (srvc->use_imaps) + else if (srvc->ssl_type == SSL_IMAPS) socket_start_tls( &ctx->conn, imap_open_store_tlsstarted1 ); #endif } @@ -1582,26 +1584,21 @@ imap_open_store_authenticate( imap_store_t *ctx ) if (ctx->greeting != GreetingPreauth) { #ifdef HAVE_LIBSSL - if (!srvc->use_imaps && srvc->use_ssl) { - /* always try to select SSL support if available */ + if (srvc->ssl_type == SSL_STARTTLS) { if (CAP(STARTTLS)) { imap_exec( ctx, 0, imap_open_store_authenticate_p2, "STARTTLS" ); return; } else { - if (srvc->require_ssl) { - error( "IMAP error: SSL support not available\n" ); - imap_open_store_bail( ctx ); - return; - } else { - warn( "IMAP warning: SSL support not available\n" ); - } + error( "IMAP error: SSL support not available\n" ); + imap_open_store_bail( ctx ); + return; } } #endif imap_open_store_authenticate2( ctx ); } else { #ifdef HAVE_LIBSSL - if (!srvc->use_imaps && srvc->require_ssl) { + if (srvc->ssl_type == SSL_STARTTLS) { error( "IMAP error: SSL support not available\n" ); imap_open_store_bail( ctx ); return; @@ -2237,8 +2234,13 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) { imap_store_conf_t *store; imap_server_conf_t *server, *srv, sserver; - const char *type, *name; + const char *type, *name, *arg; int acc_opt = 0; +#ifdef HAVE_LIBSSL + /* Legacy SSL options */ + int require_ssl = -1, use_imaps = -1; + int use_sslv2 = -1, use_sslv3 = -1, use_tlsv1 = -1, use_tlsv11 = -1, use_tlsv12 = -1; +#endif if (!strcasecmp( "IMAPAccount", cfg->cmd )) { server = nfcalloc( sizeof(*server) ); @@ -2259,32 +2261,30 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) return 0; #ifdef HAVE_LIBSSL - /* this will probably annoy people, but its the best default just in - * case people forget to turn it on - */ - server->require_ssl = 1; - server->sconf.use_tlsv1 = 1; + server->ssl_type = -1; + server->sconf.ssl_versions = -1; #endif server->max_in_progress = INT_MAX; while (getcline( cfg ) && cfg->cmd) { if (!strcasecmp( "Host", cfg->cmd )) { /* The imap[s]: syntax is just a backwards compat hack. */ + arg = cfg->val; #ifdef HAVE_LIBSSL - if (starts_with( cfg->val, -1, "imaps:", 6 )) { - cfg->val += 6; - server->use_imaps = 1; - server->sconf.use_sslv2 = 1; - server->sconf.use_sslv3 = 1; + if (starts_with( arg, -1, "imaps:", 6 )) { + arg += 6; + server->ssl_type = SSL_IMAPS; + if (server->sconf.ssl_versions == -1) + server->sconf.ssl_versions = SSLv2 | SSLv3 | TLSv1; } else #endif - { - if (starts_with( cfg->val, -1, "imap:", 5 )) - cfg->val += 5; - } - if (starts_with( cfg->val, -1, "//", 2 )) - cfg->val += 2; - server->sconf.host = nfstrdup( cfg->val ); + if (starts_with( arg, -1, "imap:", 5 )) + arg += 5; + if (!memcmp( "//", arg, 2 )) + arg += 2; + if (arg != cfg->val) + warn( "%s:%d: Notice: URL notation is deprecated; use a plain host name and possibly 'SSLType IMAPS' instead\n", cfg->file, cfg->line ); + server->sconf.host = nfstrdup( arg ); } else if (!strcasecmp( "User", cfg->cmd )) server->user = nfstrdup( cfg->val ); @@ -2308,20 +2308,51 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) cfg->file, cfg->line, server->sconf.cert_file ); cfg->err = 1; } + } else if (!strcasecmp( "SSLType", cfg->cmd )) { + if (!strcasecmp( "None", cfg->val )) { + server->ssl_type = SSL_None; + } else if (!strcasecmp( "STARTTLS", cfg->val )) { + server->ssl_type = SSL_STARTTLS; + } else if (!strcasecmp( "IMAPS", cfg->val )) { + server->ssl_type = SSL_IMAPS; + } else { + error( "%s:%d: Invalid SSL type\n", cfg->file, cfg->line ); + cfg->err = 1; + } + } else if (!strcasecmp( "SSLVersion", cfg->cmd ) || + !strcasecmp( "SSLVersions", cfg->cmd )) { + server->sconf.ssl_versions = 0; + arg = cfg->val; + do { + if (!strcasecmp( "SSLv2", arg )) { + server->sconf.ssl_versions |= SSLv2; + } else if (!strcasecmp( "SSLv3", arg )) { + server->sconf.ssl_versions |= SSLv3; + } else if (!strcasecmp( "TLSv1", arg )) { + server->sconf.ssl_versions |= TLSv1; + } else if (!strcasecmp( "TLSv1.1", arg )) { + server->sconf.ssl_versions |= TLSv1_1; + } else if (!strcasecmp( "TLSv1.2", arg )) { + server->sconf.ssl_versions |= TLSv1_2; + } else { + error( "%s:%d: Unrecognized SSL version\n", cfg->file, cfg->line ); + cfg->err = 1; + } + } while ((arg = get_arg( cfg, ARG_OPTIONAL, 0 ))); } else if (!strcasecmp( "RequireSSL", cfg->cmd )) - server->require_ssl = parse_bool( cfg ); + require_ssl = parse_bool( cfg ); else if (!strcasecmp( "UseIMAPS", cfg->cmd )) - server->use_imaps = parse_bool( cfg ); + use_imaps = parse_bool( cfg ); else if (!strcasecmp( "UseSSLv2", cfg->cmd )) - server->sconf.use_sslv2 = parse_bool( cfg ); + use_sslv2 = parse_bool( cfg ); else if (!strcasecmp( "UseSSLv3", cfg->cmd )) - server->sconf.use_sslv3 = parse_bool( cfg ); + use_sslv3 = parse_bool( cfg ); else if (!strcasecmp( "UseTLSv1", cfg->cmd )) - server->sconf.use_tlsv1 = parse_bool( cfg ); + use_tlsv1 = parse_bool( cfg ); else if (!strcasecmp( "UseTLSv1.1", cfg->cmd )) - server->sconf.use_tlsv11 = parse_bool( cfg ); + use_tlsv11 = parse_bool( cfg ); else if (!strcasecmp( "UseTLSv1.2", cfg->cmd )) - server->sconf.use_tlsv12 = parse_bool( cfg ); + use_tlsv12 = parse_bool( cfg ); else if (!strcasecmp( "RequireCRAM", cfg->cmd )) server->require_cram = parse_bool( cfg ); #endif @@ -2369,19 +2400,45 @@ imap_parse_store( conffile_t *cfg, store_conf_t **storep ) return 1; } #ifdef HAVE_LIBSSL - server->use_ssl = - server->sconf.use_sslv2 | server->sconf.use_sslv3 | - server->sconf.use_tlsv1 | server->sconf.use_tlsv11 | server->sconf.use_tlsv12; - if (server->require_ssl && !server->use_ssl) { - error( "%s '%s' requires SSL but no SSL versions enabled\n", type, name ); - cfg->err = 1; - return 1; + if ((use_sslv2 & use_sslv3 & use_tlsv1 & use_tlsv11 & use_tlsv12) != -1 || use_imaps >= 0 || require_ssl >= 0) { + if (server->ssl_type >= 0 || server->sconf.ssl_versions >= 0) { + error( "%s '%s': The deprecated UseSSL*, UseTLS*, UseIMAPS, and RequireSSL options are mutually exlusive with SSLType and SSLVersions.\n", type, name ); + cfg->err = 1; + return 1; + } + warn( "Notice: %s '%s': UseSSL*, UseTLS*, UseIMAPS, and RequireSSL are deprecated. Use SSLType and SSLVersions instead.\n", type, name ); + server->sconf.ssl_versions = + (use_sslv2 != 1 ? 0 : SSLv2) | + (use_sslv3 != 1 ? 0 : SSLv3) | + (use_tlsv1 == 0 ? 0 : TLSv1) | + (use_tlsv11 != 1 ? 0 : TLSv1_1) | + (use_tlsv12 != 1 ? 0 : TLSv1_2); + if (use_imaps == 1) { + server->ssl_type = SSL_IMAPS; + } else if (require_ssl) { + server->ssl_type = SSL_STARTTLS; + } else if (!server->sconf.ssl_versions) { + server->ssl_type = SSL_None; + } else { + warn( "Notice: %s '%s': 'RequireSSL no' is being ignored\n", type, name ); + server->ssl_type = SSL_STARTTLS; + } + if (server->ssl_type != SSL_None && !server->sconf.ssl_versions) { + error( "%s '%s' requires SSL but no SSL versions enabled\n", type, name ); + cfg->err = 1; + return 1; + } + } else { + if (server->sconf.ssl_versions < 0) + server->sconf.ssl_versions = TLSv1; /* Most compatible and still reasonably secure. */ + if (server->ssl_type < 0) + server->ssl_type = server->sconf.tunnel ? SSL_None : SSL_STARTTLS; } #endif if (!server->sconf.port) server->sconf.port = #ifdef HAVE_LIBSSL - server->use_imaps ? 993 : + server->ssl_type == SSL_IMAPS ? 993 : #endif 143; } diff --git a/src/mbsync.1 b/src/mbsync.1 index 2af9178..fe24f8a 100644 --- a/src/mbsync.1 +++ b/src/mbsync.1 @@ -271,11 +271,6 @@ optional. Specify a command to run to establish a connection rather than opening a TCP socket. This allows you to run an IMAP session over an SSH tunnel, for example. -.br -If \fBUseIMAPS\fR is disabled and the tunnel opens a preauthenticated -connection, \fBRequireSSL\fR also needs to be disabled. -If the connection is not preauthenticated, but the tunnel is secure, -disabling \fBRequireSSL\fR and \fBUseTLSv1\fR is recommended. .. .TP \fBRequireCRAM\fR \fIyes\fR|\fIno\fR @@ -283,18 +278,26 @@ If set to \fIyes\fR, \fBmbsync\fR will abort the connection if no CRAM-MD5 authentication is possible. (Default: \fIno\fR) .. .TP -\fBUseIMAPS\fR \fIyes\fR|\fIno\fR -If set to \fIyes\fR, the default for \fBPort\fR is changed to 993 and -\fBmbsync\fR will start SSL negotiation immediately after establishing -the connection to the server. +\fBSSLType\fR {\fINone\fR|\fISTARTTLS\fR|\fIIMAPS\fR} +Select the connection security/encryption method: .br -Note that modern servers support SSL on the regular IMAP port 143 via the -STARTTLS extension, which will be used automatically by default. +\fINone\fR - no security. +This is the default when \fBTunnel\fR is set, as tunnels are usually secure. +.br +\fISTARTTLS\fR - security is established via the STARTTLS extension +after connecting the regular IMAP port 143. Most servers support this, +so it is the default (unless a tunnel is used). +.br +\fIIMAPS\fR - security is established by starting SSL/TLS negotiation +right after connecting the secure IMAP port 993. .. .TP -\fBRequireSSL\fR \fIyes\fR|\fIno\fR -\fBmbsync\fR will abort the connection if a TLS/SSL session cannot be -established with the IMAP server. (Default: \fIyes\fR) +\fBSSLVersions\fR [\fISSLv2\fR] [\fISSLv3\fR] [\fITLSv1\fR] [\fITLSv1.1\fR] [\fITLSv1.2\fR] +Select the acceptable SSL/TLS versions. +Use of SSLv2 is strongly discouraged for security reasons, but might be the +only option on some very old servers. +Generally, the newest TLS version is recommended, but as this confuses some +servers, \fBTLSv1\fR is the default. .. .TP \fBCertificateFile\fR \fIpath\fR @@ -306,33 +309,6 @@ Note that the system's default certificate store is always used and should not be specified here. .. .TP -\fBUseSSLv2\fR \fIyes\fR|\fIno\fR -Use SSLv2 for communication with the IMAP server over SSL? -.br -Note that this option is deprecated for security reasons. -(Default: \fIno\fR) -.. -.TP -\fBUseSSLv3\fR \fIyes\fR|\fIno\fR -Use SSLv3 for communication with the IMAP server over SSL? -(Default: \fIno\fR) -.. -.TP -\fBUseTLSv1\fR \fIyes\fR|\fIno\fR -Use TLSv1 for communication with the IMAP server over SSL? -(Default: \fIyes\fR) -.. -.TP -\fBUseTLSv1.1\fR \fIyes\fR|\fIno\fR -Use TLSv1.1 for communication with the IMAP server over SSL? -(Default: \fIno\fR) -.. -.TP -\fBUseTLSv1.2\fR \fIyes\fR|\fIno\fR -Use TLSv1.2 for communication with the IMAP server over SSL? -(Default: \fIno\fR) -.. -.TP \fBPipelineDepth\fR \fIdepth\fR Maximum number of IMAP commands which can be simultaneously in flight. Setting this to \fI1\fR disables pipelining. diff --git a/src/socket.c b/src/socket.c index 4bc2752..c760c0a 100644 --- a/src/socket.c +++ b/src/socket.c @@ -205,18 +205,18 @@ init_ssl_ctx( const server_conf_t *conf ) mconf->SSLContext = SSL_CTX_new( SSLv23_client_method() ); - if (!conf->use_sslv2) + if (!(conf->ssl_versions & SSLv2)) options |= SSL_OP_NO_SSLv2; - if (!conf->use_sslv3) + if (!(conf->ssl_versions & SSLv3)) options |= SSL_OP_NO_SSLv3; - if (!conf->use_tlsv1) + if (!(conf->ssl_versions & TLSv1)) options |= SSL_OP_NO_TLSv1; #ifdef SSL_OP_NO_TLSv1_1 - if (!conf->use_tlsv11) + if (!(conf->ssl_versions & TLSv1_1)) options |= SSL_OP_NO_TLSv1_1; #endif #ifdef SSL_OP_NO_TLSv1_2 - if (!conf->use_tlsv12) + if (!(conf->ssl_versions & TLSv1_2)) options |= SSL_OP_NO_TLSv1_2; #endif diff --git a/src/socket.h b/src/socket.h index c7fcb78..bfc5ddf 100644 --- a/src/socket.h +++ b/src/socket.h @@ -25,16 +25,26 @@ #include "common.h" +#ifdef HAVE_LIBSSL typedef struct ssl_st SSL; typedef struct ssl_ctx_st SSL_CTX; +enum { + SSLv2 = 1, + SSLv3 = 2, + TLSv1 = 4, + TLSv1_1 = 8, + TLSv1_2 = 16 +}; +#endif + typedef struct server_conf { char *tunnel; char *host; int port; #ifdef HAVE_LIBSSL char *cert_file; - char use_sslv2, use_sslv3, use_tlsv1, use_tlsv11, use_tlsv12; + char ssl_versions; /* these are actually variables and are leaked at the end */ char ssl_ctx_valid;