Closed Bug 413874 Opened 16 years ago Closed 16 years ago

Audit mail MIME code for string buffer abuse

Categories

(MailNews Core :: MIME, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9

People

(Reporter: dveditz, Assigned: bugmil.ebirol)

References

Details

(Keywords: fixed1.8.1.15, Whiteboard: [sg:low])

Attachments

(3 files, 3 obsolete files)

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?
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.
Unfortunately this is too big a task to get done in 1.8.1.12 (code-freeze this weekend).
Flags: blocking1.8.1.12?
Assignee: nobody → dmose
Flags: blocking1.8.1.13? → blocking1.8.1.13+
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?
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)
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);
     }
>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..
>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.

> 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 on attachment 307886 [details] [diff] [review]
strcpy, strcat, scanf/sscanf path, rev 1

thx, Emre.
Attachment #307886 - Flags: review?(bienvenu) → review+
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-
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)
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+
Indeed. Here is the final patch.
Attachment #315192 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #315832 - Flags: superreview+
Attachment #315832 - Flags: review?
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
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 432469
No longer depends on: 432469
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...
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...
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.
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]
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+
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.
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]
Group: security
Product: Core → MailNews Core
Flags: blocking-thunderbird3?
Attached patch for 1.8.0Splinter Review
Attachment #324008 - Flags: approval1.8.0.next+
Comment on attachment 324008 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch, rev3, branch

a=asac for 1.8.0
Flags: blocking1.8.0.next+
Depends on: 505221
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: