Last Comment Bug 258005 - heap overflows triggered by "send page"
: heap overflows triggered by "send page"
Status: VERIFIED FIXED
[sg:fix] fixed1.7.3
: fixed-aviary1.0, fixed1.7.5
Product: MailNews Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Scott MacGregor
:
Mentors:
Depends on:
Blocks: sbb+
  Show dependency treegraph
 
Reported: 2004-09-04 05:53 PDT by georgi - hopefully not receiving bugspam
Modified: 2010-01-23 14:28 PST (History)
12 users (show)
chofmann: blocking1.7.5+
chofmann: blocking‑aviary1.0PR+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
/cgi-bin/long2.pl (132 bytes, application/x-perl)
2004-09-04 05:56 PDT, georgi - hopefully not receiving bugspam
no flags Details
/cgi-bin/long.pl (136 bytes, text/plain)
2004-09-04 05:58 PDT, georgi - hopefully not receiving bugspam
no flags Details
html testcase - click on the link, choose "send page", email it (4.02 KB, text/html)
2004-09-05 02:30 PDT, georgi - hopefully not receiving bugspam
no flags Details
Proposed patch - changes PL_strcpy with nsCAutoString (8.13 KB, patch)
2004-09-05 02:48 PDT, georgi - hopefully not receiving bugspam
no flags Details | Diff | Review
updated version of the fix (8.40 KB, patch)
2004-09-07 15:34 PDT, Scott MacGregor
mozilla: superreview+
Details | Diff | Review

Description georgi - hopefully not receiving bugspam 2004-09-04 05:53:16 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040115
Build Identifier: Mozilla/5.0

there are heap based buffer overflows triggered by "send page" and then sending
the email. the overflow is at least in 1.7 and nightly from 3.sep.

the fucked code is in
nsMsgCompUtils.cpp
mime_generate_attachment_headers

one of the overflows is triggered by:
      if (*s == ' ')
        PUSH_STRING("%20");
one char consumes 3 bytes of memory, while only 2 are allocated.
to reproduce try to send as page "data:text/html;,A_LOT_OF_SPACES.


the other buffer overflow is triggered by sending long http urls containing ' ',
not clear which code fucks it.

btw, the macro PUSH_STRING is quite unsafe and i suggest it checks whether there
is enough space left.





Reproducible: Always
Steps to Reproduce:
will attach testcases.
Actual Results:  
the heap is fucked up

Expected Results:  
the heap is not fucked up
Comment 1 georgi - hopefully not receiving bugspam 2004-09-04 05:56:34 PDT
Created attachment 157874 [details]
/cgi-bin/long2.pl

put in cgi-bin, access it, click on the link then choose "send page" and send
the email.
Comment 2 georgi - hopefully not receiving bugspam 2004-09-04 05:58:22 PDT
Created attachment 157875 [details]
/cgi-bin/long.pl

put in cgi-bin, access it, put web server on localhost:1234 which should serve
anything, send the page as email.
Comment 3 Brendan Eich [:brendan] 2004-09-04 13:11:49 PDT
mscott, can you take a look?

/be
Comment 4 georgi - hopefully not receiving bugspam 2004-09-05 02:30:43 PDT
Created attachment 157933 [details]
html testcase - click on the link, choose "send page", email it
Comment 5 georgi - hopefully not receiving bugspam 2004-09-05 02:48:20 PDT
Created attachment 157934 [details] [diff] [review]
Proposed patch - changes PL_strcpy with nsCAutoString

bug 257314 was fixed by changing PL_strcpy with nsCAutoString, so this patch
does the same.

the risk from regression is low, the performance hit is minimal.

the only potential problem may be memory leak in this:
return PL_strdup(buf.get());

couldn't find the definition of nsCAutoString
Comment 6 Scott MacGregor 2004-09-07 15:34:34 PDT
Created attachment 158152 [details] [diff] [review]
updated version of the fix

