Bug#533285: inn2: nnrpd crashes when retrieving entire thread

Russ Allbery rra at debian.org
Tue Jun 16 19:47:21 UTC 2009


Julien ÉLIE <julien at trigofacile.com> writes:

>> --- inn2-2.4.5.orig/storage/overdata.c  2008-06-29 18:56:57.000000000 +0100
>> +++ inn2-2.4.5/storage/overdata.c       2009-04-01 14:56:41.000000000 +0100
>> @@ -6,6 +6,8 @@
>> **  tab-separated list of overview fields.
>> */
>>
>> +#include <syslog.h>
>> +
>> #include "config.h"
>> #include "clibrary.h"
>> #include <ctype.h>
>> @@ -407,9 +409,26 @@
>>        p = vector->strings[element] +
>>            strlen(extra->strings[element - ARRAY_SIZE(fields)]) + 2;
>>        len = vector->strings[element + 1] - p - 1;
>> +        if(p>=vector->strings[element + 1])
>> +          len=0;
>>     field = xstrndup(p, len);
>>     return field;

This is not a correct fix.  It might be worthwhile to do a sanity check
against strlen of the field like:

--- overdata.c  (revision 8507)
+++ overdata.c  (working copy)
@@ -380,8 +380,11 @@
      * of the caller... */
     if (element >= ARRAY_SIZE(fields)) {
        /* +2 for `: ' */
-       p = vector->strings[element] +
-           strlen(extra->strings[element - ARRAY_SIZE(fields)]) + 2;
+        size_t hlen = strlen(extra->strings[element - ARRAY_SIZE(fields)]) + 2;
+
+        if (hlen >= vector->strings[element + 1] - vector->strings[element])
+            return NULL;
+       p = vector->strings[element] + hlen;
        len = vector->strings[element + 1] - p - 1;
     } else {
        p = vector->strings[element];

but this doesn't fix the underlying problem.  The real problem is that
the code is assuming that one can retrieve extended overview fields by
index.  This does not work unless one rebuilds all of overview after
adding a new field and the header is present in all articles.

Suppose that the extended field for that article was some other header
that happened to be long enough.  This code would blissfully return a
random portion of some other header in the overview information.

Retrieval of extended overview fields can't be done by index; it has to
be done by walking the extended overview fields and doing string
comparisons against the desired header name.  The interface to
overview_getheader() is therefore wrong, since it takes only an index.
The API needs to change so that it takes the name of the desired header
field instead and searches the extended header fields for the correct
field.

Once it's rewritten to do that, the above sanity check wouldn't be
necessary, so there's probably no point in making the partial solution.

Looks like I missed this use case when putting together the new overview
API.

> However, there is a more fundamental bug in xstrndup() because it
> does not check the validity of the size.

xstrndup() got a perfectly valid size.  It's a very large size, but
there wasn't anything invalid about it.  The API takes a size_t, which
is unsigned.

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



More information about the inn-workers mailing list