CVE-2018-14362: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: An issue was discovered in Mutt before 1.10.1 and NeoMutt before 2018-07-16. pop.c does not forbid characters that may have unsafe interaction with message-cache pathnames, as demonstrated by a '/' character.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 9.8
Published: 2018-07-17T17:29Z
Repository: https://gitlab.com/muttmua/mutt
Language: C
Files changed: pop.c
Commit date: 2018-07-13 11:16:33 -0700
Commit message: Sanitize POP bcache paths.

Protect against bcache directory path traversal for UID values.

Thanks for Jeriko One for the bug report and patch, which this commit
is based upon.

Diff: diff --git a/pop.c b/pop.c
index d9d95fbe5..288166def 100644
--- a/pop.c
+++ b/pop.c
@@ -38,10 +38,29 @@
 #ifdef USE_HCACHE
 #define HC_FNAME	"mutt"		/* filename for hcache as POP lacks paths */
 #define HC_FEXT		"hcache"	/* extension for hcache as POP lacks paths */
 #endif
 
+/**
+ * cache_id - Make a message-cache-compatible id
+ * @param id POP message id
+ * @retval ptr Sanitised string
+ *
+ * The POP message id may contain '/' and other awkward characters.
+ *
+ * @note This function returns a pointer to a static buffer.
+ */
+static const char *cache_id(const char *id)
+{
+  static char clean[SHORT_STRING];
+
+  strfcpy (clean, id, sizeof(clean));
+  mutt_sanitize_filename (clean, 1);
+
+  return clean;
+}
+
 /* write line to file */
 static int fetch_message (char *line, void *file)
 {
   FILE *f = (FILE *) file;
 
@@ -203,11 +222,11 @@ static int msg_cache_check (const char *id, body_cache_t *bcache, void *data)
       return 0;
 
   /* message not found in context -> remove it from cache
    * return the result of bcache, so we stop upon its first error
    */
-  return mutt_bcache_del (bcache, id);
+  return mutt_bcache_del (bcache, cache_id (id));
 }
 
 #ifdef USE_HCACHE
 static int pop_hcache_namer (const char *path, char *dest, size_t destlen)
 {
@@ -353,11 +372,11 @@ static int pop_fetch_headers (CONTEXT *ctx)
        *          folder and not when opening it XXX)
        * - if 'hcached' is 0, we don't have the message in our hcache:
        *        - if we also have a body: read
        *        - if we don't have a body: new
        */
-      bcached = mutt_bcache_exists (pop_data->bcache, ctx->hdrs[i]->data) == 0;
+      bcached = mutt_bcache_exists (pop_data->bcache, cache_id (ctx->hdrs[i]->data)) == 0;
       ctx->hdrs[i]->old = 0;
       ctx->hdrs[i]->read = 0;
       if (hcached)
       {
         if (bcached)
@@ -529,11 +548,11 @@ static int pop_fetch_message (CONTEXT* ctx, MESSAGE* msg, int msgno)
   POP_CACHE *cache;
   HEADER *h = ctx->hdrs[msgno];
   unsigned short bcache = 1;
 
   /* see if we already have the message in body cache */
-  if ((msg->fp = mutt_bcache_get (pop_data->bcache, h->data)))
+  if ((msg->fp = mutt_bcache_get (pop_data->bcache, cache_id (h->data))))
     return 0;
 
   /*
    * see if we already have the message in our cache in
    * case $message_cachedir is unset
@@ -576,11 +595,11 @@ static int pop_fetch_message (CONTEXT* ctx, MESSAGE* msg, int msgno)
 
     mutt_progress_init (&progressbar, _("Fetching message..."),
 			MUTT_PROGRESS_SIZE, NetInc, h->content->length + h->content->offset - 1);
 
     /* see if we can put in body cache; use our cache as fallback */
-    if (!(msg->fp = mutt_bcache_put (pop_data->bcache, h->data, 1)))
+    if (!(msg->fp = mutt_bcache_put (pop_data->bcache, cache_id (h->data), 1)))
     {
       /* no */
       bcache = 0;
       mutt_mktemp (path, sizeof (path));
       if (!(msg->fp = safe_fopen (path, "w+")))
@@ -622,11 +641,11 @@ static int pop_fetch_message (CONTEXT* ctx, MESSAGE* msg, int msgno)
 
   /* Update the header information.  Previously, we only downloaded a
    * portion of the headers, those required for the main display.
    */
   if (bcache)
-    mutt_bcache_commit (pop_data->bcache, h->data);
+    mutt_bcache_commit (pop_data->bcache, cache_id (h->data));
   else
   {
     cache->index = h->index;
     cache->path = safe_strdup (path);
   }
@@ -702,11 +721,11 @@ static int pop_sync_mailbox (CONTEXT *ctx, int *index_hint)
 	if (!ctx->quiet)
 	  mutt_progress_update (&progress, j, -1);
 	snprintf (buf, sizeof (buf), "DELE %d\r\n", ctx->hdrs[i]->refno);
 	if ((ret = pop_query (pop_data, buf, sizeof (buf))) == 0)
 	{
-	  mutt_bcache_del (pop_data->bcache, ctx->hdrs[i]->data);
+	  mutt_bcache_del (pop_data->bcache, cache_id (ctx->hdrs[i]->data));
 #if USE_HCACHE
 	  mutt_hcache_delete (hc, ctx->hdrs[i]->data, strlen);
 #endif
 	}
       }

Vulnerable code:   return mutt_bcache_del (bcache, id);
      bcached = mutt_bcache_exists (pop_data->bcache, ctx->hdrs[i]->data) == 0;
  if ((msg->fp = mutt_bcache_get (pop_data->bcache, h->data)))
    if (!(msg->fp = mutt_bcache_put (pop_data->bcache, h->data, 1)))
    mutt_bcache_commit (pop_data->bcache, h->data);
	  mutt_bcache_del (pop_data->bcache, ctx->hdrs[i]->data);
Fixed code: /**
 * cache_id - Make a message-cache-compatible id
 * @param id POP message id
 * @retval ptr Sanitised string
 *
 * The POP message id may contain '/' and other awkward characters.
 *
 * @note This function returns a pointer to a static buffer.
 */
static const char *cache_id(const char *id)
{
  static char clean[SHORT_STRING];

  strfcpy (clean, id, sizeof(clean));
  mutt_sanitize_filename (clean, 1);

  return clean;
}

  return mutt_bcache_del (bcache, cache_id (id));
      bcached = mutt_bcache_exists (pop_data->bcache, cache_id (ctx->hdrs[i]->data)) == 0;
  if ((msg->fp = mutt_bcache_get (pop_data->bcache, cache_id (h->data))))
    if (!(msg->fp = mutt_bcache_put (pop_data->bcache, cache_id (h->data), 1)))
    mutt_bcache_commit (pop_data->bcache, cache_id (h->data));
	  mutt_bcache_del (pop_data->bcache, cache_id (ctx->hdrs[i]->data));