Opened 18 years ago
Last modified 18 years ago
#1613 assigned defect
segmentation fault in mapimagemap.c when creating large imagemaps
Reported by: | Owned by: | sdlime | |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | MapServer C Library | Version: | unspecified |
Severity: | normal | Keywords: | |
Cc: |
Description
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.
Change History (15)
comment:2 by , 18 years ago
Status: | new → assigned |
---|
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.
comment:3 by , 18 years ago
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.
comment:4 by , 18 years ago
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?
comment:5 by , 18 years ago
Cc: | added |
---|
comment:6 by , 18 years ago
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?
comment:7 by , 18 years ago
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 ---------------------------------------------------------------------
comment:8 by , 18 years ago
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
comment:9 by , 18 years ago
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().
comment:10 by , 18 years ago
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.
comment:11 by , 18 years ago
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
comment:12 by , 18 years ago
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);
comment:13 by , 18 years ago
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..."
comment:15 by , 18 years ago
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.
Note:
See TracTickets
for help on using tickets.