Last Comment Bug 264388 - Heap overflow in MSG_UnEscapeSearchUrl
: Heap overflow in MSG_UnEscapeSearchUrl
: crash, fixed-aviary1.0, fixed1.4.4, fixed1.7.5
Product: MailNews Core
Classification: Components
Component: Networking: NNTP (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Ben Bucksch (:BenB)
: 264394 (view as bug list)
Depends on:
Blocks: 248511 sbb+
  Show dependency treegraph
Reported: 2004-10-14 10:23 PDT by Ben Bucksch (:BenB)
Modified: 2009-01-22 10:17 PST (History)
10 users (show)
dbaron: blocking1.7.5+
chofmann: blocking‑aviary1.0+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Proof of concept (216 bytes, text/html)
2004-10-14 11:09 PDT, Ben Bucksch (:BenB)
no flags Details
patch (867 bytes, patch)
2004-10-14 11:36 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Splinter Review
Patch, Way 2 (rewrite using nsString), Version 1 (1.72 KB, patch)
2004-10-14 12:13 PDT, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Patch, Way 2, Version 2 (1.73 KB, patch)
2004-10-14 12:14 PDT, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Patch, Way 2, Version 3 (1.73 KB, patch)
2004-10-14 17:06 PDT, Ben Bucksch (:BenB)
mozilla: review+
roc: superreview+
dveditz: approval‑aviary+
dveditz: approval1.7.5+
Details | Diff | Splinter Review

Description Ben Bucksch (:BenB) 2004-10-14 10:23:09 PDT
From Maurycy Prodeus <z33d>:

Hash: SHA1


A critical security vulnerability has been found in Mozilla Project code 
handling NNTP protocol.


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.


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

Version: GnuPG v1.0.7 (GNU/Linux)

Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2004-10-14 11:03:18 PDT
Can we get the attachment in here?
Comment 2 Ben Bucksch (:BenB) 2004-10-14 11:09:08 PDT
Created attachment 162097 [details]
Proof of concept

by Maurycy Prodeus
Comment 3 Ben Bucksch (:BenB) 2004-10-14 11:10:29 PDT
*** Bug 264394 has been marked as a duplicate of this bug. ***
Comment 4 Daniel Veditz [:dveditz] 2004-10-14 11:21:06 PDT
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.
Comment 5 Daniel Veditz [:dveditz] 2004-10-14 11:29:28 PDT
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.
Comment 6 Daniel Veditz [:dveditz] 2004-10-14 11:36:07 PDT
Created attachment 162104 [details] [diff] [review]
Comment 7 Ben Bucksch (:BenB) 2004-10-14 12:13:16 PDT
Created attachment 162109 [details] [diff] [review]
Patch, Way 2 (rewrite using nsString), Version 1

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.
Comment 8 Ben Bucksch (:BenB) 2004-10-14 12:14:32 PDT
Created attachment 162110 [details] [diff] [review]
Patch, Way 2, Version 2

Comment 9 Ben Bucksch (:BenB) 2004-10-14 17:06:59 PDT
Created attachment 162141 [details] [diff] [review]
Patch, Way 2, Version 3

This now compiles. Still can't test, because I am behind a proxy, so somebody
else will have to do that.
Comment 10 Daniel Veditz [:dveditz] 2004-10-15 12:56:36 PDT
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.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-10-19 06:13:42 PDT
+    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.
Comment 12 Ben Bucksch (:BenB) 2004-10-19 06:23:38 PDT
> 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 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-10-19 06:56:11 PDT
Comment on attachment 162141 [details] [diff] [review]
Patch, Way 2, Version 3

oops, you're right. My mistake.
Comment 14 chris hofmann 2004-10-19 12:32:23 PDT
is this ready for the aviary branch too?  we should get it in soon if it is.
Comment 15 Scott MacGregor 2004-10-19 14:00:17 PDT
Thanks for the patch Ben. I just checked this into the aviary 1.0 branch. 
Comment 16 Daniel Veditz [:dveditz] 2004-10-19 19:07:46 PDT
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
Comment 17 Ben Bucksch (:BenB) 2004-10-20 03:15:24 PDT
Checked into trunk.
Comment 18 Ben Bucksch (:BenB) 2004-10-20 03:43:59 PDT
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
Comment 19 Christian :Biesinger (don't email me, ping me on IRC) 2005-01-01 08:06:07 PST
now that 1.7.5 and tbird 1.0 are released, can we remove the security flag?
Comment 20 Ben Bucksch (:BenB) 2005-01-01 22:32:21 PST
bug made the news, opening.
Comment 21 Marc Bejarano 2005-01-10 11:20:41 PST
can somebody open up Bug 264394, too?  it's a dupe of this one...


Note You need to log in before you can comment on or make changes to this bug.