Re: incorrect upstream fix for CVE-2009-0840 (mapserver)

[Alan Boudreault <aboudreault () mapgears com>]

Hi

I've reported that to the devs. They should fix that as soon as possible.

ALan

On June 22, 2009 09:24:35 am Nico Golde wrote:
> 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.

-- 
Alan Boudreault
Mapgears
http://www.mapgears.com