CVE-2020-28896: Improper Handling of Exceptional Conditions (CWE-755)
value: Mutt before 2.0.2 and NeoMutt before 2020-11-20 did not ensure that $ssl_force_tls was processed if an IMAP server's initial server response was invalid. The connection was not properly closed, and the code could continue attempting to authenticate. This could result in authentication credentials being exposed on an unencrypted connection, or to a machine-in-the-middle.
CWE description: The product does not handle or incorrectly handles an exceptional condition.
CVSS v3 base score: 5.3
Published: 2020-11-23T19:15Z
Repository: https://github.com/neomutt/neomutt
Language: C
Files changed: imap/imap.c
Commit date: 2020-11-17 12:29:29 +0000
Commit message: imap: close connection on all failures

Thanks to Gabriel Salles-Loustau for spotting the problem.

Co-authored-by: Kevin McCarthy <kevin@8t8.us>

Diff: diff --git a/imap/imap.c b/imap/imap.c
index a1465c4390..097e883c13 100644
--- a/imap/imap.c
+++ b/imap/imap.c
@@ -759,11 +759,11 @@ int imap_open_connection(struct ImapAccountData *adata)
       if (C_SslForceTls)
         ans = MUTT_YES;
       else if ((ans = query_quadoption(C_SslStarttls,
                                        _("Secure connection with TLS?"))) == MUTT_ABORT)
       {
-        goto err_close_conn;
+        goto bail;
       }
       if (ans == MUTT_YES)
       {
         enum ImapExecResult rc = imap_exec(adata, "STARTTLS", IMAP_CMD_SINGLE);
         // Clear any data after the STARTTLS acknowledgement
@@ -774,11 +774,11 @@ int imap_open_connection(struct ImapAccountData *adata)
         if (rc != IMAP_EXEC_ERROR)
         {
           if (mutt_ssl_starttls(adata->conn))
           {
             mutt_error(_("Could not negotiate TLS connection"));
-            goto err_close_conn;
+            goto bail;
           }
           else
           {
             /* RFC2595 demands we recheck CAPABILITY after TLS completes. */
             if (imap_exec(adata, "CAPABILITY", IMAP_CMD_NO_FLAGS))
@@ -789,11 +789,11 @@ int imap_open_connection(struct ImapAccountData *adata)
     }
 
     if (C_SslForceTls && (adata->conn->ssf == 0))
     {
       mutt_error(_("Encrypted connection unavailable"));
-      goto err_close_conn;
+      goto bail;
     }
 #endif
   }
   else if (mutt_istr_startswith(adata->buf, "* PREAUTH"))
   {
@@ -805,11 +805,11 @@ int imap_open_connection(struct ImapAccountData *adata)
      * decide whether to abort. Note that if using $tunnel and
      * $tunnel_is_secure, adata->conn->ssf will be set to 1. */
     if ((adata->conn->ssf == 0) && C_SslForceTls)
     {
       mutt_error(_("Encrypted connection unavailable"));
-      goto err_close_conn;
+      goto bail;
     }
 #endif
 
     adata->state = IMAP_AUTHENTICATED;
     if (check_capabilities(adata) != 0)
@@ -822,15 +822,12 @@ int imap_open_connection(struct ImapAccountData *adata)
     goto bail;
   }
 
   return 0;
 
-#ifdef USE_SSL
-err_close_conn:
-  imap_close_connection(adata);
-#endif
 bail:
+  imap_close_connection(adata);
   FREE(&adata->capstr);
   return -1;
 }
 
 /**

Vulnerable code:         goto err_close_conn;
            goto err_close_conn;
      goto err_close_conn;
      goto err_close_conn;
#ifdef USE_SSL
err_close_conn:
  imap_close_connection(adata);
#endif
Fixed code:         goto bail;
            goto bail;
      goto bail;
      goto bail;
  imap_close_connection(adata);