[prev in list] [next in list] [prev in thread] [next in thread] 

List:       inn-workers
Subject:    Re: potential buffer overrun in innd/art.c due to strlcpy misuse
From:       Russ Allbery <eagle () eyrie ! org>
Date:       2016-01-24 0:57:21
Message-ID: 87r3h7vlou.fsf () hope ! eyrie ! org
[Download RAW message or body]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Russ Allbery mentioned that INN uses strlcpy and strlcat, and I looked
> through the code by hand to see how well that was working out. I noticed
> that all uses ignored the returned value, except for one place in
> innd/art.c. Unfortunately that usage assumes that strlcpy returns
> strlen(dst) afterwards, but strlcpy actually returns strlen(src). This
> looks like it could lead to a buffer overrun. Also, the code does not
> appear to ensure that the result is null-terminated (this is due to its
> appending ' ' without checking whether the space fits).

> Proposed untested patch attached.

It turns out that this couldn't cause a buffer overflow because some
entirely separate part of the code initializes the buffer to the maximum
possible size that this code could assemble (there are a lot of
unfortunate patterns like that in the source code), but indeed this is a
misuse of the strlcpy function.

This whole section of code is unnecessary, since it's manipulating a
buffer, which is a dynamically-resizeable structure if you use the correct
API.  Sigh.

I think this is the correct fix, which gets rid of all use of these
functions in the FNLnames code and uses the buffer correctly.  It requires
a bit of fixing up, since other parts of the code incorrectly used the
"used" struct member instead of the "left" struct member to figure out the
length of the data in the buffer.

I don't have any good way to test this at the moment (have no test server
setup myself at present -- I keep meaning to fix that, but haven't had
much time), but I'm fairly sure that this is correct.  I'm going to go
ahead and commit it to CURRENT.  Yell at me if it breaks anything.

Thank you very much for looking at this, Paul!

Index: innd/art.c
===================================================================
--- innd/art.c	(revision 9981)
+++ innd/art.c	(working copy)
@@ -1670,7 +1670,6 @@
   SITE		*sp, *funnel;
   int		i, j, Groupcount, Followcount, Crosscount;
   char	        *p, *q, *begin, savec;
-  struct buffer	*bp;
   bool		sendit;
 
   /* Work out which sites should really get it. */
@@ -1813,13 +1812,9 @@
       funnel = &Sites[sp->Funnel];
       funnel->Sendit = true;
       if (funnel->FNLwantsnames) {
-	bp = &funnel->FNLnames;
-	p = &bp->data[bp->used];
-	if (bp->used) {
-	  *p++ = ' ';
-	  bp->used++;
-	}
-	bp->used += strlcpy(p, sp->Name, bp->size - bp->used);
+        if (funnel->FNLnames.left != 0)
+          buffer_append(&funnel->FNLnames, " ", 1);
+        buffer_append(&funnel->FNLnames, sp->Name, strlen(sp->Name));
       }
     }
   }
@@ -2181,7 +2176,7 @@
     sp->Poison = false;
     sp->Sendit = false;
     sp->Seenit = false;
-    sp->FNLnames.used = 0;
+    buffer_set(&sp->FNLnames, NULL, 0);
     sp->ng = NULL;
   }
 
Index: innd/newsfeeds.c
===================================================================
--- innd/newsfeeds.c	(revision 9981)
+++ innd/newsfeeds.c	(working copy)
@@ -842,14 +842,7 @@
 	    result = false;
 	    continue;
 	}
-	if (funnel->FNLnames.data == NULL) {
-	    funnel->FNLnames.size = length;
-	    funnel->FNLnames.data = xmalloc(length);
-	}
-	else if (funnel->FNLnames.size != length) {
-	    funnel->FNLnames.size = length;
-            funnel->FNLnames.data = xrealloc(funnel->FNLnames.data, length);
-	}
+	buffer_resize(&funnel->FNLnames, length);
 	sp->Funnel = funnel - Sites;
     }
 
Index: innd/site.c
===================================================================
--- innd/site.c	(revision 9981)
+++ innd/site.c	(working copy)
@@ -447,11 +447,11 @@
 	    buffer_append(bp, HDR(HDR__MESSAGE_ID), HDR_LEN(HDR__MESSAGE_ID));
 	    break;
 	case FEED_FNLNAMES:
-	    if (sp->FNLnames.data) {
+	    if (sp->FNLnames.left != 0) {
 		/* Funnel; write names of our sites that got it. */
 		if (Dirty)
 		    buffer_append(bp, ITEMSEP, strlen(ITEMSEP));
-		buffer_append(bp, sp->FNLnames.data, sp->FNLnames.used);
+		buffer_append(bp, sp->FNLnames.data, sp->FNLnames.left);
 	    }
 	    else {
 		/* Not funnel; write names of all sites that got it. */
@@ -516,7 +516,7 @@
     case FTprogram:
 	/* Set up the argument vector. */
 	if (sp->FNLwantsnames) {
-	    i = strlen(sp->Param) + sp->FNLnames.used;
+	    i = strlen(sp->Param) + sp->FNLnames.left;
 	    if (i + (sizeof(TOKEN) * 2) + 3 >= sizeof buff) {
 		syslog(L_ERROR, "%s toolong need %lu for %s",
 		    sp->Name, (unsigned long) (i + (sizeof(TOKEN) * 2) + 3),
@@ -523,12 +523,10 @@
 		    sp->Name);
 		break;
 	    }
-	    temp = xmalloc(i + 1);
 	    p = strchr(sp->Param, '*');
 	    *p = '\0';
-	    strlcpy(temp, sp->Param, i + 1);
-	    strlcat(temp, sp->FNLnames.data, i + 1);
-	    strlcat(temp, &p[1], i + 1);
+	    xasprintf(&temp, "%s%.*s%s", sp->Param, (int) sp->FNLnames.left,
+		      sp->FNLnames.data, &p[1]);
 	    *p = '*';
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
 	    snprintf(buff, sizeof(buff), temp, Data->TokenText);
@@ -1071,6 +1069,8 @@
 	free(sp->FNLnames.data);
 	sp->FNLnames.data = NULL;
 	sp->FNLnames.size = 0;
+	sp->FNLnames.left = 0;
+	sp->FNLnames.used = 0;
     }
     if (sp->HashFeedList) {
         for (hf = sp->HashFeedList; hf; hf = hn) {

-- 
Russ Allbery (eagle@eyrie.org)              <http://www.eyrie.org/~eagle/>

    Please send questions to the list rather than mailing me directly.
     <http://www.eyrie.org/~eagle/faqs/questions.html> explains why.
_______________________________________________
inn-workers mailing list
inn-workers@lists.isc.org
https://lists.isc.org/mailman/listinfo/inn-workers
[prev in list] [next in list] [prev in thread] [next in thread]