Closed Bug 29503 Opened 25 years ago Closed 25 years ago

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

Categories

(Core :: Networking, defect, P3)

defect

Tracking

()

VERIFIED DUPLICATE of bug 29279

People

(Reporter: ftang, Assigned: andreas.otte)

Details

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 ...
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.
Keywords: beta1
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.
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)
adf
Assignee: warren → andreas.otte
Already fixed, thanks mscott. *** This bug has been marked as a duplicate of 29279 ***
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → DUPLICATE
dupe of 29279
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.