segmentation fault in mapimagemap.c when creating large imagemaps

[roland@…]

Mapserver crashes (segmentation fault) when using the imagemap driver to 
generate a large HTML map. This bug is caused by a buffer overflow in the 
vsprintf function in mapimagemap.c.

The function im_iprintf is used to concat a string and increase the allocated 
buffersize if the remaining size is too small. The buffersize is checked by 
using the return value of the vsnprintf. This can not be checked by using the 
return value of the vsprintf function. 
On 2004/11/23 17:09:05 Revision 1.19, Frank added a compiler check on 
vsnprintf before using it. If HAVE_VSNPRINTF is undefined (why is it 
undefined?) the vsprintf function will be used. This causes a buffer overflow 
when the generated HTML is larger then the buffer size.

[sdlime]

Add Frank as a CC since the bug references a modification he made.

Steve

[fwarmerdam]

Roland, 

What platform are you on?  Do you have vsnprintf()?

Hmm, looking through the MapServer configure and related stuff, it isn't
at all clear to me that any test for vsnprintf is ever done.  Nor is there
a way for HAVE_VSNPRINTF to get set.  

[fwarmerdam]

I have done test builds on win32, and linux and in both the HAVE_VSNPRINTF
macro is defined, though it isn't done in any MapServer include file or 
makefile.  It must be inherited from some other package that gets included
in map.h. 

I did a quick scan, but it isn't obvious to me what package is providing
the definition.  But depending on an external definition seems ... iffy. 

[dmorissette]

I would guess it comes from GDAL/OGR's cpl_config.h.

We could add a test to configure.in in the block where we look for other string
functions:
AC_CHECK_FUNC(vsnprintf, ,      STRINGS="-DHAVE_VSNPRINTF $STRINGS")

Is this the only definition that we need?

[fwarmerdam]

Daniel, 

I tried to track through mapio's includes (mainly map.h) 
but I couldn't see that this would include gdal.h.  

We could add the suggested logic to configure.in, but I kind of hate
to start dumping lots of -D macros into the makefiles.  I guess that
would be best for now.  Do  you want to try it?

[dmorissette]

I'm not setup to easily test. Roland, I have modified the MapServer 4.8
configure script to define HAVE_VSNPRINTF if it's available on the target
system. It's not in CVS yet, but you can download a copy here:
http://www2.dmsolutions.ca/tmp/configure.gz

Can you please try running this configure script on your system and see if that
resolves the bug.

If it does then I'll apply the patch in CVS.

Note for anyone reading the bug, the AC_CHECK_FUNC() call in comment #4 is not
correct, the correct patch (assuming Roland verifies it) would be:

--- configure.in        13 Dec 2005 22:52:08 -0000      1.134
+++ configure.in        19 Jan 2006 18:26:22 -0000
@@ -46,6 +46,7 @@
 AC_CHECK_FUNC(strncasecmp, , STRINGS="-DNEED_STRNCASECMP $STRINGS")
 AC_CHECK_FUNC(strdup, ,      STRINGS="-DNEED_STRDUP $STRINGS")
 AC_CHECK_FUNC(strlcat, ,      STRINGS="-DNEED_STRLCAT $STRINGS")
+AC_CHECK_FUNC(vsnprintf, STRINGS="-DHAVE_VSNPRINTF $STRINGS", )
 AC_SUBST(STRINGS, $STRINGS)
  
 dnl ---------------------------------------------------------------------

[roland@…]

Hi Daniel,

I've test your patch succesfully. HAVE_VSNPRINTF was defined. For those people 
who don't have vsn_printf on their system, the bug in im_iprintf still exists.

Roland

[dmorissette]

I have applied the patch to detect and add -DHAVE_VSNPRINTF to the configure
script in both 4.8 and 4.9-dev in CVS.

You're right that the issue remains for systems where vsnprintf() is not
available. What do we want to do Steve/Frank? The quick solution seems to be to
completely disable imagemap support if vsnprintf is not available.

A possile long-term fix would be to add an argument to im_iprintf() that would
be passed on each call and that would be the maximum buffer space that could be
used by this call (the caller should normally know that), and then the buffer
would be tested/reallocated to have at least this much space available before
the call to vsprintf().

[fwarmerdam]

I think that if vsnprintf() is not available, then im_iprintf() ought
to pre-allocate the string buffer to have a large amount of free space
(say 20K).  This would still not be safe, but it would almost always work.

It might be nice if configure reported at the end of the configuration
that some operations will be unsafe and may cause mapserver to crash if
vsnprintf() is not available. 

[sdlime]

I'm suprised the image map support needs the function. The template processing
code builds much larger buffers with just gsub and strcatalloc.

I guess Frank's suggestion is ok temporarily, but longer term the way the
imagemap is build probably needs a re-write.

Steve

[dmorissette]

From a browse of the code, it seems that the largest string that is passed to
the im_iprintf() function label text, instead of using a fixed buffer size of
20k, or any arbitrary value, I thought that requiring a minimum amount of free
space in the buffer could be safer (512 bytes should be large enough for most
(any) label). What would you think of the following as a temporary patch?

                /* If vsnprintf() is not available then require a minimum
                 * of 512 bytes of free space to prevent a buffer overflow
                 * This is not fully bulletproof but should help, see bug 1613
                 */
                if (remaining < 512)
                    n = -1;  
                else
                    n = vsprintf((*(ps->string)) + ps->string_len, fmt, ap);

[dmorissette]

Doh! I can't type! Of course here I meant: "...it seems that the largest string
that can be passed to the im_iprintf() function is a label text string..."

[fwarmerdam]

This seems reasonable to me. 

[dmorissette]

I have applied the patch above in 4.8 and 4.9dev and tested it with a huge
imagemap with valgrind and we do not overflow the buffer any more. 

However, there is still potential for trouble with text label strings larger
then 500 bytes, which is unlikely but still possible, so we'll keep the bug open
for a better long term fix.