Issues with static buffers and sprintf

[aboudreault]

This ticket is to document the work to be done to eliminate static buffers and the use of sprintf in the code. There are many unsafe practices and buffer overflow vulnerabilities.

I'm going to make a summary of the major changes needed and will certainly ask the file author (or any devs) to review the patch to avoid to break anything.

[warmerdam]

I'd like to see some of the examples of problems if it is convenient, or the patch if there is one.

[aboudreault]

In example, you can take a look in mapagg.cpp, line 2347: the function msSaveImageAGG(). And here's a patch for it:

diff -r 1d3a38815c62 mapagg.cpp
--- a/mapagg.cpp        Wed Sep 08 11:31:49 2010 -0400
+++ b/mapagg.cpp        Wed Sep 08 15:10:14 2010 -0400
@@ -2347,18 +2347,20 @@
 int msSaveImageAGG(imageObj* image, char *filename, outputFormatObj *format)
 {
     char *pFormatBuffer;
-    char cGDFormat[128];
     int   iReturn = 0;
+    size_t n = 0;
+
     msAlphaAGG2GD(image);
     pFormatBuffer = format->driver;
 
-    strcpy(cGDFormat, "gd/");
-    strcat(cGDFormat, &(format->driver[4]));
-
-    format->driver = &cGDFormat[0];
+    n = strlen(&(pFormatBuffer[4]));
+    format->driver = (char*) malloc(sizeof(char) * (n+4));
+    strcpy(format->driver, "gd/");
+    strncat(format->driver, &(pFormatBuffer[4]), n);
 
     iReturn = msSaveImageGD(image, filename, format);
 
+    free(format->driver);
     format->driver = pFormatBuffer;
 
     return iReturn;

Normally, it is as simpler as this.... but sometime not. In example: the function LutFromGimpLine in mapdrawgdal.c. In that case it's not straightforward to determine what the max size of the buffer is. I would be interested to get your opinion on that Frank.

[aboudreault]

A more evident issue can be seen in maprasterquery.c, line 1372. There are plenty of sprintf... + a 33 lines later (l.1404), a 'sprintf( szWork, "%.999s", layer->class[p_class]->name );'

[pramsey]

Would using snprintf, strncat, strncpy, etc, be simpler? If the goal is just to prevent malicious overflows, that is, rather than try to build robustness to actually handle very long inputs.

[aboudreault]

That could be a solution... but this could result in a truncated strings. Not sure if it's not better to do everything at the same time rather than getting undefined behavior in the future. I agree that it's sometime tough to get that robustness though.

[aboudreault]

I've attached the 2 patches. As suggested by pramsey, almost all changes have been to modify the code source to use the safer functions.

[rouault]

The mappostgis.patch has incorrect parts.

The original code :

for ( t = 0; t < layer->numitems; t++ ) { 
  strcat(strItems, "\""); 
  strcat(strItems, layer->items[t]); 
  strcat(strItems, "\","); 
}

is not equivalent to :

for ( t = 0; t < layer->numitems; t++ ) { 
  snprintf(strItems, length, "\"%s\",", layer->items[t]); 
}

There is at least another loop later in the patch where there's likely a similar error.

Personnally, and according to the OpenBSD guys, strncat is difficult to use correctly. I'd suggest rather to use strlcat (that is defined in mapstring.c for non BSD systems) which is much less error-prone to use. The difference is that with strlcat() you pass the size of the destination buffer and don't have to bother with how many bytes there are still available as for strncat().

See ​http://www.openbsd.org/cgi-bin/man.cgi?query=strlcat for more details and ​http://www.usenix.org/events/usenix99/full_papers/millert/millert_html/index.html for the paper that advertizes strlcat() over strncat(). Or ​http://trac.osgeo.org/gdal/changeset/20442/trunk/gdal/ogr/ogr_srs_proj4.cpp for an example of how I've used it in GDAL (CPLStrlcat() is just strlcat() redefined in gdal/port to avoid portability issues).

Or, perhaps politically incorrect, as I suggested once in IRC, perhaps a shy switch to C++ to use std::string facilities would help solving most of those issues : no more buffer overflow risk, no more memory leaks, ... The only issue is with out of memory conditions that can be either ignored and cause an abort() of the process, or could be caught by std::bad_alloc at a higher level in mapserv to return a proper error.

[aboudreault]

rouault, I've modified the incorrect part you mentioned, but I do not see the other loop with the similar error.

strlcat seems interesting. Didn't know it was already defined in mapstring.c. Well, it is certainly more elegant rather than passing the correct computed size parameter as we currently do. I will pass through the patches and do a quick replace.

thanks for your suggestion, appreciated.

[rouault]

Actually, the other loop is that one :

2502 	                        strcat(buffer, "("); 
2503	 	                   
2504	 	                strcat(buffer, "date_trunc('"); 
2505	 	                strcat(buffer, timeresolution); 
2506	 	                strcat(buffer, "', ");         
2507	 	                strcat(buffer, timefield); 
2508	 	                strcat(buffer, ")");    
2509	 	 
2510	 	                strcat(buffer, " = "); 
2511	 	                   
2512	 	                strcat(buffer,  "'"); 
2513	 	 
2514	 	                strcat(buffer, atimes[i]);

replaced by :

 	2469	                snprintf(buffer, buffer_size, "%s(date_trunc('%s', %s) = '%s", 
 	2470	                         buffer, timeresolution, timefield, atimes[i]);

I read too fast. At first sight the snprintf might be OK (I missed that it reused previous buffer value as its 1st argument), but I'm pretty sure this is risky. I remember having fixed the OGDI library that had a somewhat similar pattern. Basically I would recommand against using the destination buffer of snprintf as one of its input arguments. Perhaps if it is the first argument, it is OK, but seems risky.

[warmerdam]

I concur that this is a risky approach - it might well work with some runtimes and not others.

[aboudreault]

Updated patch

[aboudreault]

I've updated the patch of the ticket with the recommendations. It now uses strlcat and strlcpy. The risky approach with snprintf has also been replaced. This one should build properly on windows since I've fixed the array size from a variable problem. assefa, could you confirm this?

[assefa]

sorry for being late on this. It builds properly on windows, except for a couple of small issues:

• mapfile.c (in msGetExpressionString): variable buffer_size not declared at the begining og a code block.

• adding -DNEED_STRLCPY in nmkae.opt

I can fix these once committed or you can patch and commit.

Thanks

[aboudreault]

Thanks assefa. Devs, what about committing this patch?

[assefa]

nmake patch

[assefa]

Alan, I have attached patch for nmake.opt.

[sdlime]

Alan: I'd take this to the -dev list for a vote. I'm not sure how many folks are watching closely...

Steve

[dmorissette]

Alan had already written to the -dev list yesterday (in response to the ticket 3537 thread) explaining that if there are no objections he would commit the changes on Wednesday or Thursday. Is this enough for you (Steve) or do you prefer a formal vote?

[aboudreault]

Fixed and committed in r10678. Free free to test your applications and report bug here.

[sdlime]

Cool, glad you went ahead. Should some sort of a reference be put together for developers so we don't make the same mistakes moving forward? I'm thinking of don't to this, do this instead examples.

Steve

[aboudreault]

A small guideline about string manipulations is available there: ​http://trac.osgeo.org/mapserver/wiki/CodingGuidelines