413874 - Audit mail MIME code for string buffer abuse

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the safe variants of the PL_* string functions are PL_strncpyz and PL_strcatn.

Comment 3

[Daniel Veditz [:dveditz]]

Unfortunately this is too big a task to get done in 1.8.1.12 (code-freeze this weekend).

Comment 4

[Daniel Veditz [:dveditz]]

Window thought it might be good to hire an audit of this code.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Do we have folks who we think would be good to hire?  How can we get more information on cost, timeliness, etc...?

Comment 6

[Samuel Sidler (old account; do not CC)]

How is this coming? Freeze is on Friday. Does this look likely for then or should we push this out to the next release?

Comment 7

[Emre Birol]

Attachment: 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 8

[David Ascher (:davida)]

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?

Comment 9

[David Ascher (:davida)]

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.

Comment 10

[David :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);
     }

Comment 11

[Emre Birol]

>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, "&", 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);

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

[David :Bienvenu]

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

Comment 13

[Emre Birol]

Attachment: 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.

Comment 14

[Samuel Sidler (old account; do not CC)]

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.

Comment 15

[David :Bienvenu]

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

thx, Emre.

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Emre Birol]

Attachment: 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.

Comment 18

[Daniel Veditz [: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)

Comment 19

[Emre Birol]

Attachment: strcpy, strcat, scanf/sscanf patch, rev3

Indeed. Here is the final patch.

Comment 20

[Daniel Veditz [:dveditz]]

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.

Comment 21

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 22

[Mats Palmgren (inactive)]

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

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Thanks Mats, I should have been watching more closely.

Comment 24

[Samuel Sidler (old account; do not CC)]

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

Comment 25

[Emre Birol]

Working on it, going to submit this week.

Comment 26

[Samuel Sidler (old account; do not CC)]

Emre, how is this patch coming? Freeze is tomorrow...

Comment 27

[Emre Birol]

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.

Comment 28

[Emre Birol]

Attachment: strcpy, strcat, scanf/sscanf patch, rev3, branch

Branch version of the same patch.

Comment 29

[Daniel Veditz [: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

Comment 30

[Samuel Sidler (old account; do not CC)]

Gavin said he'd check in the branch patch tonight or tomorrow.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

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

Comment 32

[Al Billings [:abillings - ex-MoCo]]

I have no idea how to verify these changes and I want to run far far away from them.

Comment 33

[Daniel Veditz [:dveditz]]

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.

Comment 34

[Alexander Sack]

Attachment: for 1.8.0

Comment 35

[Alexander Sack]

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

a=asac for 1.8.0