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://github.com/neomutt/neomutt
Language: C
Files changed: newsrc.c, pop.c
Commit date: 2018-07-05 15:59:55 +0100
Commit message: sanitise cache paths

Co-authored-by: JerikoOne <jeriko.one@gmx.us>

Diff: diff --git a/newsrc.c b/newsrc.c
index a0f2c39de7..c9dfdba23c 100644
--- a/newsrc.c
+++ b/newsrc.c
@@ -713,11 +713,22 @@ int nntp_active_save_cache(struct NntpServer *nserv)
  *
  * Used by mutt_hcache_open() to compose hcache file name
  */
 static int nntp_hcache_namer(const char *path, char *dest, size_t destlen)
 {
-  return snprintf(dest, destlen, "%s.hcache", path);
+  int count = snprintf(dest, destlen, "%s.hcache", path);
+
+  /* Strip out any directories in the path */
+  char *first = strchr(dest, '/');
+  char *last = strrchr(dest, '/');
+  if (first && last && (last > first))
+  {
+    memmove(first, last, strlen(last) + 1);
+    count -= (last - first);
+  }
+
+  return count;
 }
 
 /**
  * nntp_hcache_open - Open newsgroup hcache
  * @param nntp_data NNTP data
diff --git a/pop.c b/pop.c
index 1da2972520..4f3a592a8d 100644
--- a/pop.c
+++ b/pop.c
@@ -61,10 +61,27 @@
 #ifdef USE_HCACHE
 #define HC_FNAME "neomutt" /* 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];
+  mutt_str_strfcpy(clean, id, sizeof(clean));
+  mutt_file_sanitize_filename(clean, true);
+  return clean;
+}
+
 /**
  * fetch_message - write line to file
  * @param line String to write
  * @param file FILE pointer to write to
  * @retval  0 Success
@@ -240,11 +257,11 @@ static int msg_cache_check(const char *id, struct BodyCache *bcache, void *data)
   }
 
   /* 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
 /**
  * pop_hcache_namer - Create a header cache filename for a POP mailbox
@@ -405,11 +422,11 @@ static int pop_fetch_headers(struct Context *ctx)
        * - if 'hcached' is false, we don't have the message in our hcache:
        *        - if we also have a body: read
        *        - if we don't have a body: new
        */
       const bool bcached =
-          (mutt_bcache_exists(pop_data->bcache, ctx->hdrs[i]->data) == 0);
+          (mutt_bcache_exists(pop_data->bcache, cache_id(ctx->hdrs[i]->data)) == 0);
       ctx->hdrs[i]->old = false;
       ctx->hdrs[i]->read = false;
       if (hcached)
       {
         if (bcached)
@@ -595,11 +612,11 @@ static int pop_fetch_message(struct Context *ctx, struct Message *msg, int msgno
   struct PopCache *cache = NULL;
   struct Header *h = ctx->hdrs[msgno];
   unsigned short bcache = 1;
 
   /* see if we already have the message in body cache */
-  msg->fp = mutt_bcache_get(pop_data->bcache, h->data);
+  msg->fp = mutt_bcache_get(pop_data->bcache, cache_id(h->data));
   if (msg->fp)
     return 0;
 
   /*
    * see if we already have the message in our cache in
@@ -642,11 +659,11 @@ static int pop_fetch_message(struct Context *ctx, struct 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 */
-    msg->fp = mutt_bcache_put(pop_data->bcache, h->data);
+    msg->fp = mutt_bcache_put(pop_data->bcache, cache_id(h->data));
     if (!msg->fp)
     {
       /* no */
       bcache = 0;
       mutt_mktemp(path, sizeof(path));
@@ -687,11 +704,11 @@ static int pop_fetch_message(struct Context *ctx, struct 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 = mutt_str_strdup(path);
   }
@@ -781,11 +798,11 @@ static int pop_sync_mailbox(struct Context *ctx, int *index_hint)
           mutt_progress_update(&progress, j, -1);
         snprintf(buf, sizeof(buf), "DELE %d\r\n", ctx->hdrs[i]->refno);
         ret = pop_query(pop_data, buf, sizeof(buf));
         if (ret == 0)
         {
-          mutt_bcache_del(pop_data->bcache, ctx->hdrs[i]->data);
+          mutt_bcache_del(pop_data->bcache, cache_id(ctx->hdrs[i]->data));
 #ifdef USE_HCACHE
           mutt_hcache_delete(hc, ctx->hdrs[i]->data, strlen(ctx->hdrs[i]->data));
 #endif
         }
       }

Vulnerable code:   return snprintf(dest, destlen, "%s.hcache", path);
  return mutt_bcache_del(bcache, id);
          (mutt_bcache_exists(pop_data->bcache, ctx->hdrs[i]->data) == 0);
  msg->fp = mutt_bcache_get(pop_data->bcache, h->data);
    msg->fp = mutt_bcache_put(pop_data->bcache, h->data);
    mutt_bcache_commit(pop_data->bcache, h->data);
          mutt_bcache_del(pop_data->bcache, ctx->hdrs[i]->data);
Fixed code:   int count = snprintf(dest, destlen, "%s.hcache", path);

  /* Strip out any directories in the path */
  char *first = strchr(dest, '/');
  char *last = strrchr(dest, '/');
  if (first && last && (last > first))
  {
    memmove(first, last, strlen(last) + 1);
    count -= (last - first);
  }

  return count;
/**
 * 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];
  mutt_str_strfcpy(clean, id, sizeof(clean));
  mutt_file_sanitize_filename(clean, true);
  return clean;
}

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