Last Comment Bug 425152 - heap overflow when canceling usenet message in nsNNTPProtocol::DoCancel()
: heap overflow when canceling usenet message in nsNNTPProtocol::DoCancel()
: fixed1.8.1.17
Product: Thunderbird
Classification: Client Software
Component: Security (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: Thunderbird 3
Assigned To: Joshua Cranmer [:jcranmer]
Depends on:
  Show dependency treegraph
Reported: 2008-03-26 01:15 PDT by georgi - hopefully not receiving bugspam
Modified: 2008-09-28 11:24 PDT (History)
10 users (show)
dmose: blocking‑thunderbird3.0a1+
dveditz: blocking1.8.1.15-
dveditz: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Patch (4.43 KB, patch)
2008-04-14 18:50 PDT, Joshua Cranmer [:jcranmer]
mozilla: review+
mozilla: superreview+
Details | Diff | Splinter Review
Branch patch (4.46 KB, patch)
2008-08-15 15:40 PDT, Joshua Cranmer [:jcranmer]
mozilla: review+
mozilla: superreview+
dmose: approval1.8.1.17+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2008-03-26 01:15:19 PDT
this is not trivial to reproduce, but seems wrong by inspection:
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 2008-04-06 02:56:01 PDT
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 2008-04-06 02:57:39 PDT
not sure [sg:medium?] or [sg:high?]
Comment 3 Daniel Veditz [:dveditz] 2008-04-09 01:54:00 PDT
more ancient mailnews strcpy/strcat abuse.
Comment 4 Dan Mosedale (:dmose) 2008-04-09 09:32:12 PDT
dveditz: why shouldn't this block
Comment 5 Dan Mosedale (:dmose) 2008-04-09 10:03:50 PDT
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 (:dmose) 2008-04-14 15:54:16 PDT
Reassigning to our NNTP guru.
Comment 7 Dan Mosedale (:dmose) 2008-04-14 16:00:18 PDT
We want to protect our alpha/beta users as much as we reasonably can; marking blocking-thunderbird3.0a1+
Comment 8 Joshua Cranmer [:jcranmer] 2008-04-14 18:50:32 PDT
Created attachment 315684 [details] [diff] [review]

This patch turns the other_random_headers into an nsCAutoString.
Comment 9 David :Bienvenu 2008-04-14 19:05:47 PDT
Comment on attachment 315684 [details] [diff] [review]

thx, Joshua
Comment 10 Joshua Cranmer [:jcranmer] 2008-04-15 16:57:46 PDT
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 2008-04-18 00:53:21 PDT
why not change all pl_strcat and pl_strcpy in mailnews/ ?

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

these numbers seem doable
Comment 12 Dan Mosedale (:dmose) 2008-06-04 12:26:42 PDT
Joshua, can you see if this patch applies cleanly to branch, and, if so, request approval?
Comment 13 Samuel Sidler (old account; do not CC) 2008-06-06 09:25:19 PDT
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] 2008-06-06 11:15:52 PDT
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) 2008-08-11 11:30:22 PDT
Any update on a branch patch for this bug?
Comment 16 Joshua Cranmer [:jcranmer] 2008-08-15 15:40:25 PDT
Created attachment 334030 [details] [diff] [review]
Branch patch

Branch version.
Comment 17 David :Bienvenu 2008-08-15 16:24:45 PDT
marking checkin-needed for branch patch
Comment 18 Joshua Cranmer [:jcranmer] 2008-08-15 16:51:23 PDT
/cvsroot/mozilla/mailnews/news/src/nsNNTPProtocol.cpp,v  <--  nsNNTPProtocol.cpp
new revision: 1.373.2.3; previous revision: 1.373.2.2

Checked in on branch
Comment 19 Alexander Sack 2008-08-31 06:22:52 PDT
Comment on attachment 334030 [details] [diff] [review]
Branch patch

a=asac for

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