CVE-2020-14954: Improper Neutralization of Special Elements in Output Used by a Downstream Component ('Injection') (CWE-74)
value: Mutt before 1.14.4 and NeoMutt before 2020-06-19 have a STARTTLS buffering issue that affects IMAP, SMTP, and POP3. When a server sends a "begin TLS" response, the client reads additional data (e.g., from a man-in-the-middle attacker) and evaluates it in a TLS context, aka "response injection."
CWE description: The product constructs all or part of a command, data structure, or record using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify how it is parsed or interpreted when it is sent to a downstream component.
CVSS v3 base score: 5.9
Published: 2020-06-21T17:15Z
Repository: https://github.com/neomutt/neomutt
Language: C
Files changed: conn/socket.c, conn/socket.h, imap/command.c, imap/imap.c, imap/private.h, nntp/nntp.c, pop/pop_lib.c, smtp.c
Commit date: 2020-06-19 12:58:28 +0100
Commit message: tls: clear data after a starttls acknowledgement

After a starttls acknowledgement message, clear the buffers of any
incoming data / commands.  This will ensure that all future data is
handled securely.

Co-authored-by: Pietro Cerutti <gahr@gahr.ch>

Diff: diff --git a/conn/socket.c b/conn/socket.c
index 945cc54364..2355ca60d3 100644
--- a/conn/socket.c
+++ b/conn/socket.c
@@ -299,5 +299,25 @@ struct Connection *mutt_socket_new(enum ConnectionType type)
     conn->poll = raw_socket_poll;
   }
 
   return conn;
 }
+
+/**
+ * mutt_socket_empty - Clear out any queued data
+ *
+ * The internal buffer is emptied and any data that has already arrived at this
+ * machine (in kernel buffers) is read and dropped.
+ */
+void mutt_socket_empty(struct Connection *conn)
+{
+  if (!conn)
+    return;
+
+  char buf[1024];
+  int bytes;
+
+  while ((bytes = mutt_socket_poll(conn, 0)) > 0)
+  {
+    mutt_socket_read(conn, buf, MIN(bytes, sizeof(buf)));
+  }
+}
diff --git a/conn/socket.h b/conn/socket.h
index 2e78e2d8f7..74443699e8 100644
--- a/conn/socket.h
+++ b/conn/socket.h
@@ -37,10 +37,11 @@ enum ConnectionType
   MUTT_CONNECTION_TUNNEL, ///< Tunnelled connection
   MUTT_CONNECTION_SSL,    ///< SSL/TLS-encrypted connection
 };
 
 int                mutt_socket_close   (struct Connection *conn);
+void               mutt_socket_empty   (struct Connection *conn);
 struct Connection *mutt_socket_new     (enum ConnectionType type);
 int                mutt_socket_open    (struct Connection *conn);
 int                mutt_socket_poll    (struct Connection *conn, time_t wait_secs);
 int                mutt_socket_read    (struct Connection *conn, char *buf, size_t len);
 int                mutt_socket_readchar(struct Connection *conn, char *c);