I took Georgi's original patch and did the following:

1) removed the tabs
2) changed the nsCAutoString to an nsCString
3) I also did a review on the code to make sure each .Append call had the same
arguments as the old push string call.
4) Note that many (but not all!) of these changes are inside of a
GENERATE_CONTENT_BASE ifdef which we don't defined during the build so some of
it isn't actually executed.

There are still more cases of PUSH_STRING that could probably be converted over
to using the string APIs in this file.
Comment 7 Scott MacGregor 2004-09-07 15:36:05 PDT
Comment on attachment 158152 [details] [diff] [review]
updated version of the fix

See:

http://bugzilla.mozilla.org/show_bug.cgi?id=258005#c6

for details
Comment 8 Scott MacGregor 2004-09-07 22:36:30 PDT
fixed on the aviary 1.0 branch and the trunk. We should probably let this bake
for a couple days before landing for 1.7.x?
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-08 04:55:01 PDT
+  nsCString buf("");

no need for the "" argument, strings are empty by default
Comment 10 Daniel Veditz [:dveditz] 2004-09-08 09:01:12 PDT
If the string-expanding PUSH_STRING("%20") is inside the ifdef, then the buffer
overrun must be somewhere else. Does this really fix it?
Comment 11 Daniel Veditz [:dveditz] 2004-09-08 12:31:42 PDT
Checked "updated version" into the 1.7.2 branch

Fixes the html testcase linked above, can't test the cgi-bin testcases right now.

I'm concerned similar PUSH_STRING() problems lurk in mime_generate_headers:
>  /* Add a bunch of slop for the static parts of the headers. */
>  /* size += 2048; */
>  size += 2560;

If the "slop" had to be increased once there's no guarantee we haven't added new
headers since. Or that it was even right to begin with.
Comment 12 georgi - hopefully not receiving bugspam 2004-09-08 13:01:52 PDT
dveditz: i agree that other occurences of PUSH_STRING should be fixed also in
the same simple way.
per your previous comment, i believe the c++ stuff fixes the overflow in both
testcases as evidence suggests.

btw, are there any docs which explain the methods and usage of basic NS* classes?
Comment 13 Ben Bucksch (:BenB) 2004-09-08 16:34:50 PDT
> are there any docs which explain the methods and usage of basic NS* classes?

For nsString
<http://www.mozilla.org/projects/xpcom/string-quickref.html>
<http://www.mozilla.org/projects/xpcom/string-guide.html>
but they may be outdated and incomplete.
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2004-09-12 16:38:41 PDT
at least that second link is pretty up-to-date.
Comment 15 Robin Lu 2004-11-05 00:45:01 PST
(In reply to comment #11)
> Checked "updated version" into the 1.7.2 branch
> 
I found this patch is not in 1.7 branch. Is there anything wrong?
Comment 16 Robin Lu 2004-11-07 19:02:53 PST
I found this patch is in MOZILLA_1_7_3_RELEASE branch but not in
MOZILLA_1_7_BRANCH. Should we check it into MOZILLA_1_7_BRANCH so that we can
have it in mozilla 1.7.4 and later?
Comment 17 Mike Kaply [:mkaply] (Out June 27-July 5) 2004-11-09 05:59:38 PST
Yes, please put this on the 1.7 branch.
Comment 18 OstGote! 2004-12-15 03:39:09 PST
It is still not on the MOZILLA_1_7_BRANCH for 1.7.5.
Comment 19 Marcia Knous [:marcia - use ni] 2004-12-16 14:08:16 PST
Both Tracy and I tested the html testcase using 1.75 Windows 20041216 - I tested
using 1.75 Mac 20041216-09. verifying fixed after discussion with dveditz.
Comment 20 Daniel Veditz [:dveditz] 2004-12-23 17:10:38 PST
(note: fix checked into 1.7 branch by mkaply on 12/15)

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