425152 - heap overflow when canceling usenet message in nsNNTPProtocol::DoCancel()

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

this is not trivial to reproduce, but seems wrong by inspection:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/news/src/nsNNTPProtocol.cpp&rev=1.396
4255 mscott       1.1     L = PL_strlen (id);
...
4258                      other_random_headers = (char *) PR_Malloc (L + 20);
...
4339                      PL_strcpy (other_random_headers, "Control: cancel ");
4340                      PL_strcat (other_random_headers, id);
4341                      PL_strcat (other_random_headers, CRLF);

4342 sspitzer     1.73    if (distribution) {

4343 bienvenu     1.348     PL_strcat (other_random_headers, "Distribution: ");

4344 scott        1.386     PL_strcat (other_random_headers, distribution);
4345                        PL_strcat (other_random_headers, CRLF);


so |other_random_headers| is allocated strlen(id) + 20.
later |id| + stuff is copied.

later |distribition| may be appended yet distribution doesn't contribute to
the allocation size and this may lead to overflow.

even "Control: cancel Distribution: " is longer than 20

Comment 1

[georgi - hopefully not receiving bugspam]

this is a real classical overflow.

debugging is consistent with comment #0.
the crash is scary:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000598174 in XPCWrappedNative::HasProto (this=0x5353535353535353)
    at /opt/joro/thunderbird-cvs/mozilla/js/src/xpconnect/src/xpcprivate.h:2124
2124        HasProto() const {return !IsTaggedScope(mMaybeScope);}


i reproduced it this way:
set up nntp server (not very trivial).
post a message.
on the server edit the message adding header:
Distributions: 1024chars

cancel the message and watch the crash.

potential exploit scenario is spoofing user's email, sending a nasty message with the header, wait for the user to cancel the message.

Comment 2

[georgi - hopefully not receiving bugspam]

not sure [sg:medium?] or [sg:high?]

Comment 3

[Daniel Veditz [:dveditz]]

more ancient mailnews strcpy/strcat abuse.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

dveditz: why shouldn't this block 1.8.0.14?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

ss explained to me in more details how the branches are generally run, so I withdraw the question here.  Thanks, ss!

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Reassigning to our NNTP guru.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

We want to protect our alpha/beta users as much as we reasonably can; marking blocking-thunderbird3.0a1+

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Patch

This patch turns the other_random_headers into an nsCAutoString.

Comment 9

[David :Bienvenu]

Comment on attachment 315684 [details] [diff] [review]
Patch

thx, Joshua

Comment 10

[Joshua Cranmer [:jcranmer]]

Checked in by timeless:

/cvsroot/mozilla/mailnews/news/src/nsNNTPProtocol.cpp,v <-- nsNNTPProtocol.cpp
new revision: 1.398; previous revision: 1.397

Comment 11

[georgi - hopefully not receiving bugspam]

why not change all pl_strcat and pl_strcpy in mailnews/ ?

according to grep:
mozilla/mailnews$ grep -rniI 'pl_strcpy' * | wc -l
38
mozilla/mailnews$ grep -rniI 'pl_strcat' * | wc -l
80

these numbers seem doable

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Joshua, can you see if this patch applies cleanly to branch, and, if so, request 1.8.1.15 approval?

Comment 13

[Samuel Sidler (old account; do not CC)]

We're freezing tonight at 11:59pm PDT. Joshua, are you going to be able to get to a branch patch for this? (Or testing the trunk one to see if it applies...)

Comment 14

[Daniel Veditz [:dveditz]]

Guess it's getting too late to continue "blocking" on this for the next Thunderbird update. If a branch patch gets written and reviewed today ping drivers on IRC for approval otherwise "next time".

Comment 15

[Samuel Sidler (old account; do not CC)]

Any update on a branch patch for this bug?

Comment 16

[Joshua Cranmer [:jcranmer]]

Attachment: Branch patch

Branch version.

Comment 17

[David :Bienvenu]

marking checkin-needed for branch patch

Comment 18

[Joshua Cranmer [:jcranmer]]

/cvsroot/mozilla/mailnews/news/src/nsNNTPProtocol.cpp,v  <--  nsNNTPProtocol.cpp
new revision: 1.373.2.3; previous revision: 1.373.2.2
done

Checked in on branch

Comment 19

[Alexander Sack]

Comment on attachment 334030 [details] [diff] [review]
Branch patch

a=asac for 1.8.0.15