From Maurycy Prodeus <z33d isec.pl>: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Issue: ====== A critical security vulnerability has been found in Mozilla Project code handling NNTP protocol. Details: ======== Mozilla browser supports NNTP urls. Remote side is able to trigger news:// connection to any server. We found a flaw in NNTP handling code which may cause heap overflow and allow remote attacker to execute arbitrary code on client machine. Bugus function from nsNNTPProtocol.cpp: char *MSG_UnEscapeSearchUrl (const char *commandSpecificData) 329 { 330 char *result = (char*) PR_Malloc (PL_strlen(commandSpecificData) + 1); 331 if (result) 332 { 333 char *resultPtr = result; 334 while (1) 335 { 336 char ch = *commandSpecificData++; 337 if (!ch) 338 break; 339 if (ch == '\\') 340 { 341 char scratchBuf[3]; 342 scratchBuf[0] = (char) *commandSpecificData++; 343 scratchBuf[1] = (char) *commandSpecificData++; 344 scratchBuf[2] = '\0'; 345 int accum = 0; 346 PR_sscanf(scratchBuf, "%X", &accum); 347 *resultPtr++ = (char) accum; 348 } 349 else 350 *resultPtr++ = ch; 351 } 352 *resultPtr = '\0'; 353 } 354 return result; 355 } When commandSpecificData points to last (next is NULL) character which is '\\' copying loop may omit termination of source char array and overflow result buffer. Exploitation ============ I have attached proof of concept HTML file which causes heap corruption and crashes Mozilla 1.7.3 browser (with mozilla-mail). News server must be existing and available. - -- Maurycy Prodeus iSEC Security Research http://isec.pl/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.7 (GNU/Linux) iD8DBQFBauFTC+8U3Z5wpu4RAnLzAJ49gRC+SpRN93/0r5oHqEoRs1r6GgCgild3 A3te72LQqkW5KjonyD98jSA= =BnNQ -----END PGP SIGNATURE-----

Can we get the attachment in here?

by Maurycy Prodeus

*** Bug 264394 has been marked as a duplicate of this bug. ***

I confirmed the overflow and crash A '\' on the end will certainly trash memory, but at that point you're no longer reading attacker-supplied data; Are you claiming this is exploitable other than as a crash/denial-of-service? How would you do that? The "commandSpecificData" passed to this function is a pointer into a string that was strdup()d in nsNNTPProtocol::ParseURL(). The URL parsed there was an allocated null-terminated string pulled out of the original content. Whatever might have been after the null in the original URL location is gone.

this is old ugly code, wouldn't be a bad idea to examine the rest of the buffer usage here. Possibly convert a lot of it to the string classes.

Here's a rewrite using nsStrings. It assumes that the Substring class makes appropriate checks, if the chars are there, and returns something meaningful. It's completely untested, probably doesn't even compile (we're desparately lacking reference docs for nsString), because my build is still compiling.

oops.

This now compiles. Still can't test, because I am behind a proxy, so somebody else will have to do that.

Patch tests fine. More string copies involved, and I'm not so sure about replacing bogus escapes with "X"... need input from the mailnews folks.

+ result.Replace(slashpos, 3, err == NS_OK && ch != 0 ? (char) ch : 'X'); + slashpos++; This doesn't look right. Since Replace can shorten the string at that point, you might skip over characters when you have a sequence of multiple escaped characters in a row. This would be much simpler and safer (and probably faster) if you just accumulated the result in a different string.

> Since Replace can shorten the string at that point, you > might skip over characters when you have a sequence of multiple escaped > characters in a row. hm, I replace "\41" with "A". slashpos points to "\". So, slashpos++ should point to the char after "A" after the replacement.

Comment on attachment 162141 [details] [diff] [review] Patch, Way 2, Version 3 oops, you're right. My mistake.

is this ready for the aviary branch too? we should get it in soon if it is.

Thanks for the patch Ben. I just checked this into the aviary 1.0 branch.

Comment on attachment 162141 [details] [diff] [review] Patch, Way 2, Version 3 de facto a=aviary: it's in already :-) a=dveditz for the 1.7 branch

Checked into trunk.

Checked into 1.7 branch. dveditz, I don't know the right keyword, can you set that, please? In both checkins, I changed the superfluous PRUnichar('\\') to '\\' (for nsCAutoString::FindChar).

now that 1.7.5 and tbird 1.0 are released, can we remove the security flag?

bug made the news, opening. <http://isec.pl/vulnerabilities/isec-0020-mozilla.txt> <http://www.heise.de/newsticker/meldung/54710>

can somebody open up Bug 264394, too? it's a dupe of this one... marc