445295 - misuse of int32 and int64 in API on 64 bit platforms

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[georgi - hopefully not receiving bugspam]

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.

Comment 1

[georgi - hopefully not receiving bugspam]

>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

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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. :)

Comment 3

[Wan-Teh Chang]

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.

Comment 4

[georgi - hopefully not receiving bugspam]

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

Comment 5

[Wan-Teh Chang]

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.

Comment 6

[georgi - hopefully not receiving bugspam]

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

Comment 7

[georgi - hopefully not receiving bugspam]

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 

Comment 8

[georgi - hopefully not receiving bugspam]

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)

Comment 9

[Brendan Eich [:brendan]]

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

Comment 10

[georgi - hopefully not receiving bugspam]

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

Comment 11

[Wan-Teh Chang]

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 12

[georgi - hopefully not receiving bugspam]

comment #11 seems reasonable.

Comment 13

[Wan-Teh Chang]

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?

Comment 14

[Wan-Teh Chang]

Attachment: PL_strlen should also assert valid string length in optimized build

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

Comment 15

[georgi - hopefully not receiving bugspam]

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

Comment 16

[Wan-Teh Chang]

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