From 250f0ed84ac892ea85654790cb83331dcbd8d44f Mon Sep 17 00:00:00 2001 From: Matthias Andree Date: Sun, 26 Nov 2006 10:11:39 +0000 Subject: [PATCH] First step towards really fixing TLS vuln, CVE-2006-5867, still incomplete. svn path=/branches/BRANCH_6-3/; revision=4962 --- Makefile.am | 2 +- fetchmail.h | 4 ++ imap.c | 108 ++++++++++++++++++++++++++-------------------------- pop3.c | 104 +++++++++++++++++++++++++------------------------- tls.c | 33 ++++++++++++++++ 5 files changed, 145 insertions(+), 106 deletions(-) create mode 100644 tls.c diff --git a/Makefile.am b/Makefile.am index 27b302e9..31ac75dd 100644 --- a/Makefile.am +++ b/Makefile.am @@ -39,7 +39,7 @@ libfm_a_SOURCES= xmalloc.c base64.c rfc822.c report.c rfc2047e.c \ servport.c ntlm.h smbbyteorder.h smbdes.h smbmd4.h \ smbencrypt.h smbdes.c smbencrypt.c smbmd4.c smbutil.c \ libesmtp/gethostbyname.h libesmtp/gethostbyname.c \ - smbtypes.h fm_getaddrinfo.c + smbtypes.h fm_getaddrinfo.c tls.c libfm_a_LIBADD= $(EXTRAOBJ) libfm_a_DEPENDENCIES= $(EXTRAOBJ) LDADD = libfm.a @LIBINTL@ $(LIBOBJS) diff --git a/fetchmail.h b/fetchmail.h index e5e1dda1..3d6866f7 100644 --- a/fetchmail.h +++ b/fetchmail.h @@ -767,5 +767,9 @@ int servport(const char *service); int fm_getaddrinfo(const char *node, const char *serv, const struct addrinfo *hints, struct addrinfo **res); void fm_freeaddrinfo(struct addrinfo *ai); +/* prototypes from tls.c */ +int maybe_tls(struct query *ctl); +int must_tls(struct query *ctl); + #endif /* fetchmail.h ends here */ diff --git a/imap.c b/imap.c index c2d36370..23db2188 100644 --- a/imap.c +++ b/imap.c @@ -369,6 +369,10 @@ static int imap_getauth(int sock, struct query *ctl, char *greeting) /* apply for connection authorization */ { int ok = 0; +#ifdef SSL_ENABLE + int got_tls = 0; + char *realhost; +#endif (void)greeting; /* @@ -393,64 +397,62 @@ static int imap_getauth(int sock, struct query *ctl, char *greeting) } #ifdef SSL_ENABLE - if ((!ctl->sslproto || !strcasecmp(ctl->sslproto,"tls1")) - && !ctl->use_ssl - && strstr(capabilities, "STARTTLS")) - { - char *realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname; - - /* Use "tls1" rather than ctl->sslproto because tls1 is the only - * protocol that will work with STARTTLS. Don't need to worry - * whether TLS is mandatory or opportunistic unless SSLOpen() fails - * (see below). */ - if (gen_transact(sock, "STARTTLS") == PS_SUCCESS - && SSLOpen(sock, ctl->sslcert, ctl->sslkey, "tls1", ctl->sslcertck, - ctl->sslcertpath, ctl->sslfingerprint, realhost, - ctl->server.pollname, &ctl->remotename) != -1) + realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname; + + if (maybe_tls(ctl)) { + if (strstr(capabilities, "STARTTLS")) { - /* - * RFC 2595 says this: - * - * "Once TLS has been started, the client MUST discard cached - * information about server capabilities and SHOULD re-issue the - * CAPABILITY command. This is necessary to protect against - * man-in-the-middle attacks which alter the capabilities list prior - * to STARTTLS. The server MAY advertise different capabilities - * after STARTTLS." - * - * Now that we're confident in our TLS connection we can - * guarantee a secure capability re-probe. - */ - capa_probe(sock, ctl); - if (outlevel >= O_VERBOSE) + /* Use "tls1" rather than ctl->sslproto because tls1 is the only + * protocol that will work with STARTTLS. Don't need to worry + * whether TLS is mandatory or opportunistic unless SSLOpen() fails + * (see below). */ + if (gen_transact(sock, "STARTTLS") == PS_SUCCESS + && SSLOpen(sock, ctl->sslcert, ctl->sslkey, "tls1", ctl->sslcertck, + ctl->sslcertpath, ctl->sslfingerprint, realhost, + ctl->server.pollname, &ctl->remotename) != -1) { - report(stdout, GT_("%s: upgrade to TLS succeeded.\n"), realhost); + /* + * RFC 2595 says this: + * + * "Once TLS has been started, the client MUST discard cached + * information about server capabilities and SHOULD re-issue the + * CAPABILITY command. This is necessary to protect against + * man-in-the-middle attacks which alter the capabilities list prior + * to STARTTLS. The server MAY advertise different capabilities + * after STARTTLS." + * + * Now that we're confident in our TLS connection we can + * guarantee a secure capability re-probe. + */ + got_tls = 1; + capa_probe(sock, ctl); + if (outlevel >= O_VERBOSE) + { + report(stdout, GT_("%s: upgrade to TLS succeeded.\n"), realhost); + } } } - else if (ctl->sslfingerprint || ctl->sslcertck - || (ctl->sslproto && !strcasecmp(ctl->sslproto, "tls1"))) - { - /* Config required TLS but we couldn't guarantee it, so we must - * stop. */ - report(stderr, GT_("%s: upgrade to TLS failed.\n"), realhost); - return PS_SOCKET; - } - else - { - if (outlevel >= O_VERBOSE) - { - report(stdout, GT_("%s: opportunistic upgrade to TLS failed, trying to continue\n"), realhost); - } - /* We don't know whether the connection is in a working state, so - * test by issuing a NOOP. */ - if (gen_transact(sock, "NOOP") != PS_SUCCESS) - { - /* Not usable. Empty sslproto to force an unencrypted - * connection on the next attempt, and repoll. */ - ctl->sslproto = xstrdup(""); - return PS_REPOLL; + + if (!got_tls) { + if (must_tls(ctl)) { + /* Config required TLS but we couldn't guarantee it, so we must + * stop. */ + report(stderr, GT_("%s: upgrade to TLS failed.\n"), realhost); + return PS_SOCKET; + } else { + if (outlevel >= O_VERBOSE) { + report(stdout, GT_("%s: opportunistic upgrade to TLS failed, trying to continue\n"), realhost); + } + /* We don't know whether the connection is in a working state, so + * test by issuing a NOOP. */ + if (gen_transact(sock, "NOOP") != PS_SUCCESS) { + /* Not usable. Empty sslproto to force an unencrypted + * connection on the next attempt, and repoll. */ + ctl->sslproto = xstrdup(""); + return PS_REPOLL; + } + /* Usable. Proceed with authenticating insecurely. */ } - /* Usable. Proceed with authenticating insecurely. */ } } #endif /* SSL_ENABLE */ diff --git a/pop3.c b/pop3.c index e637ab0e..b41d4e53 100644 --- a/pop3.c +++ b/pop3.c @@ -304,6 +304,7 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting) #ifdef SSL_ENABLE char *realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname; flag connection_may_have_tls_errors = FALSE; + flag got_tls = FALSE; #endif /* SSL_ENABLE */ #if defined(GSSAPI) @@ -440,60 +441,59 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting) } #ifdef SSL_ENABLE - if (has_stls - && !ctl->use_ssl - /* opportunistic or forced TLS */ - && (!ctl->sslproto || !strcasecmp(ctl->sslproto, "tls1"))) - { - /* Use "tls1" rather than ctl->sslproto because tls1 is the only - * protocol that will work with STARTTLS. Don't need to worry - * whether TLS is mandatory or opportunistic unless SSLOpen() fails - * (see below). */ - if (gen_transact(sock, "STLS") == PS_SUCCESS - && SSLOpen(sock, ctl->sslcert, ctl->sslkey, "tls1", ctl->sslcertck, + if (maybe_tls(ctl)) { + if (has_stls) + { + /* Use "tls1" rather than ctl->sslproto because tls1 is the only + * protocol that will work with STARTTLS. Don't need to worry + * whether TLS is mandatory or opportunistic unless SSLOpen() fails + * (see below). */ + if (gen_transact(sock, "STLS") == PS_SUCCESS + && SSLOpen(sock, ctl->sslcert, ctl->sslkey, "tls1", ctl->sslcertck, ctl->sslcertpath, ctl->sslfingerprint, realhost, ctl->server.pollname, &ctl->remotename) != -1) - { - /* - * RFC 2595 says this: - * - * "Once TLS has been started, the client MUST discard cached - * information about server capabilities and SHOULD re-issue the - * CAPABILITY command. This is necessary to protect against - * man-in-the-middle attacks which alter the capabilities list prior - * to STARTTLS. The server MAY advertise different capabilities - * after STARTTLS." - * - * Now that we're confident in our TLS connection we can - * guarantee a secure capability re-probe. - */ - (void)capa_probe(sock); - if (outlevel >= O_VERBOSE) - { - report(stdout, GT_("%s: upgrade to TLS succeeded.\n"), realhost); - } - } - else if (ctl->sslfingerprint || ctl->sslcertck - || (ctl->sslproto && !strcasecmp(ctl->sslproto, "tls1"))) - { - /* Config required TLS but we couldn't guarantee it, so we must - * stop. */ - report(stderr, GT_("%s: upgrade to TLS failed.\n"), realhost); - return PS_SOCKET; - } - else - { - /* We don't know whether the connection is usable, and there's - * no command we can reasonably issue to test it (NOOP isn't - * allowed til post-authentication), so leave it in an unknown - * state, mark it as such, and check more carefully if things - * go wrong when we try to authenticate. */ - connection_may_have_tls_errors = TRUE; - if (outlevel >= O_VERBOSE) - { - report(stdout, GT_("%s: opportunistic upgrade to TLS failed, trying to continue.\n"), realhost); - } - } + { + /* + * RFC 2595 says this: + * + * "Once TLS has been started, the client MUST discard cached + * information about server capabilities and SHOULD re-issue the + * CAPABILITY command. This is necessary to protect against + * man-in-the-middle attacks which alter the capabilities list prior + * to STARTTLS. The server MAY advertise different capabilities + * after STARTTLS." + * + * Now that we're confident in our TLS connection we can + * guarantee a secure capability re-probe. + */ + got_tls = TRUE; + (void)capa_probe(sock); + if (outlevel >= O_VERBOSE) + { + report(stdout, GT_("%s: upgrade to TLS succeeded.\n"), realhost); + } + } + } + + if (!got_tls) { + if (must_tls(ctl)) { + /* Config required TLS but we couldn't guarantee it, so we must + * stop. */ + report(stderr, GT_("%s: upgrade to TLS failed.\n"), realhost); + return PS_SOCKET; + } else { + /* We don't know whether the connection is usable, and there's + * no command we can reasonably issue to test it (NOOP isn't + * allowed til post-authentication), so leave it in an unknown + * state, mark it as such, and check more carefully if things + * go wrong when we try to authenticate. */ + connection_may_have_tls_errors = TRUE; + if (outlevel >= O_VERBOSE) + { + report(stdout, GT_("%s: opportunistic upgrade to TLS failed, trying to continue.\n"), realhost); + } + } + } } #endif /* SSL_ENABLE */ diff --git a/tls.c b/tls.c new file mode 100644 index 00000000..2a1fee3e --- /dev/null +++ b/tls.c @@ -0,0 +1,33 @@ +/** \file tls.c - collect common TLS functionality + * \author Matthias Andree + * \year 2006 + */ + +#include "fetchmail.h" + +#ifdef HAVE_STRINGS_H +#include +#endif + +/** return true if user allowed TLS */ +int maybe_tls(struct query *ctl) { +#ifdef SSL_ENABLE + /* opportunistic or forced TLS */ + return (!ctl->sslproto || !strcasecmp(ctl->sslproto,"tls1")) + && !ctl->use_ssl; +#else + return 0; +#endif +} + +/** return true if user requires TLS, note though that this code must + * always use a logical AND with maybe_tls(). */ +int must_tls(struct query *ctl) { +#ifdef SSL_ENABLE + return maybe_tls(ctl) + && (ctl->sslfingerprint || ctl->sslcertck + || (ctl->sslproto && !strcasecmp(ctl->sslproto, "tls1"))); +#else + return 0; +#endif +} -- 2.43.2