29503 - security!!! nsURLEscape may trash stack when string is too long

Status: VERIFIED DUPLICATE of bug 29279

(Core :: Networking, defect, P3)

Description

[Frank Tang]

I hit a stack trash when I accenditial copy and paste a wrong string into the
Location bar and hit return. The same routine will be called by any URL I
believe, even some URL send by the server (from the net). It think this bug is a
big security problem. Web server can send long url to trash our stack or even
place virus into our code. I think this is simlar to the first Netsacpe security
bug Sept 1995.
Here is the stack trace
nsURLEscape(const char * 0x032b91c0, short 0x0080, nsCString & {...}) line 98 +
20 bytes
nsAppendURLEscapedString(nsCString & {...}, const char * 0x032b91c0, short
0x0080) line 117 + 18 bytes
nsStdURL::AppendString(nsCString & {...}, char * 0x032b91c0, nsStdURL::Format
ESCAPED, short 0x0080) line 290 + 18 bytes
nsStdURL::GetPath(nsStdURL * const 0x032b9300, char * * 0x0012d330) line 773 +
26 bytes
nsStdURL::GetSpec(nsStdURL * const 0x032b9300, char * * 0x0012d9ac) line 373 +
16 bytes
nsWebShell::LoadURL(nsWebShell * const 0x02b43b30, const unsigned short *
0x02146f80, const char * 0x00382fcc, nsIInputStream * 0x00000000, int
0x00000001, unsigned int 0x00000000, const unsigned int 0x00000000, nsISupports
* 0x00000000, const unsigned short * 0x00000000, const char * 0x00000000) line
2156 + 60 bytes
nsWebShell::LoadURL(nsWebShell * const 0x02b43b30, const unsigned short *
0x02146f80, nsIInputStream * 0x00000000, int 0x00000001, unsigned int
0x00000000, const unsigned int 0x00000000, nsISupports * 0x00000000, const
unsigned short * 0x00000000) line 1386
nsBrowserInstance::LoadUrl(nsBrowserInstance * const 0x02bdef00, const unsigned
short * 0x02146f80) line 957 + 34 bytes
XPTC_InvokeByIndex(nsISupports * 0x02bdef00, unsigned int 0x00000007, unsigned
int 0x00000001, nsXPTCVariant * 0x0012dbac) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x023a5670,
nsXPCWrappedNative * 0x02bdc3e0, const XPCNativeMemberDescriptor * 0x02bdc6a4,
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 0x00000001, long *
0x0212bbb0, long * 0x0012dd6c) line 898 + 43 bytes
WrappedNative_CallMethod(JSContext * 0x023a5670, JSObject * 0x00f9dc88, unsigned
int 0x00000001, long * 0x0212bbb0, long * 0x0012dd6c) line 200 + 34 bytes

Look at the code-

 	char tempBuffer[100];
 	int tempBufferPos = 0;

    char c1[] = " ";
    char c2[] = " ";
    char* const pc1 = c1;
    char* const pc2 = c2;

    for (i = 0; i < len; i++)
    {

        c1[0] = *(src+1);
        c2[0] = *(src+2);
        unsigned char c = *src++;

        /* if the char has not to be escaped or whatever follows % is
           a valid escaped string, just copy the char */
        if (IS_OK(c) || (c == HEX_ESCAPE && (pc1) && (pc2) &&
           PL_strpbrk(pc1, CheckHexChars) != 0 &&
           PL_strpbrk(pc2, CheckHexChars) != 0)) {
		    tempBuffer[tempBufferPos++]=c;
        }
        else
            /* do the escape magic */
        {
		    tempBuffer[tempBufferPos++] = HEX_ESCAPE;
            tempBuffer[tempBufferPos++] = hexChars[c >> 4];	/* high nibble */
            tempBuffer[tempBufferPos++] = hexChars[c & 0x0f]; /* low nibble */
        }
 		if(tempBufferPos == 96)
 		{
 			tempBuffer[tempBufferPos] = '¡À0';
 	        result += tempBuffer;
 			tempBufferPos = 0;
 		}
	}
 	tempBuffer[tempBufferPos] = '¡À0';
 	result += tempBuffer;
    return NS_OK;
}

and the local variable
	tempBufferPos	108
	len	160

I think the following is the bad line
 		if(tempBufferPos == 96)

Probably if(tempBufferPos >= 96) will fix it.
Also, is 100 big enough for most url ? I think it is not enough for some system
like bugzilla or yahoo map ...

Comment 1

[Frank Tang]

forget my last comment about the size limitation , i didn't realize that it will
add to the result ....

this is definitely a beta 1 stoper. May cause security attack if we don't fix it.

Comment 2

[Frank Tang]

The trick to reproduce this is not type a "very long" string. The tricky part is
to make sure when tempbufferPos is 95 the next characters need to be escaped.
For example, "http://" (len =7) +  88 'a' + ' ' + " a.html probably will do it.

Comment 3

[Frank Tang]

sorry, that I cannot make up a URL that reproduce this problem right now. But
from the code itself it is obvious it is possible to cause it trash the stack.
The for loop may increment tempbuffPos by 1 or 3 but the if statement after it
only match ONE value ( ==96)

Comment 4

[Warren Harris]

adf

Comment 5

[Andreas Otte]

Already fixed, thanks mscott.

*** This bug has been marked as a duplicate of 29279 ***

Comment 6

[Tom Everingham]

dupe of 29279