stack smashing detected: stack overflow in nvp.c; patch included

[mcba]

There are a few places in nvp.c where the stack can overflow. This can occur when parsing emails that contain headers with "values" longer than 256 characters. I have seen some from gmail where each line of an attachment name is preceded by "=?iso-8859-1?Q?".

Here is a patch to work around the problem. It should also fix the long filename problem from a previous issue.

--- old/nvp.c   2014-02-14 15:11:59.467863930 +1100
+++ new/nvp.c   2014-02-14 15:17:09.629689807 +1100
@@ -200,18 +200,27 @@
         fprintf(stderr, "  COPY_TO_NAME\n");
 #endif
         *nn++ = *q;
+        if (nn == name + sizeof name) {
+          nn--;
+        }
         break;
       case COPY_TO_MINOR:
 #ifdef VERBOSE_TEST
         fprintf(stderr, "  COPY_TO_MINOR\n");
 #endif
         *mm++ = *q;
+        if (mm == minor + sizeof minor) {
+          mm--;
+        }
         break;
       case COPY_TO_VALUE:
 #ifdef VERBOSE_TEST
         fprintf(stderr, "  COPY_TO_VALUE\n");
 #endif
         *vv++ = *q;
+        if (vv == value + sizeof value) {
+          vv--;
+        }          
         break;
       case COPY_NOWHERE:
         break;

[Feh]

I also ran into this problem, and solved it independently before finding your patch :-)

FWIW, I think one should abort parsing instead of truncating the header in a strange way. I’m using the following two patches:

From c41afdde28fd4faedb4fb01e7b9d556dcce0e4d6 Mon Sep 17 00:00:00 2001
From: Julius Plenz <plenz@cis.fu-berlin.de>
Date: Thu, 5 Jun 2014 12:06:46 +0200
Subject: [PATCH 1/3] make_nvp: check size of destination buffer before writing
 to it

This is really very much a strcat()-with-unchecked-source-length-like
bug. Either of the writes of the kind

    *nn++ = *q;  or  *mm++ = *q;  or  *vv++ = *q;

can smash the stack if a too long header occurs.
---
 nvp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/nvp.c b/nvp.c
index 04e2087..83483a0 100644
--- a/nvp.c
+++ b/nvp.c
@@ -194,6 +194,14 @@ struct nvp *make_nvp(struct msg_src *src, char *s, const char *pfx)/*{{{*/
       return NULL;
     }

+    if(nn >= name  + sizeof(name) ||
+       mm >= minor + sizeof(minor) ||
+       vv >= value + sizeof(value)) {
+      fprintf(stderr, "Copying to stacked buffer would exceed buffer size. Parsing aborted.\n");
+      release_nvp(result);
+      return NULL;
+    }
+
     switch (nvp_copier[current_state]) {
       case COPY_TO_NAME:
 #ifdef VERBOSE_TEST

From 423ece2cf8b10fb6b3036dadfe3277e927187359 Mon Sep 17 00:00:00 2001
From: Julius Plenz <plenz@cis.fu-berlin.de>
Date: Thu, 5 Jun 2014 12:09:13 +0200
Subject: [PATCH 2/3] make_nvp: Increase sizes of stacked buffers from 256 to
 4K bytes

Stack allocs are cheap, and in 2014 header lines are sometimes longer
than just 256 characters!
---
 nvp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/nvp.c b/nvp.c
index 83483a0..46639b5 100644
--- a/nvp.c
+++ b/nvp.c
@@ -146,9 +146,9 @@ struct nvp *make_nvp(struct msg_src *src, char *s, const char *pfx)/*{{{*/
   unsigned int tok;
   char *q;
   unsigned char qq;
-  char name[256];
-  char minor[256];
-  char value[256];
+  char name[4096];
+  char minor[4096];
+  char value[4096];
   enum nvp_action last_action, current_action;
   struct nvp *result;
   size_t pfxlen;

[kinnison]

All of these cause other arbitrary issues -- pr #17 simply uses the size of the inputs to ensure things don't overflow. I didn't see this PR before.