Closed
Bug 425152
Opened 16 years ago
Closed 16 years ago
heap overflow when canceling usenet message in nsNNTPProtocol::DoCancel()
Categories
(Thunderbird :: Security, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3
People
(Reporter: guninski, Assigned: jcranmer)
Details
(Keywords: fixed1.8.1.17, Whiteboard: [sg:critical?])
Attachments
(2 files)
4.43 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
4.46 KB,
patch
|
Bienvenu
:
review+
Bienvenu
:
superreview+
dmosedale
:
approval1.8.1.17+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•16 years ago
|
Whiteboard: [sg:investigate]
Reporter | ||
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
not sure [sg:medium?] or [sg:high?]
Whiteboard: [sg:investigate] → [sg:medium?] [sg:high?]
Reporter | ||
Updated•16 years ago
|
Summary: potential heap overflow in nsNNTPProtocol::DoCancel() → heap overflow when canceling usenet message in nsNNTPProtocol::DoCancel()
Comment 3•16 years ago
|
||
more ancient mailnews strcpy/strcat abuse.
Assignee: dveditz → dmose
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?
Flags: blocking-thunderbird3?
Whiteboard: [sg:medium?] [sg:high?] → [sg:critical?]
Comment 4•16 years ago
|
||
dveditz: why shouldn't this block 1.8.0.14?
Comment 5•16 years ago
|
||
ss explained to me in more details how the branches are generally run, so I withdraw the question here. Thanks, ss!
Comment 7•16 years ago
|
||
We want to protect our alpha/beta users as much as we reasonably can; marking blocking-thunderbird3.0a1+
Flags: blocking-thunderbird3? → blocking-thunderbird3.0a1+
Assignee | ||
Comment 8•16 years ago
|
||
This patch turns the other_random_headers into an nsCAutoString.
Attachment #315684 -
Flags: superreview?(bienvenu)
Attachment #315684 -
Flags: review?(bienvenu)
Comment 9•16 years ago
|
||
Comment on attachment 315684 [details] [diff] [review] Patch thx, Joshua
Attachment #315684 -
Flags: superreview?(bienvenu)
Attachment #315684 -
Flags: superreview+
Attachment #315684 -
Flags: review?(bienvenu)
Attachment #315684 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Keywords: checkin-needed
Assignee | ||
Comment 10•16 years ago
|
||
Checked in by timeless: /cvsroot/mozilla/mailnews/news/src/nsNNTPProtocol.cpp,v <-- nsNNTPProtocol.cpp new revision: 1.398; previous revision: 1.397
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3
Reporter | ||
Comment 11•16 years ago
|
||
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
Updated•16 years ago
|
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Comment 12•16 years ago
|
||
Joshua, can you see if this patch applies cleanly to branch, and, if so, request 1.8.1.15 approval?
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?][needs branch patch]
Comment 13•16 years ago
|
||
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•16 years ago
|
||
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".
Flags: blocking1.8.1.16?
Flags: blocking1.8.1.15-
Flags: blocking1.8.1.15+
Updated•16 years ago
|
Flags: blocking1.8.1.17? → blocking1.8.1.17+
Comment 15•16 years ago
|
||
Any update on a branch patch for this bug?
Assignee | ||
Comment 16•16 years ago
|
||
Branch version.
Attachment #334030 -
Flags: superreview?(bienvenu)
Attachment #334030 -
Flags: review?(bienvenu)
Attachment #334030 -
Flags: approval1.8.1.17?
Updated•16 years ago
|
Attachment #334030 -
Flags: superreview?(bienvenu)
Attachment #334030 -
Flags: superreview+
Attachment #334030 -
Flags: review?(bienvenu)
Attachment #334030 -
Flags: review+
Comment 17•16 years ago
|
||
marking checkin-needed for branch patch
Keywords: checkin-needed
Whiteboard: [sg:critical?][needs branch patch] → [sg:critical?]
Updated•16 years ago
|
Attachment #334030 -
Flags: approval1.8.1.17? → approval1.8.1.17+
Assignee | ||
Comment 18•16 years ago
|
||
/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
Keywords: checkin-needed → fixed1.8.1.17
Comment 19•16 years ago
|
||
Comment on attachment 334030 [details] [diff] [review] Branch patch a=asac for 1.8.0.15
Attachment #334030 -
Flags: approval1.8.0.15+
Updated•16 years ago
|
Flags: blocking1.8.0.15+
Updated•15 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•