http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/malloc/prmem.c&rev=3.19&mark=463#463 463 PR_IMPLEMENT(void *) PR_Malloc(PRUint32 size) on 64 bit systems, this truncates int64 to int32. example of theoretically wrong usage: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/mimeebod.cpp&rev=1.33&mark=424-432#424 424 body = (char *) PR_MALLOC(strlen(pre) + strlen(s2) + 425 strlen(suf) + 1); ... 431 PL_strcpy(body, pre); 432 PL_strcat(body, s2); on 424 the math is mod 2^32 and in 431-432 there is no math - just copying memory without checks. another one: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/lib/libc/src/strlen.c&rev=3.7&mark=43-44#44 43 PR_IMPLEMENT(PRUint32) 44 PL_strlen(const char *str) |int32| truncates length of strings >= 4GB == 2^32 with possible side effects as in: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/news/src/nsNNTPNewsgroupPost.cpp&mark=74-91&rev=#74 83 string = (char *)PR_Calloc(PL_strlen(oldString) + ... 88 PL_strcpy(string, oldString); even if PR_Calloc were sane, on 83 the math is mod 2^32 and on 88 there is no math this seems not to be a problem on 32bit with restriction of 4GB VM per process, but on 64 bit systems strings >2^32 are real.

>83 string = (char *)PR_Calloc(PL_strlen(oldString) + >... >88 PL_strcpy(string, oldString); >even if PR_Calloc were sane, on 83 the math is mod 2^32 and on 88 >there is no math that was a lie. what i meant was: even if the math was in infinite ring and/or there was a check for |+| overflowing, PL_strlen will return at most 2^32-1 and it will return 1 for C string of length 2^32+1

PR_Malloc is not malloc. PR_Calloc is not calloc. In general, the NSPR functions are explicitly and deliberately defined to use integral sizes that DO NOT vary from platform to platform, including from 32-bit to 64-bit. It's up to callers to use NSPR's functions correctly. So, if some code is calling PR_Malloc, and passing a 64-bit value for a 32-bit argument, that calling code is in error, but this is not a defect in the design or implementation of PR_Malloc. I'd be amused if any NNTP postings had sizes getting anywhere near 4 gigs. :)

We can't change the prototypes of these NSPR functions. We can make PL_strlen() call abort() if the string is longer than 2^32. A general solution to this class of integer overflow problems is that the callers need to enforce a maximum string length that's reasonable for the application. This max string length should be much smaller than the max integer size so that when a few such strings are concatenated, the total length won't overflow.

>I'd be amused if any NNTP postings had sizes getting anywhere near 4 gigs. :) the buzzword for this is "defense in depth" >so that when a few such strings >are concatenated, the total length won't overflow. you pick a constant |few| and |few+1| screws you, right?

Right, once you pick a constant |few|, you cannot concatenate more than |few| strings. |few| is typically very large. For example, if the max string length allowed for a specific application (say, HTTP headers) is 1 MB (2^20 bytes), then |few| is 2^12. The code snippets you listed in comment 0 concatenate only 3 strings plus 1 or 2 characters. The use of PRUint32 in PR_Malloc and PL_strlen does not introduce this security issue because the same source code has the issue on 32-bit systems where size_t is the same as PRUint32. The use of PRUint32 in PR_Malloc and PL_strlen does prevent 64-bit systems from allocating large (>= 4GB) memory blocks and using long (>= 4GB) strings.

>the same source code has the issue on 32-bit >systems where size_t is the same as PRUint32. basically you are right. yet a single PR_malloc(strlen(str)) seems safe on 32bits and vulnerable on 64 bits. probably adding |+1| is still safe on 32bits. 2^64 is just a random limit, it can easily be overflowed with arithmetic like width*height, though i find it nasty to be easily overflowed by seemingly innocent |strlen|. note that |PL_strlen| followed by |PL_strcpy| is not safe IMO on 64 bits.

note that on 32 bits, in most applications i have seen, creating a 2GB string is difficult if possible at all - the algorithms use additional memory that hits the 3G VM limit -> OOM

i consider this bug a real threat: at least linux vendors are shipping 64 bit builds. currently one can buy a machine with 8GB ram for less than $1000, probably even less if one can build large enough string to overflow, the chance for overflowing without crashing is not negligible: say your are copying a 4G string [A] that is in memory in overflowed space [B]. if addr[B] < addr[A] one won't hit SEGV and will overflow. possible mitigation is fixing Bug 367721 – limiting virtual memory (mainly 64bit)

Seems like we would be better off with size_t parameters for any successors (if there will be successors) to PR_*alloc. We could even add just new entry points today and save callers from having to police 32-bit overflow hazards. This still seems like a bug about NSPR's malloc wrappers. If so, the component should be set to NSPR. /be

brendan note that comment #0 describes truncation in PL_strlen() so PR_*alloc is not the only problem in this bug.

Rather than adding new NSPR functions, I suggest that you switch to the equivalent Standard C library functions. PR_Malloc ==> malloc PR_Calloc ==> calloc PR_Realloc ==> realloc PR_Free ==> free PL_strlen ==> strlen The only "advantage" of PL_strlen is that it returns 0 rather than crashing on a null pointer. There is little need to use PR_*alloc today. All platforms we support have native threads, so NSPR threads are native threads, and malloc can be used safely by NSPR threads. The NSPR "zone allocator" isn't enabled by Mozilla clients. I can mark these NSPR functions as deprecated in the NSPR Reference and header files.

comment #11 seems reasonable.

I found that NSPR also has a PL_strnlen function that can be used to limit the portion of a string you'll use. It may be useful: http://mxr.mozilla.org/nspr/ident?i=PL_strnlen I have a question for you guys. How should PL_strlen handle a string longer than 2^32? Right now it truncates the string length with a cast: return (PRUint32)l; The alternatives are: 1. call abort(). 2. return PR_UINT32_MAX or PR_INT32_MAX. Which failure mode is the best for security?

This won't fix the integer overflow problem, but is better than the current code.

i reiterate my opinion that there should be real fix for this, not shaky kludge. not trusting core library functions is not nice. >The alternatives are: see above. >1. call abort(). if these 2 are the only alternatives, i vote for this, though it may cause DOS. >2. return PR_UINT32_MAX or PR_INT32_MAX. imo this will be a blatant lie and shouldn't be deployed - it will badly screw this used code: char *str=PR_Malloc(PL_strlen(a)+1); PL_strcpy(str,a); (modulo +1 overflowing) >Which failure mode is the best for security? [1] is less ugly [2] is a blatant lie, not a failure

I checked in a variant of the patch (attacment 336159) when I fixed bug 492779.