Raster tile filename handling could produce buffer overflow with long (> MS_MAXPATHLEN)

[sdlime]

Reported by Even Rouault...

• line 1402 : "strcpy( tilename, tshp.values[tileitemindex] )" --> gdaltindex and the version of shapelib in GDAL currently limit DBF

string fields to 255 characters. So strlen(tshp.values[tileitemindex]) < MS_MAXPATHLEN : OK... But a modified version of gdalttindex.c and dbfopen.c in GDAL can easily produce field sizes up to 65535 characters (or 32767 if it is a signed value, not sure), what mapserver can use. I managed to cause a buffer overflow this way by creating a 2000 character long string.

• line 1404 : "sprintf(tilename, "%s/%s", tshp.values[tileitemindex],

layer->data)"

--> nothing checks that strlen(tshp.values[tileitemindex]) +

strlen(layer->data) + 1 < MS_MAXPATHLEN. I actually managed to cause a stack overflow with a long string in the DATA keyword and a sufficient long string as a name of my tileindex. However, I'm not sure at all what the use case for this line is : why do we need to append the content of the DATA as a subdirectory of a filename referenced by the tileindex ??

• line 1450 and 1452 : "strcpy( szPath, filename )". --> I'm not sure if filename is always smaller than MS_MAXPATHLEN . It is

set at line 1405 as "filename = tilename", so this is OK. But at line 1409, it is set as "filename = layer->data". My understanding is that strlen(layer->data) < MS_MAXPATHLEN, but I didn't dig enough to be sure that.

[sdlime]

Seems like simply switching to strncpy() would fix line 1402, e.g.:

(void)strncpy(tilename, tshp.values[tileitemindex], sizeof(tilename) - 1);
tilename[sizeof(tilename) - 1] = '\0';

and using snprint() would fix line 1404, e.g.:

snprintf(tilename, sizeof(tilename), "%s/%s", tshp.values[tileitemindex], layer->data);

Not sure about lines 1450/1452.

Steve

[sdlime]

Seems also that strlcpy() is a better alternative to strncpy()- safer. But is it portable????

Steve

[dmorissette]

I just did a quick check in GDAL and there is absolutely no use of strlcpy that I could find... just strncpy... so I'd probably avoid it in 5.4.1... but we could try it in trunk and see.

[rouault]

strlcpy/strlcat are functions that come from BSD systems, and are not available in GNU libc. strlcat has been "imported" in MapServer a long time ago and is defined in mapstring.c only if it is not detected by the configure script (NEED_STRLCAT macro). The same trick could be done for strlcpy (if you have one, you have the other one)

As far as GDAL is concerned, I mentionned in my email that I introduced renamed versions of strlcpy/strlcat (CPLStrlcpy/CPLStrlcat) in trunk (1.7.0dev). I have used them extensively in port/cpl_path.cpp instead of strncpy/strncat previously.

[sdlime]

I applied the two changes in the first comment to mapraster.c in r9043 in the 5.4 branch. We can add strlcpy() in trunk and apply it more broadly. Lines 1450/1452 I believe are ok "as is" now that we are limiting the contents of filename properly.

Moving this ticket to the 6.0 milestone now that 5.4 is patched.

Steve