I noticed this in PyErr_Format(exception, format, va_alist): char buffer[500]; /* Caller is responsible for limiting the format */ ... vsprintf(buffer, format, vargs); Making the caller responsible for this is error-prone. The danger, of course, is a buffer overflow caused by generating an error string that's larger than the buffer, possibly letting people execute arbitrary code. We could add a test to the configure script for vsnprintf() and use it when possible, but that only fixes the problem on platforms which have it. Can we find an implementation of vsnprintf() someplace? -- A.M. Kuchling http://starship.python.net/crew/amk/ One form to rule them all, one form to find them, one form to bring them all and in the darkness rewrite the hell out of them. -- Digital Equipment Corporation, in a comment from SENDMAIL Ruleset 3
On Sun, 14 Nov 1999, A.M. Kuchling wrote:
Apache has a safe implementation (they have reviewed the heck out of it for obvious reasons :-). In the Apache source distribution, it is located in src/ap/ap_snprintf.c. Cheers, -g -- Greg Stein, http://www.lyra.org/
"A.M. Kuchling" wrote:
In sysmodule.c, this check is done which should be safe enough since no "return" is issued (Py_FatalError() does an abort()): if (vsprintf(buffer, format, va) >= sizeof(buffer)) Py_FatalError("PySys_WriteStdout/err: buffer overrun"); -- Marc-Andre Lemburg ______________________________________________________________________ Y2000: 46 days left Business: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
On Mon, 15 Nov 1999, M.-A. Lemburg wrote:
I believe the return from vsprintf() itself would be the problem. Cheers, -g -- Greg Stein, http://www.lyra.org/
Greg Stein wrote:
Ouch, yes, you are right... but who could exploit this security hole ? Since PyErr_Format() is only reachable for C code, only bad programming style in extensions could make it exploitable via user input. Wouldn't it be possible to assign thread globals for these functions to use ? These would live on the heap instead of on the stack and eliminate the buffer overrun possibilities (I guess -- I don't have any experience with these...). -- Marc-Andre Lemburg ______________________________________________________________________ Y2000: 46 days left Business: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
M.-A. Lemburg writes:
99% of security holes arise out of carelessness, and besides, this buffer size doesn't seem to be documented in either api.tex or ext.tex. I'll look into borrowing Apache's implementation and modifying it into a varargs form. -- A.M. Kuchling http://starship.python.net/crew/amk/ I can also withstand considerably more G-force than most people, even though I do say so myself. -- The Doctor, in "The Ambassadors of Death"
Agreed. The limit of 500 chars, while technically undocumented, is part of the specs for PyErr_Format (which is currently wholly undocumented). The current callers all have explicit precautions, but of course I agree that this is a potential danger.
Assuming that Linux and Solaris have vsnprintf(), can't we just use the configure script to detect it, and issue a warning blaming the platform for those platforms that don't have it? That seems much simpler (from a maintenance perspective) than carrying our own implementation around (even if we can borrow the Apache version). --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum writes:
But people using an already-installed Python binary won't see any such configure-time warning, and won't find out about the potential problem. Plus, how do people fix the problem on platforms that don't have vsnprintf() -- switch to Solaris or Linux? Not much of a solution. (vsnprintf() isn't ANSI C, though it's a common extension, so platforms that lack it aren't really deficient.) Hmm... could we maybe use Python's existing (string % vars) machinery? <think think> No, that seems to be hard, because it would want PyObjects, and we can't know what Python types to convert the varargs to, unless we parse the format string (at which point we may as well get a vsnprintf() implementation. -- A.M. Kuchling http://starship.python.net/crew/amk/ A successful tool is one that was used to do something undreamed of by its author. -- S.C. Johnson
"Andrew M. Kuchling" wrote:
It's easy. You use two format strings. One a Python string format, and the other a Py_BuildValue format. See my other note. Jim -- Jim Fulton mailto:jim@digicool.com Python Powered! Technical Director (888) 344-4332 http://www.python.org Digital Creations http://www.digicool.com http://www.zope.org Under US Code Title 47, Sec.227(b)(1)(C), Sec.227(a)(2)(B) This email address may not be added to any commercial mail list with out my permission. Violation of my privacy with advertising or SPAM will result in a suit for a MINIMUM of $500 damages/incident, $1500 for repeats.
"Guido" == Guido van Rossum <guido@cnri.reston.va.us> writes:
Guido> Assuming that Linux and Solaris have vsnprintf(), can't we Guido> just use the configure script to detect it, and issue a Guido> warning blaming the platform for those platforms that don't Guido> have it? That seems much simpler (from a maintenance Guido> perspective) than carrying our own implementation around Guido> (even if we can borrow the Apache version). Mailman uses vsnprintf in it's C wrapper. There's a simple configure test... # Checks for library functions. AC_CHECK_FUNCS(vsnprintf) ...and for systems that don't have a vsnprintf, I modified a version from GNU screen. It may not have gone through the scrutiny of Apache's implementation, but for Mailman it was more important that it be GPL'd (not a Python requirement). -Barry
Guido van Rossum wrote:
All but one (checked them all): In ceval.c, function call_builtin, there is a possible security hole. If an extension module happens to create a very long type name (maybe just via a bug), we will crash. } PyErr_Format(PyExc_TypeError, "call of non-function (type %s)", func->ob_type->tp_name); return NULL; } ciao - chris -- Christian Tismer :^) <mailto:tismer@appliedbiometrics.com> Applied Biometrics GmbH : Have a break! Take a ride on Python's Kaiserin-Augusta-Allee 101 : *Starship* http://starship.python.net 10553 Berlin : PGP key -> http://wwwkeys.pgp.net PGP Fingerprint E182 71C7 1A9D 66E9 9D15 D3CC D4D7 93E2 1FAE F6DF we're tired of banana software - shipped green, ripens at home
All but one (checked them all):
Thanks for checking.
I would think that an extension module with a name of nearly 500 characters would draw a lot of attention as being ridiculous. If there was a bug through which you could make tp_name point to such a long string, you could probably exploit that bug without having to use this particular PyErr_Format() statement. However, I agree it's better to be safe than sorry, so I've checked in a fix making it %.400s. --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
All but one (checked them all):
[ceval.c without limits]
Of course this case is very unlikely. My primary intent was to create such a mess without an extension, and ExtensionClass seemed to be a candidate since it synthetizes a type name at runtime (!). This would have been dangerous since EC is in the heart of Zope. But, I could not get at this special case since EC always stands the class/instance checks and so this case can never happen :( The above lousy result was just to say *something* after no success.
However, I agree it's better to be safe than sorry, so I've checked in a fix making it %.400s.
cheap, consistent, fine - thanks - chris -- Christian Tismer :^) <mailto:tismer@appliedbiometrics.com> Applied Biometrics GmbH : Have a break! Take a ride on Python's Kaiserin-Augusta-Allee 101 : *Starship* http://starship.python.net 10553 Berlin : PGP key -> http://wwwkeys.pgp.net PGP Fingerprint E182 71C7 1A9D 66E9 9D15 D3CC D4D7 93E2 1FAE F6DF we're tired of banana software - shipped green, ripens at home
"A.M. Kuchling" wrote:
I would prefer to see a different interface altogether: PyObject *PyErr_StringFormat(errtype, format, buildformat, ...) So, you could generate an error like this: return PyErr_StringFormat(ErrorObject, "You had too many, %d, foos. The last one was %s", "iO", n, someObject) I implemented this in cPickle. See cPickle_ErrFormat. (Note that it always returns NULL.) Jim -- Jim Fulton mailto:jim@digicool.com Python Powered! Technical Director (888) 344-4332 http://www.python.org Digital Creations http://www.digicool.com http://www.zope.org Under US Code Title 47, Sec.227(b)(1)(C), Sec.227(a)(2)(B) This email address may not be added to any commercial mail list with out my permission. Violation of my privacy with advertising or SPAM will result in a suit for a MINIMUM of $500 damages/incident, $1500 for repeats.
On Sun, 14 Nov 1999, A.M. Kuchling wrote:
Apache has a safe implementation (they have reviewed the heck out of it for obvious reasons :-). In the Apache source distribution, it is located in src/ap/ap_snprintf.c. Cheers, -g -- Greg Stein, http://www.lyra.org/
"A.M. Kuchling" wrote:
In sysmodule.c, this check is done which should be safe enough since no "return" is issued (Py_FatalError() does an abort()): if (vsprintf(buffer, format, va) >= sizeof(buffer)) Py_FatalError("PySys_WriteStdout/err: buffer overrun"); -- Marc-Andre Lemburg ______________________________________________________________________ Y2000: 46 days left Business: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
On Mon, 15 Nov 1999, M.-A. Lemburg wrote:
I believe the return from vsprintf() itself would be the problem. Cheers, -g -- Greg Stein, http://www.lyra.org/
Greg Stein wrote:
Ouch, yes, you are right... but who could exploit this security hole ? Since PyErr_Format() is only reachable for C code, only bad programming style in extensions could make it exploitable via user input. Wouldn't it be possible to assign thread globals for these functions to use ? These would live on the heap instead of on the stack and eliminate the buffer overrun possibilities (I guess -- I don't have any experience with these...). -- Marc-Andre Lemburg ______________________________________________________________________ Y2000: 46 days left Business: http://www.lemburg.com/ Python Pages: http://www.lemburg.com/python/
M.-A. Lemburg writes:
99% of security holes arise out of carelessness, and besides, this buffer size doesn't seem to be documented in either api.tex or ext.tex. I'll look into borrowing Apache's implementation and modifying it into a varargs form. -- A.M. Kuchling http://starship.python.net/crew/amk/ I can also withstand considerably more G-force than most people, even though I do say so myself. -- The Doctor, in "The Ambassadors of Death"
Agreed. The limit of 500 chars, while technically undocumented, is part of the specs for PyErr_Format (which is currently wholly undocumented). The current callers all have explicit precautions, but of course I agree that this is a potential danger.
Assuming that Linux and Solaris have vsnprintf(), can't we just use the configure script to detect it, and issue a warning blaming the platform for those platforms that don't have it? That seems much simpler (from a maintenance perspective) than carrying our own implementation around (even if we can borrow the Apache version). --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum writes:
But people using an already-installed Python binary won't see any such configure-time warning, and won't find out about the potential problem. Plus, how do people fix the problem on platforms that don't have vsnprintf() -- switch to Solaris or Linux? Not much of a solution. (vsnprintf() isn't ANSI C, though it's a common extension, so platforms that lack it aren't really deficient.) Hmm... could we maybe use Python's existing (string % vars) machinery? <think think> No, that seems to be hard, because it would want PyObjects, and we can't know what Python types to convert the varargs to, unless we parse the format string (at which point we may as well get a vsnprintf() implementation. -- A.M. Kuchling http://starship.python.net/crew/amk/ A successful tool is one that was used to do something undreamed of by its author. -- S.C. Johnson
"Andrew M. Kuchling" wrote:
It's easy. You use two format strings. One a Python string format, and the other a Py_BuildValue format. See my other note. Jim -- Jim Fulton mailto:jim@digicool.com Python Powered! Technical Director (888) 344-4332 http://www.python.org Digital Creations http://www.digicool.com http://www.zope.org Under US Code Title 47, Sec.227(b)(1)(C), Sec.227(a)(2)(B) This email address may not be added to any commercial mail list with out my permission. Violation of my privacy with advertising or SPAM will result in a suit for a MINIMUM of $500 damages/incident, $1500 for repeats.
"Guido" == Guido van Rossum <guido@cnri.reston.va.us> writes:
Guido> Assuming that Linux and Solaris have vsnprintf(), can't we Guido> just use the configure script to detect it, and issue a Guido> warning blaming the platform for those platforms that don't Guido> have it? That seems much simpler (from a maintenance Guido> perspective) than carrying our own implementation around Guido> (even if we can borrow the Apache version). Mailman uses vsnprintf in it's C wrapper. There's a simple configure test... # Checks for library functions. AC_CHECK_FUNCS(vsnprintf) ...and for systems that don't have a vsnprintf, I modified a version from GNU screen. It may not have gone through the scrutiny of Apache's implementation, but for Mailman it was more important that it be GPL'd (not a Python requirement). -Barry
Guido van Rossum wrote:
All but one (checked them all): In ceval.c, function call_builtin, there is a possible security hole. If an extension module happens to create a very long type name (maybe just via a bug), we will crash. } PyErr_Format(PyExc_TypeError, "call of non-function (type %s)", func->ob_type->tp_name); return NULL; } ciao - chris -- Christian Tismer :^) <mailto:tismer@appliedbiometrics.com> Applied Biometrics GmbH : Have a break! Take a ride on Python's Kaiserin-Augusta-Allee 101 : *Starship* http://starship.python.net 10553 Berlin : PGP key -> http://wwwkeys.pgp.net PGP Fingerprint E182 71C7 1A9D 66E9 9D15 D3CC D4D7 93E2 1FAE F6DF we're tired of banana software - shipped green, ripens at home
All but one (checked them all):
Thanks for checking.
I would think that an extension module with a name of nearly 500 characters would draw a lot of attention as being ridiculous. If there was a bug through which you could make tp_name point to such a long string, you could probably exploit that bug without having to use this particular PyErr_Format() statement. However, I agree it's better to be safe than sorry, so I've checked in a fix making it %.400s. --Guido van Rossum (home page: http://www.python.org/~guido/)
Guido van Rossum wrote:
All but one (checked them all):
[ceval.c without limits]
Of course this case is very unlikely. My primary intent was to create such a mess without an extension, and ExtensionClass seemed to be a candidate since it synthetizes a type name at runtime (!). This would have been dangerous since EC is in the heart of Zope. But, I could not get at this special case since EC always stands the class/instance checks and so this case can never happen :( The above lousy result was just to say *something* after no success.
However, I agree it's better to be safe than sorry, so I've checked in a fix making it %.400s.
cheap, consistent, fine - thanks - chris -- Christian Tismer :^) <mailto:tismer@appliedbiometrics.com> Applied Biometrics GmbH : Have a break! Take a ride on Python's Kaiserin-Augusta-Allee 101 : *Starship* http://starship.python.net 10553 Berlin : PGP key -> http://wwwkeys.pgp.net PGP Fingerprint E182 71C7 1A9D 66E9 9D15 D3CC D4D7 93E2 1FAE F6DF we're tired of banana software - shipped green, ripens at home
"A.M. Kuchling" wrote:
I would prefer to see a different interface altogether: PyObject *PyErr_StringFormat(errtype, format, buildformat, ...) So, you could generate an error like this: return PyErr_StringFormat(ErrorObject, "You had too many, %d, foos. The last one was %s", "iO", n, someObject) I implemented this in cPickle. See cPickle_ErrFormat. (Note that it always returns NULL.) Jim -- Jim Fulton mailto:jim@digicool.com Python Powered! Technical Director (888) 344-4332 http://www.python.org Digital Creations http://www.digicool.com http://www.zope.org Under US Code Title 47, Sec.227(b)(1)(C), Sec.227(a)(2)(B) This email address may not be added to any commercial mail list with out my permission. Violation of my privacy with advertising or SPAM will result in a suit for a MINIMUM of $500 damages/incident, $1500 for repeats.
participants (8)
-
A.M. Kuchling -
Andrew M. Kuchling -
Barry A. Warsaw -
Christian Tismer -
Greg Stein -
Guido van Rossum -
Jim Fulton -
M.-A. Lemburg