Fix TLS issue: fail if sslfingerprint, sslproto tls1 or sslcertck are configured and STARTTLS fails. Only by omitting all of these options, fetchmail will try opportunistic TLS.

svn path=/branches/BRANCH_6-3/; revision=4929

5 files changed, 149 insertions, 139 deletions

diff --git a/NEWS b/NEWS
index 489ff523..7981a6e0 100644
--- a/NEWS
+++ b/NEWS
@@ -41,6 +41,23 @@ change.  MA = Matthias Andree, ESR = Eric S. Raymond, RF = Rob Funk.)
 
 fetchmail 6.3.6 (not yet released):
 
+# SECURITY FIX (INCOMPATIBLE):
+* Using at least one of the options "sslproto 'tls1'", "sslfingerprint" or
+  "sslcertck" enforces STARTTLS for POP3 and IMAP and terminates the connection
+  if unsuccessful. The same configuration causes permanent connection failure
+  with POP2 unless --ssl is used.
+
+  fetchmail 6.3.5 and older had no way to enforce TLS. With those older
+  versions, TLS was always opportunistic, but fetchmail would happily transmit
+  the password in cleartext if STARTTLS failed. --ssl configurations however
+  have been safe.
+
+  Reported by and fixed in cooperation with Isaac Wilcox.
+
+# BUG FIXES:
+* Repair --logfile, broken in 6.3.5. BerliOS Bug #9059,
+  reported by Brian Harring.
+
 # KNOWN BUGS AND WORKAROUNDS:
   (this section floats upwards through the NEWS to be on top of the list)
 * fetchmail does not handle messages without Message-ID header well
@@ -57,15 +74,6 @@ fetchmail 6.3.6 (not yet released):
 * some of the logging output is not very helpful
 * some of the documentation is still not up to date
 
-# IMPORTANT CHANGE:
-* sslproto 'tls1' enforces STARTTLS for POP3/IMAP and terminates the connection
-  if unsuccessful. The same configuration causes connection failure with POP2.
-  Reported by Isaac Wilcox.
-
-# BUG FIXES:
-* Repair --logfile, broken in 6.3.5. BerliOS Bug #9059,
-  reported by Brian Harring.
-
 fetchmail 6.3.5 (released 2006-10-09):
 
 # BUG FIXES:
diff --git a/fetchmail.man b/fetchmail.man
index 9f267f57..aa82d765 100644
--- a/fetchmail.man
+++ b/fetchmail.man
@@ -421,12 +421,10 @@ on context.
 (Keyword: sslcertck)
 Causes fetchmail to strictly check the server certificate against a set of
 local trusted certificates (see the \fBsslcertpath\fR option). If the server
-certificate is not signed by one of the trusted ones (directly or indirectly),
-the SSL connection will fail, regardless of the \fBsslfingerprint\fR
-option. This checking is required, but not sufficient, to prevent
-man-in-the-middle attacks against the SSL connection. Use \-\-ssl or
-\-\-sslproto to enforce SSL or TLS. Note that CRLs are seemingly not
-currently supported by OpenSSL in certificate verification! Your system
+certificate cannot be obtained or is not signed by one of the trusted ones
+(directly or indirectly), the SSL connection will fail, regardless of
+the \fBsslfingerprint\fR option.
+Note that CRL are only supported in OpenSSL 0.9.7 and newer! Your system
 clock should be reasonably accurate when using this option.
 .IP
 Note that this optional behavior may become default behavior in future
@@ -448,7 +446,8 @@ hex digits must be in upper case. This is the default format OpenSSL uses,
 and the one fetchmail uses to report the fingerprint when an SSL connection
 is established. When this is specified, fetchmail will compare the server key
 fingerprint with the given one, and the connection will fail if they do not
-match regardless of the \fBsslcertck\fR setting.
+match regardless of the \fBsslcertck\fR setting. The connection will
+also fail if fetchmail cannot obtain an SSL certificate from the server.
 This can be used to prevent man-in-the-middle attacks, but the finger
 print from the server needs to be obtained or verified over a secure
 channel, and certainly not over the same Internet connection that
@@ -1056,12 +1055,14 @@ protocols (default: v2 or v3).  The \-\-sslcertck command line or
 sslcertck run control file option should be used to force strict
 certificate checking - see below.
 .PP
-If SSL is not configured, fetchmail may opportunistically try to use
-TLS. It can be forced to use TLS by using \-\-sslproto "TLS1". TLS
+If SSL is not configured, fetchmail will usually opportunistically try to use
+TLS. TLS can be enforced by using \-\-sslproto "TLS1". TLS
 connections use the same port as the unencrypted version of the