diff --git a/imap/command.c b/imap/command.c
index 40558a4f85..9cf312742f 100644
--- a/imap/command.c
+++ b/imap/command.c
@@ -1248,10 +1248,17 @@ const char *imap_cmd_trailer(struct ImapAccountData *adata)
  */
 int imap_exec(struct ImapAccountData *adata, const char *cmdstr, ImapCmdFlags flags)
 {
   int rc;
 
+  if (flags & IMAP_CMD_SINGLE)
+  {
+    // Process any existing commands
+    if (adata->nextcmd != adata->lastcmd)
+      imap_exec(adata, NULL, IMAP_CMD_POLL);
+  }
+
   rc = cmd_start(adata, cmdstr, flags);
   if (rc < 0)
   {
     cmd_handle_fatal(adata);
     return IMAP_EXEC_FATAL;
@@ -1271,10 +1278,13 @@ int imap_exec(struct ImapAccountData *adata, const char *cmdstr, ImapCmdFlags fl
   /* Allow interruptions, particularly useful if there are network problems. */
   mutt_sig_allow_interrupt(true);
   do
   {
     rc = imap_cmd_step(adata);
+    // The queue is empty, so the single command has been processed
+    if ((flags & IMAP_CMD_SINGLE) && (adata->nextcmd == adata->lastcmd))
+      break;
   } while (rc == IMAP_RES_CONTINUE);
   mutt_sig_allow_interrupt(false);
 
   if (rc == IMAP_RES_NO)
     return IMAP_EXEC_ERROR;
diff --git a/imap/imap.c b/imap/imap.c
index 71d761841f..898cf68c15 100644
--- a/imap/imap.c
+++ b/imap/imap.c
@@ -737,11 +737,14 @@ int imap_open_connection(struct ImapAccountData *adata)
       {
         goto err_close_conn;
       }
       if (ans == MUTT_YES)
       {
-        enum ImapExecResult rc = imap_exec(adata, "STARTTLS", IMAP_CMD_NO_FLAGS);
+        enum ImapExecResult rc = imap_exec(adata, "STARTTLS", IMAP_CMD_SINGLE);
+        // Clear any data after the STARTTLS acknowledgement
+        mutt_socket_empty(adata->conn);
+
         if (rc == IMAP_EXEC_FATAL)
           goto bail;
         if (rc != IMAP_EXEC_ERROR)
         {
           if (mutt_ssl_starttls(adata->conn))
diff --git a/imap/private.h b/imap/private.h
index 0ea6946809..df84615ab6 100644
--- a/imap/private.h
+++ b/imap/private.h
@@ -71,10 +71,11 @@ typedef uint8_t ImapOpenFlags;         ///< Flags, e.g. #MUTT_THREAD_COLLAPSE
 typedef uint8_t ImapCmdFlags;          ///< Flags for imap_exec(), e.g. #IMAP_CMD_PASS
 #define IMAP_CMD_NO_FLAGS          0   ///< No flags are set
 #define IMAP_CMD_PASS        (1 << 0)  ///< Command contains a password. Suppress logging
 #define IMAP_CMD_QUEUE       (1 << 1)  ///< Queue a command, do not execute
 #define IMAP_CMD_POLL        (1 << 2)  ///< Poll the tcp connection before running the imap command
+#define IMAP_CMD_SINGLE      (1 << 3)  ///< Run a single command
 
 /**
  * enum ImapExecResult - imap_exec return code
  */
 enum ImapExecResult
diff --git a/nntp/nntp.c b/nntp/nntp.c
index d10cbe907e..207f6b7a5a 100644
--- a/nntp/nntp.c
+++ b/nntp/nntp.c
@@ -1897,10 +1897,12 @@ int nntp_open_connection(struct NntpAccountData *adata)
       if ((mutt_socket_send(conn, "STARTTLS\r\n") < 0) ||
           (mutt_socket_readln(buf, sizeof(buf), conn) < 0))
       {
         return nntp_connect_error(adata);
       }
+      // Clear any data after the STARTTLS acknowledgement
+      mutt_socket_empty(conn);
       if (!mutt_str_startswith(buf, "382", CASE_MATCH))
       {
         adata->use_tls = 0;
         mutt_error("STARTTLS: %s", buf);
       }
diff --git a/pop/pop_lib.c b/pop/pop_lib.c
index 674c57cdf0..920926f2c0 100644
--- a/pop/pop_lib.c
+++ b/pop/pop_lib.c
@@ -356,10 +356,12 @@ int pop_open_connection(struct PopAccountData *adata)
     }
     if (adata->use_stls == 2)
     {
       mutt_str_strfcpy(buf, "STLS\r\n", sizeof(buf));
       rc = pop_query(adata, buf, sizeof(buf));
+      // Clear any data after the STLS acknowledgement
+      mutt_socket_empty(adata->conn);
       if (rc == -1)
         goto err_conn;
       if (rc != 0)
       {
         mutt_error("%s", adata->err_msg);
diff --git a/smtp.c b/smtp.c
index 498a2a3fa3..67faa75910 100644
--- a/smtp.c
+++ b/smtp.c
@@ -717,10 +717,12 @@ static int smtp_open(struct Connection *conn, bool esmtp)
   if (ans == MUTT_YES)
   {
     if (mutt_socket_send(conn, "STARTTLS\r\n") < 0)
       return SMTP_ERR_WRITE;
     rc = smtp_get_resp(conn);
+    // Clear any data after the STARTTLS acknowledgement
+    mutt_socket_empty(conn);
     if (rc != 0)
       return rc;
 
     if (mutt_ssl_starttls(conn))
     {

Vulnerable code:         enum ImapExecResult rc = imap_exec(adata, "STARTTLS", IMAP_CMD_NO_FLAGS);
Fixed code: 
/**
 * mutt_socket_empty - Clear out any queued data
 *
 * The internal buffer is emptied and any data that has already arrived at this
 * machine (in kernel buffers) is read and dropped.
 */
void mutt_socket_empty(struct Connection *conn)
{
  if (!conn)
    return;

  char buf[1024];
  int bytes;

  while ((bytes = mutt_socket_poll(conn, 0)) > 0)
  {
    mutt_socket_read(conn, buf, MIN(bytes, sizeof(buf)));
  }
}
void               mutt_socket_empty   (struct Connection *conn);
  if (flags & IMAP_CMD_SINGLE)
  {
    // Process any existing commands
    if (adata->nextcmd != adata->lastcmd)
      imap_exec(adata, NULL, IMAP_CMD_POLL);
  }

    // The queue is empty, so the single command has been processed
    if ((flags & IMAP_CMD_SINGLE) && (adata->nextcmd == adata->lastcmd))
      break;
        enum ImapExecResult rc = imap_exec(adata, "STARTTLS", IMAP_CMD_SINGLE);
        // Clear any data after the STARTTLS acknowledgement
        mutt_socket_empty(adata->conn);

#define IMAP_CMD_SINGLE      (1 << 3)  ///< Run a single command
      // Clear any data after the STARTTLS acknowledgement
      mutt_socket_empty(conn);
      // Clear any data after the STLS acknowledgement
      mutt_socket_empty(adata->conn);
    // Clear any data after the STARTTLS acknowledgement
    mutt_socket_empty(conn);