The default bug view has changed. See this FAQ.

Heap overflow in MSG_UnEscapeSearchUrl

RESOLVED FIXED

Status

MailNews Core
Networking: NNTP
--
critical
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

(4 keywords)

Trunk
crash, fixed-aviary1.0, fixed1.4.4, fixed1.7.5
Dependency tree / graph
Bug Flags:
blocking1.7.5 +
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:dos])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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-----
(Assignee)

Updated

13 years ago
Flags: blocking1.7.x?
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?
Flags: blocking1.7.x? → blocking1.7.x+
Can we get the attachment in here?
(Assignee)

Comment 2

13 years ago
Created attachment 162097 [details]
Proof of concept

by Maurycy Prodeus
(Assignee)

Comment 3

13 years ago
*** Bug 264394 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
Summary: Explotable heap overflow in MSG_UnEscapeSearchUrl → Exploitable heap overflow in MSG_UnEscapeSearchUrl
(Assignee)

Updated

13 years ago
Assignee: sspitzer → ben.bucksch
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.
Keywords: crash
Whiteboard: [sg:dos]
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.
Created attachment 162104 [details] [diff] [review]
patch
(Assignee)

Updated

13 years ago
Summary: Exploitable heap overflow in MSG_UnEscapeSearchUrl → Heap overflow in MSG_UnEscapeSearchUrl
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
Created attachment 162110 [details] [diff] [review]
Patch, Way 2, Version 2

oops.
Attachment #162109 - Attachment is obsolete: true
(Assignee)

Comment 9

13 years ago
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.
Attachment #162110 - Attachment is obsolete: true
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.
(Assignee)

Comment 12

13 years ago
> 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.
(Assignee)

Updated

13 years ago
Attachment #162141 - Flags: superreview?(roc)
Attachment #162141 - Flags: review?(bienvenu)
Comment on attachment 162141 [details] [diff] [review]
Patch, Way 2, Version 3

oops, you're right. My mistake.
Attachment #162141 - Flags: superreview?(roc) → superreview+

Updated

13 years ago
Attachment #162141 - Flags: review?(bienvenu) → review+

Comment 14

13 years ago
is this ready for the aviary branch too?  we should get it in soon if it is.
Flags: blocking-aviary1.0? → blocking-aviary1.0+

Comment 15

13 years ago
Thanks for the patch Ben. I just checked this into the aviary 1.0 branch. 
Flags: blocking-aviary1.0mac?
Keywords: fixed-aviary1.0
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
Attachment #162141 - Flags: approval1.7.x+
Attachment #162141 - Flags: approval-aviary+
Blocks: 256195
(Assignee)

Comment 17

13 years ago
Checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

13 years ago
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).
Keywords: fixed1.7.x
Product: MailNews → Core
now that 1.7.5 and tbird 1.0 are released, can we remove the security flag?
(Assignee)

Comment 20

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

Updated

12 years ago
Blocks: 248511

Comment 21

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

marc
Keywords: fixed1.4.4
Blocks: 256197
No longer blocks: 256195
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.