[DebianGIS-dev] Bug#523027: incorrect upstream fix for CVE-2009-0840 (mapserver)

[Nico Golde]

Hi,
from the CVE description:
| Heap-based buffer underflow in the readPostBody function in cgiutil.c in
| mapserv in MapServer 4.x before 4.10.4 and 5.x before 5.2.2 allows remote
| attackers to have an unknown impact via a negative value in the Content-Length
| HTTP header.

The affected code is in cgiutil.c:
41 static char *readPostBody( cgiRequestObj *request ) 
42 {
43   char *data; 
44   int data_max, data_len, chunk_size;
45 
46   msIO_needBinaryStdin();
47 
48   /* -------------------------------------------------------------------- */
49   /*      If the length is provided, read in one gulp.                    */
50   /* -------------------------------------------------------------------- */
51   if( getenv("CONTENT_LENGTH") != NULL ) {
52     data_max = atoi(getenv("CONTENT_LENGTH"));
53     data = (char *) malloc(data_max+1);
54     if( data == NULL ) {
55       msIO_printf("Content-type: text/html%c%c",10,10);
56       msIO_printf("malloc() failed, Content-Length: %d unreasonably large?\n", data_max );
57       exit( 1 );
58     }
59 
60     if( (int) msIO_fread(data, 1, data_max, stdin) < data_max ) {

There is obviously a problem in case the content-length is negative.
The following is the upstream patch which was used to "fix" this issue:
 static char *readPostBody( cgiRequestObj *request ) 
 {
   char *data; 
-  int data_max, data_len, chunk_size;
+  unsigned int data_max, data_len; 
+  int chunk_size;


Unfortunately this doesn't fix the issue and I wonder why people always think
changing signed types to unsigned will fix such errors.
If I pass 0xffffffff as the content-length according to type conversion rules
in C atoi() will convert this to -1 which is again converted to 0xffff when
assigning it to an unsigned int. data_max+1 in line 53 will then overflow and
malloc is called with a parameter of 0. This causes malloc to allocated the smallest
possible chunk but it will _not_ return NULL (well, implementation defined). So it
is still possible to perform a heap-based buffer overflow after the upstream
fix.

I'm not sure if this should get a new CVE id but the versions in the CVE id
description should be adjusted and the upstream patch revised.

Cheers
Nico
P.S. @Alan, this is also the reason I have to reject your packages in our
security queue again.



-- 
Nico Golde - http://www.ngolde.de - nion at jabber.ccc.de - GPG: 0xA0A0AAAA
For security reasons, all text in this mail is double-rot13 encrypted.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-grass-devel/attachments/20090622/678f8800/attachment.pgp>