Audit mail MIME code for string buffer abuse

RESOLVED FIXED in mozilla1.9

Status

defect
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: dveditz, Assigned: bugmil.ebirol)

Tracking

(Depends on 1 bug, {fixed1.8.1.15})

Dependency tree / graph
Bug Flags:
blocking1.8.1.13 -
blocking1.8.1.15 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

12 years ago
As seen in bug 412363 the mailnews MIME code is ancient--on the 1.8 branch lots of it is revision 1.1 from March 1999!--and uses lots of strcpy/strcat and sometimes doesn't get the buffer sizes right (see also sg:critical bug 362213 fixed in Thunderbird 1.5.0.9/2).

This all needs to get audited and at the very least converted to the PL_strncpy/PL_strncat forms, and possibly for not much more work converted to use our safer string classes.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Flags: blocking-thunderbird3?
Reporter

Comment 1

12 years ago
until proven otherwise I'll assume there's another sg:critical problem hiding in this code like the previously mentioned bug 362213 and bug 412363
Whiteboard: [sg:critical?]
I think the safe variants of the PL_* string functions are PL_strncpyz and PL_strcatn.
Reporter

Comment 3

12 years ago
Unfortunately this is too big a task to get done in 1.8.1.12 (code-freeze this weekend).
Flags: blocking1.8.1.12?
Reporter

Updated

12 years ago
Assignee: nobody → dmose
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Reporter

Comment 4

12 years ago
Window thought it might be good to hire an audit of this code.
Do we have folks who we think would be good to hire?  How can we get more information on cost, timeliness, etc...?
Assignee: dmose → ebirol
How is this coming? Freeze is on Friday. Does this look likely for then or should we push this out to the next release?
Assignee

Comment 7

11 years ago
I have investigated libmime for strcpy, strcat, scanf/sscanf usage, and replaced them with their safer counterparts if necessary. Some of the changes are not critical but I did change it anyway for consistency.

This work doesn't cover external MACROs and functions that use strcpy, strcat and sscanf internally, just the modules in mime directory.

I have randomly tested my changes with Debug and Release version 3.0a1pre (2008030521). I like to emphasize the word randomly one more time since I couldn't be able to cover every functions I have changed. Simply because I
couldn't find a scenario that calls the function.

Couple things I have noticed:
 - some of the pointers passed to strcpy are offsets and moving in the actual buffer. It makes difficult to adjust available buffer size accordingly.
 - in some cases there is no way to make sure that the passed value (2nd param to strcpy) is NULL terminated.
 - there are lots of Magic numbers, lots!
 - strlen() is used alot. It's only good if we are sure that the passed string is NULL terminated.

I have deliberately left functions like gets, sprintf, printf (and variances) to another cycle.

IMHO before discussing a rewrite, replacement or further modification, we should come up with a complete test plan/framework for libmime where we can run irregular MIME file against the library to identify performance, security
and memory leak issues. This would also help us to benchmark any candidate for replacement.
Comment on attachment 307734 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch

dmose, don't know if you have the time for this this week -- if not, maybe check w/ bienvenu?
Attachment #307734 - Flags: superreview?(dveditz)
Attachment #307734 - Flags: review?(dmose)
Comment on attachment 307734 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch

Forgot that mime is its own module, and dmose isn't a peer there.  Bienvenu, I hope you have the time -- dveditz is trying to land it this week.
Attachment #307734 - Flags: review?(dmose) → review?(bienvenu)

Comment 10

11 years ago
ugh, libmime makes my head hurt.

In comi18n.cpp, we protect overflowing the output buffer everywhere but here, right? Those functions are internal to libmime.

          if (method == 'B')
             enclen = intlmime_encode_b((const unsigned char *)ibuf, strlen(ibuf), o);
           else
             enclen = intlmime_encode_q((const unsigned char *)ibuf, strlen(ibuf), o);
  

