430193 - Audit comi18n.cpp for string buffer abuse

Status: RESOLVED WORKSFORME

(Thunderbird :: Security, defect)

Description

[Emre Birol]

intlmime_encode_b, and and intlmime_encode_q functions in comi18n.cpp should be re-factored to prevent buffer overflows.

Comment 1

[Emre Birol]

Thanks to David Bienvenu for pointing this out in #413874

>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);

Comment 2

[Daniel Veditz [:dveditz]]

It looks like the output buffer was allocated assuming worst-case expansion in apply_rfc2047_encoding() -- at least I think so. Confusing enough it's definitely worth an audit but for now I don't think this is actually exploitable.

Watch out for integer overflows, too.  "in+srclen" might wrap around, although in this case that should result in in > end so we'd skip the loop.

Comment 4

[Mats Palmgren (inactive)]

My comment from bug 432469:

I looked throught the patch in bug 413874 and it seems to me that
buffer overflow is still possible even when using PL_strncpyz().
An example:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/mime/src/comi18n.cpp&rev=1.133&root=/cvsroot&mark=349,352-353#348

'obufsize' is declared as PRInt32.
The buffer size parameter of PL_strncpyz() is declared PRUint32.

Suppose the buffer is near full such that PL_strncpyz() cannot
write the full length of 'encodedword_head'.
We still do "obufsize -= encodedword_headlen" causing 'obufsize'
to become negative.  The next PL_strncpyz() to 'o' will be a
buffer overflow.

Briefly looking through the patch, I think all the places that updates
the remaining buffer length needs to use strlen() to find out how much
was actually copied.

Also, I looked at the code for PL_strncpyz():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/lib/libc/src/strcpy.c&rev=3.8&root=/cvsroot&mark=74-75#69
Silently accepting NULL src/dest hides programming errors.
I'd like a fatal assert there in debug builds.

Comment 5

[Mats Palmgren (inactive)]

Another issue:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/mime/src/nsMsgHeaderParser.cpp&rev=1.66&mark=1548-1550,1557-1559#1536
'outlen' needs to be decremented -1 for each embedded NUL too.

Comment 6

[Brandon Sterne (:bsterne)]

Do we need to spin off separate bugs from comment 4 and comment 5?  Or is there progress being made in fixing those areas of code?

Comment 7

[Daniel Veditz [:dveditz]]

dmose: this should probably block tb3, cc'ing you in case you aren't looking at the "wanted" flags.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Bug is already marked blocking-thunderbird3+.

Comment 9

[Brandon Sterne (:bsterne)]

Mats, can you respond to comment 6?  Do we need spin-off bugs filed?  What needs to be done to close the books on this bug?

Comment 10

[David :Bienvenu]

taking to investigate c#4

Comment 11

[David :Bienvenu]

I believe that since we pre-allocate the buffer to a worst-case size for the expansions, this shouldn't be exploitable, so I'm clearing this from the blocking list. It's still worth leaving this open to really fix the code, though.

Comment 12

[Josh Aas]

(In reply to David :Bienvenu from comment #11)
> I believe that since we pre-allocate the buffer to a worst-case size for the
> expansions, this shouldn't be exploitable

If this isn't exploitable can we open this bug up?

Comment 13

[David :Bienvenu]

yes, opening up.

Comment 14

[Magnus Melin [:mkmelin]]

The functions in question do not exist anymore. https://searchfox.org/comm-central/source/mailnews/mime/src/comi18n.cpp