-protocol. The \-\-sslcertck command line or sslcertck run control file
-option should be used to force strict certificate checking - see below.
+protocol and negotiate TLS via special parameter. The \-\-sslcertck
+command line or sslcertck run control file option should be used to
+force strict certificate checking - see below.
 .PP
+.B \-\-sslcheck recommended:
 When connecting to an SSL or TLS encrypted server, the server presents a certificate
 to the client for validation.  The certificate is checked to verify that
 the common name in the certificate matches the name of the server being
@@ -1086,13 +1087,13 @@ the certificate files is that required by the underlying SSL libraries
 .PP
 A word of care about the use of SSL: While above mentioned
 setup with self-signed server certificates retrieved over the wires
-can protect you from a passive eavesdropper it doesn't help against an
+can protect you from a passive eavesdropper, it doesn't help against an
 active attacker. It's clearly an improvement over sending the
-passwords in clear but you should be aware that a man-in-the-middle
+passwords in clear, but you should be aware that a man-in-the-middle
 attack is trivially possible (in particular with tools such as dsniff,
 http://monkey.org/~dugsong/dsniff/).  Use of strict certificate checking
 with a certification authority recognized by server and client, or
-perhaps of an ssh tunnel (see below for some examples) is preferable if
+perhaps of an SSH tunnel (see below for some examples) is preferable if
 you care seriously about the security of your mailbox and passwords.
 .SS ESMTP AUTH
 .PP
diff --git a/imap.c b/imap.c
index 5c1d8236..c2d36370 100644
--- a/imap.c
+++ b/imap.c
@@ -369,12 +369,8 @@ static int imap_getauth(int sock, struct query *ctl, char *greeting)
 /* apply for connection authorization */
 {
     int ok = 0;
-#ifdef SSL_ENABLE
-    flag did_stls = FALSE;
-    flag using_tls = FALSE;
-#endif /* SSL_ENABLE */
-
     (void)greeting;
+
     /*
      * Assumption: expunges are cheap, so we want to do them
      * after every message unless user said otherwise.
@@ -398,54 +394,64 @@ 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"))
+ 	&& !ctl->use_ssl
+	&& strstr(capabilities, "STARTTLS"))
     {
-           char *realhost;
-
-           realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname;
-           ok = gen_transact(sock, "STARTTLS");
-
-           /* We use "tls1" instead of ctl->sslproto, as we want STARTTLS,
-            * not other SSL protocols
-            */
-           if (ok == PS_SUCCESS &&
-	       SSLOpen(sock,ctl->sslcert,ctl->sslkey,"tls1",ctl->sslcertck,
-		   ctl->sslcertpath,ctl->sslfingerprint,
-		   realhost,ctl->server.pollname,&ctl->remotename) == -1)
-           {
-	       if (!ctl->sslproto && !ctl->wehaveauthed)
-	       {
-		   ctl->sslproto = xstrdup("");
-		   /* repoll immediately with TLS disabled */
-		   return(PS_REPOLL);
-	       }
-               report(stderr,
-                      GT_("TLS connection failed.\n"));
-               return PS_SOCKET;
-           } else {
-	       using_tls = TRUE;
-	       if (outlevel >= O_VERBOSE && !ctl->sslproto)
-		   report(stdout, GT_("%s: opportunistic upgrade to TLS.\n"), realhost);
-	   }
-	   did_stls = TRUE;
-
-	   /*
-	    * 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."
-	    */
-	   capa_probe(sock, ctl);
-    }
-    /* Check if TLS was enforced. */
-    if ((ctl->sslproto && !strcasecmp(ctl->sslproto,"tls1")) && !ctl->use_ssl && !using_tls) {
-	report(stderr, GT_("TLS connection failed.\n"));
-	return PS_SOCKET;
+	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)
+	{
+	    /*
+	     * 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)
+	    {
+		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;
+	    }
+	    /* Usable.  Proceed with authenticating insecurely. */
+	}
     }
 #endif /* SSL_ENABLE */
 
@@ -605,16 +611,6 @@ static int imap_getauth(int sock, struct query *ctl, char *greeting)
 	shroud[0] = '\0';
 	free(password);
 	free(remotename);
