Hi,
On Wed, Apr 23, 2014 at 8:08 PM, Peter Malone <[email protected]> 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 <[email protected]>
>
>
