From: SourceForge.net <no...@so...> - 2005-08-10 17:10:07
|
Bugs item #1256046, was opened at 2005-08-10 17:10 Message generated for change (Tracker Item Submitted) made by Item Submitter You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102915&aid=1256046&group_id=2915 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: None Group: None Status: Open Resolution: None Priority: 5 Submitted By: Orion Poplawski (opoplawski) Assigned to: Nobody/Anonymous (nobody) Summary: buffer overflow in plcont.c:plfloatlabel() Initial Comment: There is a buffer overflow in plfloatlabel in plcont.c in version 5.5.3. tmpstring is obviously too small. I patched locally by doubling its size, but really it should get changed to use snprintf() and the like. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=102915&aid=1256046&group_id=2915 |
From: Alan W. I. <ir...@be...> - 2005-08-10 20:48:12
|
On 2005-08-10 10:10-0700 SourceForge.net wrote: > Bugs item #1256046, was opened at 2005-08-10 17:10 > Message generated for change (Tracker Item Submitted) made by Item Submitter > You can respond by visiting: > https://sourceforge.net/tracker/?func=detail&atid=102915&aid=1256046&group_id=2915 > > Please note that this message will contain a full copy of the comment thread, > including the initial issue submission, for this request, > not just the latest update. > Category: None > Group: None > Status: Open > Resolution: None > Priority: 5 > Submitted By: Orion Poplawski (opoplawski) > Assigned to: Nobody/Anonymous (nobody) > Summary: buffer overflow in plcont.c:plfloatlabel() > > Initial Comment: > There is a buffer overflow in plfloatlabel in plcont.c > in version 5.5.3. tmpstring is obviously too small. I > patched locally by doubling its size, but really it > should get changed to use snprintf() and the like. The code in question is char form[10], tmpstring[10]; 10 is obviously too small, but making it very large is not the answer either since some cracker will always try something larger. Maurice, as a veteran C coder do you feel the proposed move to snprintf() is a good solution for eliminating buffer overflows from user text input to PLplot? Are there cross-platform issues with snprintf? Once we agree on a good solution, I am willing to help propagate it to all our code since I think dealing with security issues should be a priority for our development team. For example, plugging security holes is required before we can put up a web demo of PLplot. Also, if some of our users are already trying web demos of PLplot, PLplot may become famous for the wrong reasons. :-) Alan __________________________ Alan W. Irwin email: ir...@be... phone: 250-727-2902 Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the Yorick front-end to PLplot (yplot.sf.net); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Andrew R. <aro...@ya...> - 2005-08-11 03:30:33
|
At 01:48 PM 10/08/2005 -0700, you wrote: >The code in question is > >char form[10], tmpstring[10]; > >10 is obviously too small, but making it very large is not the answer either >since some cracker will always try something larger. > >Maurice, as a veteran C coder do you feel the proposed move to snprintf() is >a good solution for eliminating buffer overflows from user text input to >PLplot? Are there cross-platform issues with snprintf? snprintf isn't ANSI, so compilers are not obliged to support it. -Andrew |
From: <mj...@ga...> - 2005-08-11 03:51:42
|
Andrew Roach writes: > At 01:48 PM 10/08/2005 -0700, you wrote: > > >The code in question is > > > >char form[10], tmpstring[10]; > > > >10 is obviously too small, but making it very large is not the answer either > >since some cracker will always try something larger. > > > >Maurice, as a veteran C coder do you feel the proposed move to snprintf() is > >a good solution for eliminating buffer overflows from user text input to > >PLplot? Are there cross-platform issues with snprintf? > > snprintf isn't ANSI, so compilers are not obliged to support it. Yeah it looked pretty suspicious to me too, but according to my RH9 man page: NOTES The glibc implementation of the functions snprintf and vsnprintf con- forms to the C99 standard, i.e., behaves as described above, since glibc version 2.1. Until glibc 2.0.6 they would return -1 when the out- put was truncated. so it is part of the standard now. I never did look at the C99 standard much. Of course, we only require C89 compliance. There may be an autoconf macro that'd allow us to portably use it, i.e. just on systems that support it. Anyone want to look into that? For the others just stick with what we have but with a *much* bigger buffer. As for the latter, it won't be secure but maybe in this & similar cases just draw a line in the sand & require C99 compliance for some semblance of security. And that's if someone does the monumental job of a security audit through the entire code base. Plus we'd have to figure out how to monitor submissions for security flaws. -- Maurice LeBrun mj...@ga... |
From: Arjen M. <arj...@wl...> - 2005-08-11 06:33:46
|
mj...@ga... wrote: >=20 > > > > snprintf isn't ANSI, so compilers are not obliged to support it. >=20 The MS Visual C/C++ compiler supports a function _snprintf() (the underscore is used to distinguish ANSI-standard functions from ubiquitous "almost-standard"=20 functions. Well, that can easily be solved with a macro for the win32 driver ... > Yeah it looked pretty suspicious to me too, but according to my RH9 man= page: >=20 > NOTES > The glibc implementation of the functions snprintf and vsnprin= tf con- > forms to the C99 standard, i.e., behaves as described above,= since > glibc version 2.1. Until glibc 2.0.6 they would return -1 when t= he out- > put was truncated. >=20 > so it is part of the standard now. I never did look at the C99 standar= d much. > Of course, we only require C89 compliance. >=20 I was uncertain of the semantics of this function, so I tried it out with a little program on Windows, using MSVC/C++ 6.0: #include <stdio.h> #include <stdlib.h> int main( int argc, char *argv[] ) { float x =3D 1.0e20 ; char string[10] ; int i ; i =3D _snprintf( string, sizeof(string)-1, "%f", x ) ; printf( "Returned: %d\n", i ) ; printf( "String: %s\n", string ) ; } =20 The result was _not_ what I had hoped for: Returned: -1 String: 100000002=A6=A6=A68x=A1`+=A0? Press any key to continue It appears that the MSVC implementation just checks whether more than the buffer was used but it does not guarantee that _at most_ the available buffer is used.=20 For the win32 driver this is not going to be a universal solution! Isn't it possible to do the check ourselves somehow? Regards, Arjen |
From: Alan W. I. <ir...@be...> - 2005-08-11 14:44:58
|
On 2005-08-11 08:33+0200 Arjen Markus wrote: > I was uncertain of the semantics of this function, so I tried it out > with a > little program on Windows, using MSVC/C++ 6.0: > > #include <stdio.h> > #include <stdlib.h> > > int main( int argc, char *argv[] ) { > float x =3D 1.0e20 ; > char string[10] ; > int i ; > > i =3D _snprintf( string, sizeof(string)-1, "%f", x ) ; > printf( "Returned: %d\n", i ) ; > printf( "String: %s\n", string ) ; > } > > The result was _not_ what I had hoped for: > > Returned: -1 > String: 100000002=A6=A6=A68x=A1`+=A0? > Press any key to continue On Linux with the change from _snprintf to snprintf the result is Returned: 28 String: 10000000 Thanks for the demo programme. > > > It appears that the MSVC implementation just checks whether more > than the buffer was used but it does not guarantee that _at most_ > the available buffer is used. Yeah, that's ugly. Why go out of your way to provide the function with the key security functionality (string truncation to make sure no writing can occur outside the specified length) completely broken? > > For the win32 driver this is not going to be a universal solution! > > Isn't it possible to do the check ourselves somehow? Yes. Please reread my earlier post in this thread where I found not only a= n autoconf macro for testing snprintf functionality but also replacement code for snprintf if a platform didn't have a working snprintf. Alan __________________________ Alan W. Irwin email: ir...@be... phone: 250-727-2902 Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the Yorick front-end to PLplot (yplot.sf.net); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Orion P. <or...@co...> - 2005-08-11 15:27:57
|
Alan W. Irwin wrote: > On 2005-08-11 08:33+0200 Arjen Markus wrote: >=20 >> I was uncertain of the semantics of this function, so I tried it out >> with a >> little program on Windows, using MSVC/C++ 6.0: >> >> #include <stdio.h> >> #include <stdlib.h> >> >> int main( int argc, char *argv[] ) { >> float x =3D 1.0e20 ; >> char string[10] ; >> int i ; >> >> i =3D _snprintf( string, sizeof(string)-1, "%f", x ) ; >> printf( "Returned: %d\n", i ) ; >> printf( "String: %s\n", string ) ; >> } >> >> The result was _not_ what I had hoped for: >> >> Returned: -1 >> String: 100000002=A6=A6=A68x=A1`+ ? >> Press any key to continue >=20 >=20 > On Linux with the change from _snprintf to snprintf the result is > Returned: 28 > String: 10000000 >=20 > Thanks for the demo programme. >=20 >> >> >> It appears that the MSVC implementation just checks whether more >> than the buffer was used but it does not guarantee that _at most_ >> the available buffer is used. >=20 >=20 > Yeah, that's ugly. Why go out of your way to provide the function with= the > key security functionality (string truncation to make sure no writing c= an > occur outside the specified length) completely broken? It's not (completely) broken. It only wrote 9 characters into the=20 buffer. What it didn't do was put in a trailing null. From the linux man pages: Return value Upon successful return, these functions return the number of=20 characters printed (not including the trailing '\0' used to end output to=20 strings). The functions snprintf and vsnprintf do not write more than=20 size bytes (including the trailing '\0'). If the output was truncated=20 due to this limit then the return value is the number of characters (not=20 including the trailing '\0') which would have been written to the final=20 string if enough space had been available. Thus, a return value of size=20 or more means that the output was truncated. (See also below under=20 NOTES.) If an output error is encountered, a negative value is returned. NOTES The glibc implementation of the functions snprintf and vsnprintf=20 conforms to the C99 standard, i.e., behaves as described above, since=20 glibc ver- sion 2.1. Until glibc 2.0.6 they would return -1 when the=20 output was truncated. Looks like MSVC is more like glibc 2.0.6 behaviour. --=20 Orion Poplawski System Administrator 303-415-9701 x222 Colorado Research Associates/NWRA FAX: 303-415-9702 3380 Mitchell Lane, Boulder CO 80301 http://www.co-ra.com |
From: Arjen M. <arj...@wl...> - 2005-08-15 06:22:47
|
Orion Poplawski wrote: >=20 > Alan W. Irwin wrote: > > On 2005-08-11 08:33+0200 Arjen Markus wrote: > > > >> I was uncertain of the semantics of this function, so I tried it out > >> with a > >> little program on Windows, using MSVC/C++ 6.0: > >> > >> #include <stdio.h> > >> #include <stdlib.h> > >> > >> int main( int argc, char *argv[] ) { > >> float x =3D 1.0e20 ; > >> char string[10] ; > >> int i ; > >> > >> i =3D _snprintf( string, sizeof(string)-1, "%f", x ) ; > >> printf( "Returned: %d\n", i ) ; > >> printf( "String: %s\n", string ) ; > >> } > >> > >> The result was _not_ what I had hoped for: > >> > >> Returned: -1 > >> String: 100000002=A6=A6=A68x=A1`+ ? > >> Press any key to continue > > > > > > On Linux with the change from _snprintf to snprintf the result is > > Returned: 28 > > String: 10000000 > > > > Thanks for the demo programme. > > > >> > >> > >> It appears that the MSVC implementation just checks whether more > >> than the buffer was used but it does not guarantee that _at most_ > >> the available buffer is used. > > > > > > Yeah, that's ugly. Why go out of your way to provide the function wi= th the > > key security functionality (string truncation to make sure no writing= can > > occur outside the specified length) completely broken? >=20 > It's not (completely) broken. It only wrote 9 characters into the > buffer. What it didn't do was put in a trailing null. >=20 > From the linux man pages: >=20 > Return value > Upon successful return, these functions return the number of > characters > printed (not including the trailing '\0' used to end output to > strings). > The functions snprintf and vsnprintf do not write more than > size bytes > (including the trailing '\0'). If the output was truncated > due to this > limit then the return value is the number of characters (not > including > the trailing '\0') which would have been written to the final > string if > enough space had been available. Thus, a return value of size > or more > means that the output was truncated. (See also below under > NOTES.) If an > output error is encountered, a negative value is returned. >=20 > NOTES > The glibc implementation of the functions snprintf and vsnprint= f > conforms > to the C99 standard, i.e., behaves as described above, since > glibc ver- > sion 2.1. Until glibc 2.0.6 they would return -1 when the > output was > truncated. >=20 > Looks like MSVC is more like glibc 2.0.6 behaviour. >=20 Okay, but that means that the resulting is unuseable if an error occurs: - there is no way of knowing how many characters were actually written - there is no way of setting the null character afterwards No, wait, if the buffer is initialized properly, then it might work: char buffer[10] ; buffer[sizeof(buffer)-1] =3D '\0' ; snprintf( buffer, sizeof(buffer)-1, .... ) ; Rather elaborate and confusing ("sizeof(buffer)-1" having two=20 entirely different meanings), but that ought to work.=20 I tested this with my test program, and indeed this did the trick! Regards, Arjen |
From: Alan W. I. <ir...@be...> - 2005-08-11 04:51:08
|
On 2005-08-10 21:40-0500 mj...@ga... wrote: > Andrew Roach writes: > > At 01:48 PM 10/08/2005 -0700, you wrote: > > > > >The code in question is > > > > > >char form[10], tmpstring[10]; > > > > > >10 is obviously too small, but making it very large is not the answer either > > >since some cracker will always try something larger. > > > > > >Maurice, as a veteran C coder do you feel the proposed move to snprintf() is > > >a good solution for eliminating buffer overflows from user text input to > > >PLplot? Are there cross-platform issues with snprintf? > > > > snprintf isn't ANSI, so compilers are not obliged to support it. > > Yeah it looked pretty suspicious to me too, but according to my RH9 man page: > > NOTES > The glibc implementation of the functions snprintf and vsnprintf con- > forms to the C99 standard, i.e., behaves as described above, since > glibc version 2.1. Until glibc 2.0.6 they would return -1 when the out- > put was truncated. > > so it is part of the standard now. I never did look at the C99 standard much. > Of course, we only require C89 compliance. > > There may be an autoconf macro that'd allow us to portably use it, i.e. just > on systems that support it. Anyone want to look into that? Here is the result of a quick google search for (autoconf macro snprintf): http://autoconf-archive.cryp.to/ac_func_snprintf.html This macro's functionality looks like exactly what we need although we need an m4 expert to make sure there are no cross-platform (or otherwise) implementation glitches. The macro also refers to alternative snprintf code (see http://www.ijs.si/software/snprintf/) that can be used if snprintf is not functional on a particular platform. HTH. Alan __________________________ Alan W. Irwin email: ir...@be... phone: 250-727-2902 Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the Yorick front-end to PLplot (yplot.sf.net); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Arjen M. <arj...@wl...> - 2005-08-15 06:12:05
|
"Alan W. Irwin" wrote: > > Thanks for the demo programme. > Nitpicking: should that be "program" :) > > > > > > It appears that the MSVC implementation just checks whether more > > than the buffer was used but it does not guarantee that _at most_ > > the available buffer is used. > > Yeah, that's ugly. Why go out of your way to provide the function with the > key security functionality (string truncation to make sure no writing can > occur outside the specified length) completely broken? > > > > > For the win32 driver this is not going to be a universal solution! > > > > Isn't it possible to do the check ourselves somehow? > > Yes. Please reread my earlier post in this thread where I found not only an > autoconf macro for testing snprintf functionality but also replacement code > for snprintf if a platform didn't have a working snprintf. > I saw that, it just did not register properly, I guess. I can entirely live with that solution. Regards, Arjen |
From: Alan W. I. <ir...@be...> - 2005-12-06 02:24:46
|
On 2005-08-10 13:48-0700 Alan W. Irwin wrote: > On 2005-08-10 10:10-0700 SourceForge.net wrote: > >> Bugs item #1256046, was opened at 2005-08-10 17:10 >> Message generated for change (Tracker Item Submitted) made by Item >> Submitter >> You can respond by visiting: >> https://sourceforge.net/tracker/?func=detail&atid=102915&aid=1256046&group_id=2915 >> >> Please note that this message will contain a full copy of the comment >> thread, >> including the initial issue submission, for this request, >> not just the latest update. >> Category: None >> Group: None >> Status: Open >> Resolution: None >> Priority: 5 >> Submitted By: Orion Poplawski (opoplawski) >> Assigned to: Nobody/Anonymous (nobody) >> Summary: buffer overflow in plcont.c:plfloatlabel() >> >> Initial Comment: >> There is a buffer overflow in plfloatlabel in plcont.c >> in version 5.5.3. tmpstring is obviously too small. I >> patched locally by doubling its size, but really it >> should get changed to use snprintf() and the like. > > The code in question is > > char form[10], tmpstring[10]; This post generated some useful discussion. For example, Maurice asked for an autoconf macro, and I found them for him: > Here is the result of a quick google search for (autoconf macro snprintf): > http://autoconf-archive.cryp.to/ac_func_snprintf.html > This macro's functionality looks like exactly what we need although we need > an m4 expert to make sure there are no cross-platform (or otherwise) > implementation glitches. The macro also refers to alternative snprintf code > (see http://www.ijs.si/software/snprintf/) that can be used if snprintf is > not functional on a particular platform. So if snprintf is not available for a platform, it appears we can supply it from the above source. Maurice, would you be willing to take responsibility for making the decision on the best way to deal with this security issue? If you do take action and sort out the configuration problem with the above macro and/or above source for snprintf and fix just one of the insecure sprintf's anywhere in our code, then it should be straightforward for any developer here to propagate your solution elsewhere. Although the bad security aspects of the code that generates tmpstring (and form) depending on user input has not been resolved yet, the dimension for tmpstring leads to obvious string overflow for *any* exponent use. Thus, I decided to address that minor part of the problem by expanding to tmpstring[14] instead (see my recent commit). That gives room for exponents from -999 to 9999 which should be good enough for non-malicious use. Oops! I think that should be tmpstring[15] to allow for null termination so I have just made another cvs commit. Alan __________________________ Alan W. Irwin email: ir...@be... phone: 250-727-2902 Astronomical research affiliation with Department of Physics and Astronomy, University of Victoria (astrowww.phys.uvic.ca). Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); PLplot scientific plotting software package (plplot.org); the Yorick front-end to PLplot (yplot.sf.net); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |