File c547433cdf2e79191b15c6932c57f1472bfb5ff4.dif of Package mutt.23793

Overview Repositories Revisions Requests Users Attributes Meta

File c547433cdf2e79191b15c6932c57f1472bfb5ff4.dif of Package mutt.23793

From c547433cdf2e79191b15c6932c57f1472bfb5ff4 Mon Sep 17 00:00:00 2001
From: Kevin McCarthy <kevin@8t8.us>
Date: Tue, 16 Jun 2020 13:49:20 -0700
Subject: [PATCH] Fix STARTTLS response injection attack.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Thanks again to Damian Poddebniak and Fabian Ising from the Münster
University of Applied Sciences for reporting this issue.  Their
summary in ticket 248 states the issue clearly:

We found another STARTTLS-related issue in Mutt. Unfortunately, it
  affects SMTP, POP3 and IMAP.

When the server responds with its "let's do TLS now message", e.g. A
  OK begin TLS\r\n in IMAP or +OK begin TLS\r\n in POP3, Mutt will
  also read any data after the \r\n and save it into some internal
  buffer for later processing. This is problematic, because a MITM
  attacker can inject arbitrary responses.

There is a nice blogpost by Wietse Venema about a "command
  injection" in postfix (http://www.postfix.org/CVE-2011-0411.html).
  What we have here is the problem in reverse, i.e. not a command
  injection, but a "response injection."

This commit fixes the issue by clearing the CONNECTION input buffer in
mutt_ssl_starttls().

To make backporting this fix easier, the new functions only clear the
top-level CONNECTION buffer; they don't handle nested buffering in
mutt_zstrm.c or mutt_sasl.c.  However both of those wrap the
connection *after* STARTTLS, so this is currently okay.  mutt_tunnel.c
occurs before connecting, but it does not perform any nesting.
---
 mutt_socket.c     |   30 ++++++++++++++++++++++++++++++
 mutt_socket.h     |    2 ++
 mutt_ssl.c        |   12 ++++++++++++
 mutt_ssl_gnutls.c |   12 ++++++++++++
 4 files changed, 56 insertions(+)

--- mutt_socket.c
+++ mutt_socket.c	2020-06-24 09:13:59.014091891 +0000
@@ -150,6 +150,36 @@ int mutt_socket_write_d (CONNECTION *con
   return sent;
 }
 
+/* Checks if the CONNECTION input buffer has unread data.
+ *
+ * NOTE: for general use, the function needs to expand to poll nested
+ * connections.  It currently does not to make backporting a security
+ * fix easier.
+ *
+ * STARTTLS occurs before SASL and COMPRESS=DEFLATE processing, and
+ * mutt_tunnel() does not wrap the connection.  So this and the next
+ * function are safe for current usage in mutt_ssl_starttls().
+ */
+int mutt_socket_has_buffered_input (CONNECTION *conn)
+{
+  return conn->bufpos < conn->available;
+}
+
+/* Clears buffered input from a connection.
+ *
+ * NOTE: for general use, the function needs to expand to call nested
+ * connections.  It currently does not to make backporting a security
+ * fix easier.
+ *
+ * STARTTLS occurs before SASL and COMPRESS=DEFLATE processing, and
+ * mutt_tunnel() does not wrap the connection.  So this and the previous
+ * function are safe for current usage in mutt_ssl_starttls().
+ */
+void mutt_socket_clear_buffered_input (CONNECTION *conn)
+{
+  conn->bufpos = conn->available = 0;
+}
+
 /* poll whether reads would block.
  *   Returns: >0 if there is data to read,
  *            0 if a read would block,
--- mutt_socket.h
+++ mutt_socket.h	2020-06-24 09:16:11.375633020 +0000
@@ -53,6 +53,8 @@ typedef struct _connection
 
 int mutt_socket_open (CONNECTION* conn);
 int mutt_socket_close (CONNECTION* conn);
+int mutt_socket_has_buffered_input (CONNECTION *conn);
+void mutt_socket_clear_buffered_input (CONNECTION *conn);
 int mutt_socket_read (CONNECTION* conn, char* buf, size_t len);
 int mutt_socket_poll (CONNECTION* conn, time_t wait_secs);
 int mutt_socket_readchar (CONNECTION *conn, char *c);
--- mutt_ssl.c
+++ mutt_ssl.c	2020-06-24 09:13:59.014091891 +0000
@@ -180,6 +180,18 @@ int mutt_ssl_starttls (CONNECTION* conn)
   int maxbits;
   long ssl_options = 0;
 
+  if (mutt_socket_has_buffered_input (conn))
+  {
+    /* L10N:
+       The server is not supposed to send data immediately after
+       confirming STARTTLS.  This warns the user that something
+       weird is going on.
+    */
+    mutt_error _("Warning: clearing unexpected buffered data before STARTTLS");
+    mutt_sleep (0);
+    mutt_socket_clear_buffered_input (conn);
+  }
+
   if (ssl_init())
     goto bail;
 
--- mutt_ssl_gnutls.c
+++ mutt_ssl_gnutls.c	2020-06-24 09:13:59.014091891 +0000
@@ -218,6 +218,18 @@ static int tls_socket_open (CONNECTION*
 
 int mutt_ssl_starttls (CONNECTION* conn)
 {
+  if (mutt_socket_has_buffered_input (conn))
+  {
+    /* L10N:
+       The server is not supposed to send data immediately after
+       confirming STARTTLS.  This warns the user that something
+       weird is going on.
+    */
+    mutt_error _("Warning: clearing unexpected buffered data before STARTTLS");
+    mutt_sleep (0);
+    mutt_socket_clear_buffered_input (conn);
+  }
+
   if (tls_init() < 0)
     return -1;

Places

File c547433cdf2e79191b15c6932c57f1472bfb5ff4.dif of Package mutt.23793

Places