-#ifdef SSL_ENABLE
-	/* this is for servers which claim to support TLS, but actually
-	 * don't! */
-	if (did_stls && ok == PS_SOCKET && !ctl->sslproto && !ctl->wehaveauthed)
-	{
-	    ctl->sslproto = xstrdup("");
-	    /* repoll immediately */
-	    ok = PS_REPOLL;
-	}
-#endif
 	if (ok)
 	{
 	    /* SASL cancellation of authentication */
diff --git a/pop2.c b/pop2.c
index 8bfefb4c..fe00cd6c 100644
--- a/pop2.c
+++ b/pop2.c
@@ -63,7 +63,7 @@ static int pop2_getauth(int sock, struct query *ctl, char *buf)
     (void)buf;
     if (ctl->sslproto && !strcasecmp(ctl->sslproto, "tls1") && !ctl->use_ssl)
     {
-	report(stderr, GT_("POP2 does not support STARTTLS. Giving up.\n"));
+	report(stderr, GT_("POP2 does not support STLS. Giving up.\n"));
 	return PS_SOCKET;
     }
     strlcpy(shroud, ctl->password, sizeof(shroud));
diff --git a/pop3.c b/pop3.c
index c38fbf67..e637ab0e 100644
--- a/pop3.c
+++ b/pop3.c
@@ -52,7 +52,7 @@ static flag has_cram = FALSE;
 flag has_otp = FALSE;
 #endif /* OPIE_ENABLE */
 #ifdef SSL_ENABLE
-static flag has_ssl = FALSE;
+static flag has_stls = FALSE;
 #endif /* SSL_ENABLE */
 
 /* mailbox variables initialized in pop3_getrange() */
@@ -261,7 +261,7 @@ static int capa_probe(int sock)
 		break;
 #ifdef SSL_ENABLE
 	    if (strstr(buffer, "STLS"))
-		has_ssl = TRUE;
+		has_stls = TRUE;
 #endif /* SSL_ENABLE */
 #if defined(GSSAPI)
 	    if (strstr(buffer, "GSSAPI"))
@@ -302,8 +302,8 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting)
     char *challenge;
 #endif /* OPIE_ENABLE */
 #ifdef SSL_ENABLE
-    flag did_stls = FALSE;
-    flag using_tls = FALSE;
+    char *realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname;
+    flag connection_may_have_tls_errors = FALSE;
 #endif /* SSL_ENABLE */
 
 #if defined(GSSAPI)
@@ -317,7 +317,7 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting)
     has_otp = FALSE;
 #endif /* OPIE_ENABLE */
 #ifdef SSL_ENABLE
-    has_ssl = FALSE;
+    has_stls = FALSE;
 #endif /* SSL_ENABLE */
 
     /* Set this up before authentication quits early. */
@@ -440,56 +440,61 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting)
 	}
 
 #ifdef SSL_ENABLE
-	if (has_ssl
+	if (has_stls
 	    && !ctl->use_ssl
-	    && (!ctl->sslproto || !strcasecmp(ctl->sslproto,"tls1")))
+	    /* opportunistic   or    forced TLS */
+	    && (!ctl->sslproto || !strcasecmp(ctl->sslproto, "tls1")))
 	{
-	    char *realhost;
-
-	   realhost = ctl->server.via ? ctl->server.via : ctl->server.pollname;
-           ok = gen_transact(sock, "STLS");
-
-           /* We use "tls1" instead of ctl->sslproto, as we want STLS,
-            * not other SSL protocols */
-	   if (ok == PS_SUCCESS &&
-	       SSLOpen(sock,ctl->sslcert,ctl->sslkey,"tls1",ctl->sslcertck,
-		   ctl->sslcertpath,ctl->sslfingerprint,
-		   realhost,ctl->server.pollname,&ctl->remotename) == -1)
-	   {
-	       if (!ctl->sslproto && !ctl->wehaveauthed)
-	       {
-		   ctl->sslproto = xstrdup("");
-		   /* repoll immediately without TLS */
-		   return PS_REPOLL;
-	       }
-	       report(stderr,
-		       GT_("TLS connection failed.\n"));
-	       return PS_SOCKET;
-	   } else {
-	       if (outlevel >= O_VERBOSE && !ctl->sslproto)
-		   report(stdout, GT_("%s: opportunistic upgrade to TLS.\n"), realhost);
-	       using_tls = TRUE;
-	   }
-	   did_stls = TRUE;
-
-	   /*
-	    * 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."
-	    */
-	   capa_probe(sock);
-	}
-	if ((ctl->sslproto && !strcasecmp(ctl->sslproto,"tls1")) && !ctl->use_ssl && !using_tls) {
-	    report(stderr,
-		    GT_("TLS connection failed.\n"));
-	    return PS_SOCKET;
+	    /* 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);
+		}
+	    }
 	}
-
 #endif /* SSL_ENABLE */
 
 	/*
@@ -579,7 +584,7 @@ static int pop3_getauth(int sock, struct query *ctl, char *greeting)
 #ifdef SSL_ENABLE
 	/* this is for servers which claim to support TLS, but actually
 	 * don't! */
-	if (did_stls && ok == PS_SOCKET && !ctl->sslproto && !ctl->wehaveauthed)
+	if (connection_may_have_tls_errors && ok == PS_SOCKET)
 	{
 	    ctl->sslproto = xstrdup("");
 	    /* repoll immediately without TLS */