Re: sudo alloc.c -- cleanup required?
Sudo runs on more systems thsan just OpenBSD and so has a lot of configure goo and defines as a result. There's really no point in removing that. Any changes made to the sudo in OpenBSD just makes updates harder. The alloc functions implement integer overflow checks that are not present on most systems as well as a malloc(0) check that has caught bugs in the past. Nothing in sudo should be calling malloc with a zero size. - todd
Re: sudo alloc.c -- cleanup required?
Great explanation - thanks. On 04/24/14 08:28, Todd C. Miller wrote: Sudo runs on more systems thsan just OpenBSD and so has a lot of configure goo and defines as a result. There's really no point in removing that. Any changes made to the sudo in OpenBSD just makes updates harder. The alloc functions implement integer overflow checks that are not present on most systems as well as a malloc(0) check that has caught bugs in the past. Nothing in sudo should be calling malloc with a zero size. - todd
sudo alloc.c -- cleanup required?
Hi,
I see something with sudo which I would like to raise with the team. This
impacts most of the source files in the sudo package.
Before I go down this rat hole, I wanted to run it by you folks to see if you
agree.
/*
* If there is no SIZE_MAX or SIZE_T_MAX we have to assume that size_t
* could be signed (as it is on SunOS 4.x). This just means that
* emalloc2() and erealloc3() cannot allocate huge amounts on such a
* platform but that is OK since sudo doesn't need to do so anyway.
*/
#ifndef SIZE_MAX
# ifdef SIZE_T_MAX
# define SIZE_MAX SIZE_T_MAX
# else
# define SIZE_MAX INT_MAX
# endif /* SIZE_T_MAX */
#endif /* SIZE_MAX */
SunOS 4.x? The latest version is 11.x I believe. Can we do without this?
Lets look at emalloc() and emalloc2()
/*
* emalloc() calls the system malloc(3) and exits with an error if
* malloc(3) fails.
*/
void *
emalloc(size)
size_t size;
{
void *ptr;
if (size == 0)
errorx(1, internal error, tried to emalloc(0));
if ((ptr = malloc(size)) == NULL)
errorx(1, unable to allocate memory);
return(ptr);
}
/*
* emalloc2() allocates nmemb * size bytes and exits with an error
* if overflow would occur or if the system malloc(3) fails.
*/
void *
emalloc2(nmemb, size)
size_t nmemb;
size_t size;
{
void *ptr;
if (nmemb == 0 || size == 0)
errorx(1, internal error, tried to emalloc2(0));
if (nmemb SIZE_MAX / size)
errorx(1, internal error, emalloc2() overflow);
size *= nmemb;
if ((ptr = malloc(size)) == NULL)
errorx(1, unable to allocate memory);
return(ptr);
}
I'm failing to see the need for these two functions. This could be implemented
with less code.
In fact, most of alloc.c contains wrapper functions. Some are useful, like
estrdup(), but could be implemented better (it calls the malloc wrapper).
Any thoughts on this?
--
Peter Malone pe...@petermalone.org
Re: sudo alloc.c -- cleanup required?
Hi,
On Wed, Apr 23, 2014 at 8:08 PM, Peter Malone pe...@petermalone.org wrote:
Hi,
I see something with sudo which I would like to raise with the team. This
impacts most of the source files in the sudo package.
Before I go down this rat hole, I wanted to run it by you folks to see if
you agree.
/*
* If there is no SIZE_MAX or SIZE_T_MAX we have to assume that size_t
* could be signed (as it is on SunOS 4.x). This just means that
* emalloc2() and erealloc3() cannot allocate huge amounts on such a
* platform but that is OK since sudo doesn't need to do so anyway.
*/
#ifndef SIZE_MAX
# ifdef SIZE_T_MAX
# define SIZE_MAX SIZE_T_MAX
# else
# define SIZE_MAX INT_MAX
# endif /* SIZE_T_MAX */
#endif /* SIZE_MAX */
SunOS 4.x? The latest version is 11.x I believe. Can we do without this?
OpenBSD can but this will break it for older SunOS releases. This decision
is up to millert@ but my guess is that if it still here the support is not
going away.
Todd will correct me if I am wrong but the decision for sudo to have
wrappers around malloc was probably chosen for security considerations. On
many OS it is legal have the argument size = 0 and the return in that case
would be a small chunk (12, 16 bytes). emalloc explicitly disallows that
and hence prevents a class of integer related vulnerabilities.
*/
void *
emalloc(size)
size_t size;
{
void *ptr;
if (size == 0)
errorx(1, internal error, tried to emalloc(0));
if ((ptr = malloc(size)) == NULL)
errorx(1, unable to allocate memory);
return(ptr);
}
/*
* emalloc2() allocates nmemb * size bytes and exits with an error
* if overflow would occur or if the system malloc(3) fails.
*/
void *
emalloc2(nmemb, size)
size_t nmemb;
size_t size;
{
void *ptr;
if (nmemb == 0 || size == 0)
errorx(1, internal error, tried to emalloc2(0));
if (nmemb SIZE_MAX / size)
errorx(1, internal error, emalloc2() overflow);
size *= nmemb;
if ((ptr = malloc(size)) == NULL)
errorx(1, unable to allocate memory);
return(ptr);
}
There is a bound check in this function that a naive use of malloc(nmemb *
size) will fail to account for and be vulnerable to an overflow.
I'm failing to see the need for these two functions. This could be
implemented with less code.
In fact, most of alloc.c contains wrapper functions. Some are useful, like
estrdup(), but could be implemented better (it calls the malloc wrapper).
I haven't gone through the rest of the functions in this file but if you
think that the implementations could be improved, give it a shot.
Cheers,
Nayden
Any thoughts on this?
--
Peter Malone pe...@petermalone.org