In theory, we don't need that last statement, since the PL routines should always leave us with a null terminated string?

+  PL_strcatn(result, resultLen, imappart);
+  PL_strcatn(result, resultLen, "?part=");
+  PL_strcatn(result, resultLen, libmimepart);
   result[strlen(result)] = 0;

calling strlen multiple times seems wasteful here (and several other places):

+      PL_strncpyz (out, "&", outlen); outlen -= strlen(out); out += strlen (out);
     }
     else if (*this_start == '>')
     {
-      PL_strcpy (out, ">"); out += strlen (out);
+      PL_strncpyz (out, ">", outlen); outlen -= strlen(out); out += strlen (out);
     }
     else if (enriched_p &&
          this_start < data_end + 1 &&
          this_start[0] == '<' &&
          this_start[1] == '<')
     {
-      PL_strcpy (out, "&lt;"); out += strlen (out);
+      PL_strncpyz (out, "&lt;", outlen); outlen -= strlen(out); out += strlen (out);
     }
Assignee

Comment 11

11 years ago
>In comi18n.cpp, we protect overflowing the output buffer everywhere but here,
>right? Those functions are internal to libmime.

Correct, we don't. I have only changed strcpy, strcat, scanf/sscanf functions. My intention is to limit this patch with infamous function removal. This one needs us to change the signature of at least 3 functions -- to pass the  output buffer length. Do you suggest to add this change too? 

>In theory, we don't need that last statement, since the PL routines should
>always leave us with a null terminated string?

>+  PL_strcatn(result, resultLen, imappart);
>+  PL_strcatn(result, resultLen, "?part=");
>+  PL_strcatn(result, resultLen, libmimepart);
>   result[strlen(result)] = 0;

Not all PL functions do, but PL_strcatn definitely does. You are right, last line is redundant.

>calling strlen multiple times seems wasteful here (and several other places):

