The default bug view has changed. See this FAQ.

Audit mail MIME code for string buffer abuse

RESOLVED FIXED in mozilla1.9

Status

MailNews Core
MIME
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dveditz, Assigned: Emre Birol)

Tracking

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

unspecified
mozilla1.9
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

9 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

9 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

9 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

9 years ago
Assignee: nobody → dmose
Flags: blocking1.8.1.13? → blocking1.8.1.13+
(Reporter)

Comment 4

9 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...?

Updated

9 years ago
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

9 years ago
Created attachment 307734 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch

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

9 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

9 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

9 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

9 years ago
Created attachment 307886 [details] [diff] [review]
strcpy, strcat, scanf/sscanf path, rev 1

> 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

9 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

9 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

9 years ago
Created attachment 315192 [details] [diff] [review]
strcpy, strcat, scanf/sscanf, rev2

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

9 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

9 years ago
Created attachment 315832 [details] [diff] [review]
 strcpy, strcat, scanf/sscanf patch, rev3   

Indeed. Here is the final patch.
Attachment #315192 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Attachment #315832 - Flags: superreview+
Attachment #315832 - Flags: review?
(Reporter)

Comment 20

9 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
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

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 432469

Updated

9 years ago
No longer depends on: 432469

Updated

9 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

9 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

9 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

9 years ago
Created attachment 324008 [details] [diff] [review]
strcpy, strcat, scanf/sscanf patch, rev3, branch

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

9 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

9 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
Keywords: checkin-needed → fixed1.8.1.15
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

9 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

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

Comment 34

8 years ago
Created attachment 355431 [details] [diff] [review]
for 1.8.0

Updated

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

Comment 35

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

a=asac for 1.8.0

Updated

8 years ago
Flags: blocking1.8.0.next+
Depends on: 505221
You need to log in before you can comment on or make changes to this bug.