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> > >