264388 - Heap overflow in MSG_UnEscapeSearchUrl

Status: RESOLVED FIXED

(MailNews Core :: Networking: NNTP, defect)

Description

[Ben Bucksch (:BenB)]

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

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Can we get the attachment in here?

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Proof of concept

by Maurycy Prodeus

Comment 3

[Ben Bucksch (:BenB)]

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

Comment 4

[Daniel Veditz [:dveditz]]

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

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

Attachment: patch

Comment 7

[Ben Bucksch (:BenB)]

Attachment: 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)]

Attachment: Patch, Way 2, Version 2

oops.

Comment 9

[Ben Bucksch (:BenB)]

Attachment: 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]]

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)]

+    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)]

> 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)]

Comment on attachment 162141 [details] [diff] [review]
Patch, Way 2, Version 3

oops, you're right. My mistake.

Comment 14

[chris hofmann]

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

Comment 15

[Scott MacGregor]

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

Comment 16

[Daniel Veditz [:dveditz]]

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)]

Checked into trunk.

Comment 18

[Ben Bucksch (:BenB)]

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

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 20

[Ben Bucksch (:BenB)]

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

Comment 21

[Marc Bejarano]

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

marc