CVE-2018-14363: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: An issue was discovered in NeoMutt before 2018-07-16. newsrc.c does not properly restrict '/' characters that may have unsafe interaction with cache pathnames.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 7.5
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));