>+      PL_strncpyz (out, "&amp;", outlen); outlen -= strlen(out); out += strlen
>(out);
>     }
>     else if (*this_start == '>')
>     {
>-      PL_strcpy (out, "&gt;"); out += strlen (out);
>+      PL_strncpyz (out, "&gt;", outlen); outlen -= strlen(out); out += strlen
>(out);

In this particular code snippet, since outlen and strlen(out) not the same, alternative to not calling strlen() multiple times would be to call it once, store the value in a variable, and use it second time. Do I understand your suggestion correctly?

I am going to check other strlen() usages at other places..

Thanks for the review..

Comment 12

11 years ago
>In this particular code snippet, since outlen and strlen(out) not the same,
>alternative to not calling strlen() multiple times would be to call it once,
>store the value in a variable, and use it second time. Do I understand your
>suggestion correctly?

yes, that's what I was thinking. It's a pain, I know...

>Correct, we don't. I have only changed strcpy, strcat, scanf/sscanf functions.
>My intention is to limit this patch with infamous function removal. This one
>needs us to change the signature of at least 3 functions -- to pass the  output
>buffer length. Do you suggest to add this change too? 

If that's the purpose of this patch, then what you've done is fine. My feeling though, is that, if that routine was vulnerable before, it might still be vulnerable. But I imagine this is just the first step along the way.

Assignee

Comment 13

11 years ago
> yes, that's what I was thinking.

done.

> My feeling though, is that, if that routine was vulnerable before, it might still be vulnerable. But I imagine this is just the first step along the way.

Agreed. I think it is better for us to apply patches (relatively small, and focused) iteratively and monitor the result carefully until we have good test cases for libmime.
Attachment #307734 - Attachment is obsolete: true
Attachment #307886 - Flags: superreview?(dveditz)
Attachment #307886 - Flags: review?(bienvenu)
Attachment #307734 - Flags: superreview?(dveditz)
Attachment #307734 - Flags: review?(bienvenu)
Since code freeze is tonight, we're going to punt on this for this release. That said, we want to get it in as soon as possible for 1.8.1.14 so it can get lots of testing.
Flags: blocking1.8.1.14+
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+

Comment 15

11 years ago
Comment on attachment 307886 [details] [diff] [review]
strcpy, strcat, scanf/sscanf path, rev 1

thx, Emre.
Attachment #307886 - Flags: review?(bienvenu) → review+
Reporter

Comment 16

11 years ago
Comment on attachment 307886 [details] [diff] [review]
strcpy, strcat, scanf/sscanf path, rev 1

>@@ -671,17 +678,17 @@ char * apply_rfc2047_encoding(const char
>-          strcpy(outputtail, ", ");
>+          PL_strcpy(outputtail, ", ");

Needs to be PL_strncpyz(), checking against outputlen it looks like

>@@ -1527,33 +1527,39 @@ msg_remove_duplicate_addresses(const cha
>+    PL_strncpy(out, a_array3[i], outlen);
...
>+    PL_strncpy(out, n_array3[i], outlen);

I'd prefer PL_strncpyz's here because it's immediately followed by strlen(). Just in case the length calc above is wrong or goes wrong in the future (which is the sort of bug we're trying to protect against).

Looks good otherwise. Since you need someone else to check in for you we need a corrected patch attached to the bug so technically an r-, but the re-review will go fast if that's all you change.
Attachment #307886 - Flags: superreview?(dveditz) → superreview-
Assignee

Comment 17

11 years ago
Good ideas, all applied. Additionally I had to change the location of a variable declaration (addlen) in the source which doesn't change the logic or the flow.

thanks for the review.
Attachment #307886 - Attachment is obsolete: true
Attachment #315192 - Flags: superreview?(dveditz)
Reporter

Comment 18

11 years ago
Comment on attachment 315192 [details] [diff] [review]
strcpy, strcat, scanf/sscanf, rev2

>-    PL_strcpy(out, n_array3[i]);
>+    PL_strncpy(out, n_array3[i], outlen);

You missed making this one PL_strncpyz().

Would like a new version for check-in ease, but if you want to work it out with dmose and he agrees to make the change on checkin I'm fine with that.

sr=dveditz (which you can carry over to an updated version if you make the above change)
Attachment #315192 - Flags: superreview?(dveditz) → superreview+
Assignee

Comment 19

11 years ago
Indeed. Here is the final patch.
Attachment #315192 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Keywords: checkin-needed
Assignee

Updated

11 years ago
Attachment #315832 - Flags: superreview+
Attachment #315832 - Flags: review?
Reporter

Comment 20

11 years ago
Comment on attachment 315832 [details] [diff] [review]
 strcpy, strcat, scanf/sscanf patch, rev3   

You generally don't need re-review for minor changes prompted by a super-review. For larger changes of course it can be a good idea.
Attachment #315832 - Flags: review? → review+
Checked in on the trunk:

mozilla/mailnews/mime/src/comi18n.cpp 	1.133
mozilla/mailnews/mime/src/mimedrft.cpp 	1.163
mozilla/mailnews/mime/src/mimeeobj.cpp 	1.22
mozilla/mailnews/mime/src/mimefilt.cpp 	1.21
mozilla/mailnews/mime/src/mimei.cpp 	1.89
mozilla/mailnews/mime/src/mimemoz2.cpp 	1.235
mozilla/mailnews/mime/src/mimemsg.cpp 	1.61
mozilla/mailnews/mime/src/mimethtm.cpp 	1.35
mozilla/mailnews/mime/src/mimetric.cpp 	1.19
mozilla/mailnews/mime/src/mimeunty.cpp 	1.19
mozilla/mailnews/mime/src/nsMsgHeaderParser.cpp 	1.65

Does that make this FIXED?
Keywords: checkin-needed

Comment 22

11 years ago
Bustage fix:
mozilla/mailnews/mime/src/nsMsgHeaderParser.cpp 	1.66 

The error was:
nsMsgHeaderParser.cpp:1562: error: jump to label 'FAIL'
nsMsgHeaderParser.cpp:1475: error:   from here
nsMsgHeaderParser.cpp:1536: error:   crosses initialization of 'PRUint32 outlen'
etc...
Thanks Mats, I should have been watching more closely.
Assignee

Updated

11 years ago
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 432469

Updated

11 years ago
No longer depends on: 432469

Updated

11 years ago
Depends on: 430193
Comment on attachment 315832 [details] [diff] [review]
 strcpy, strcat, scanf/sscanf patch, rev3   

Does this patch apply cleaning on the branch? We need a branch patch here...
Assignee

Comment 25

11 years ago
Working on it, going to submit this week.
Whiteboard: [sg:critical?] → [sg:critical?] need branch patch
Emre, how is this patch coming? Freeze is tomorrow...
Assignee

Comment 27

11 years ago
Hmm, I was planning to submit tomorrow, but in this case I am going to try to finish today. will let you know how it goes before tomorrow.
Assignee

Comment 28

11 years ago
Branch version of the same patch.
Attachment #324008 - Flags: superreview?(dveditz)
Attachment #324008 - Flags: review?(dveditz)
Whiteboard: [sg:critical?] need branch patch → [sg:critical?][needs r/sr dveditz]
Reporter

Comment 29

11 years ago
Comment on attachment 324008 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch, rev3, branch

r/sr=dveditz
Approved for 1.8.1.15, a=dveditz for release-drivers
Attachment #324008 - Flags: superreview?(dveditz)
Attachment #324008 - Flags: superreview+
Attachment #324008 - Flags: review?(dveditz)
Attachment #324008 - Flags: review+
Attachment #324008 - Flags: approval1.8.1.15+
Reporter

Updated

11 years ago
Whiteboard: [sg:critical?][needs r/sr dveditz] → [sg:critical?]
Gavin said he'd check in the branch patch tonight or tomorrow.
Keywords: checkin-needed
mozilla/mailnews/mime/src/comi18n.cpp 	1.122.12.4
mozilla/mailnews/mime/src/mimedrft.cpp 	1.142.2.9
mozilla/mailnews/mime/src/mimeeobj.cpp 	1.18.28.1
mozilla/mailnews/mime/src/mimefilt.cpp 	1.18.28.1
mozilla/mailnews/mime/src/mimei.cpp 	1.77.4.6
mozilla/mailnews/mime/src/mimemoz2.cpp 	1.217.8.1
mozilla/mailnews/mime/src/mimemsg.cpp 	1.55.28.2
mozilla/mailnews/mime/src/mimethtm.cpp 	1.31.28.1
mozilla/mailnews/mime/src/mimetric.cpp 	1.15.28.2
mozilla/mailnews/mime/src/mimeunty.cpp 	1.15.28.2
mozilla/mailnews/mime/src/nsMsgHeaderParser.cpp 	1.57.20.2
Target Milestone: --- → mozilla1.9
I have no idea how to verify these changes and I want to run far far away from them.
Reporter

Comment 33

11 years ago
Lowering severity to low: looks like the remaining buffer lengths were accurate as far as I can tell, but this will prevent critical problems like bug 412323 that crop up when people modify the code and miss bits.
Whiteboard: [sg:critical?] → [sg:low]
Reporter

Updated

11 years ago
Group: security
Product: Core → MailNews Core
Flags: blocking-thunderbird3?

Comment 34

11 years ago
Posted patch for 1.8.0Splinter Review

Updated

11 years ago
Attachment #324008 - Flags: approval1.8.0.next+

Comment 35

11 years ago
Comment on attachment 324008 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch, rev3, branch

a=asac for 1.8.0

Updated

11 years ago
Flags: blocking1.8.0.next+

Updated

10 years ago
Depends on: 505221
You need to log in before you can comment on or make changes to this bug.