Buffer overflow in msOWSParseRequestMetadata

[constantinius]

Since version 6 of Mapserver a bug exists in msOWSParseRequestMetadata, resulting in a buffer overflow/stack smashing on 32 bit machines (the error may also be applicable for other architectures, but did not yet show).

The cause is the char requestBuffer[32]; where data is written over the boundaries of the array in some cases.

Example MapFile: http://pastebin.com/pTvyS4q6
Result: http://pastebin.com/Xp15Pkwd

[sdlime]

Need to confirm affected versions and get a fix together...

CC'ing @aboudreault @dmorissette @assefay

[dmorissette]

I had a quick look at the code and your test case (without actually running it because I'm not setup at the moment)

Please note that the value of "wms_enable_request" should be space-delimited and not comma-delimited, so the quick fix for you would be to use:

"wms_enable_request"      "getcapabilities getmap getfeatureinfo"

A short term fix for the buffer overflow in 6.0.x could be to allocate requestBuffer to be of size strlen(metadata)+1 at the beginning of msOWSParseRequestMetadata() (and make sure it is freed before returning).

For the longer term 6.2 fix, this part of the code could probably be simplified with some refactoring. @tbonfort had some suggestions to that effect on the mapserver-dev list.

[tbonfort]

No need for rushing out a 6.0.4 then, right ? I'm not milestoning a rewrite for 6.2 though as we are too far into the release process for this kind of change. A simple fix for 6.2 could be to allow a comma as a separator.

[tbonfort]

This is my proposed change for msOWSParseRequestMetadata:

/* msOWSParseRequestMetadata 
 *  
 * This function parse a enable_request metadata string and check if the
 * given request is present and enabled.
 * 
 * returns:
 *  - MS_TRUE if the request was explicitely enabled
 *  - MS_FALSE if the request was explicitely disabled
 *  - MS_UNKNOWN if there was no entry for this request
 */
int msOWSParseRequestMetadata(const char *metadata, const char *request)
{
  int authorize = MS_UNKNOWN;
  char *ptr = NULL;

  ptr = strchr(metadata,'*');
  if(ptr) {
    if(ptr == metadata || *(ptr-1)!='!') {
      authorize = MS_TRUE;
    } else {
      /* found a '!*' */
      authorize = MS_FALSE;
    }
  }

  ptr = strcasestr(metadata,request);
  if(ptr) {
    if(ptr == metadata || *(ptr-1)!='!') {
      authorize = MS_TRUE;
    } else {
      /* found a '!request' */
      authorize = MS_FALSE;
    }
  }
  return authorize;
}

[constantinius]

@dmorissette: The problem also persists when I use

"wms_enable_request"      "getcapabilities getmap getfeatureinfo"

So this is not a fix!

@tbonfort: Your solution looks simple and nice and is easily understandable in case of future edits.

[tbonfort]

I have tried with

"wms_enable_request"      "getcapabilities getmap getfeatureinfo"

and get neither a segfault nor any related valgrind errors.

[constantinius]

As I've said, the problem did only occur in special setups (like the LiveDVD). On my local machine I don't have problems either.

[tbonfort]

@constantinius can you post the valgrind output of the crash please ?

[constantinius]

@tbonfort: http://pastebin.com/r7Qc5Qqs

Not much additional info I fear.

[constantinius]

@tbonfort: I only set the "wms_enable_request" "getcapabilities getmap getfeatureinfo" in the map.web.metadata not in the layer one.

Setting the field in the layer to that value actually solved the issue. But the function should still be redone, IMHO.

[sdlime]

I agree on not rushing a 6.0.4. The overflow cannot be triggered externally, only though direct mapfile editing. -Steve

[dmorissette]

Agreed with no need for 6.0.4. And I like @tbonfort's proposed new version of msOWSParseRequestMetadata().

[tbonfort]

pushing this to 6.4

[tbonfort]

demilestoning until someone feels the need to fix this

[jratike80]

The place to insert the patch is perhaps https://github.com/mapserver/mapserver/blob/branch-7-0/mapows.c#L743. I am not sure if this part of code is still the same, the history is https://github.com/mapserver/mapserver/commits/branch-7-0/mapows.c