1611105 - (CVE-2020-6795) TB crashes during opening s/mime signed mail from mailman mailinglist

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Stephan Lauffer]

Attachment: TB-Crash-Fwd_attachment_both_signed.eml

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.130 Safari/537.36

Steps to reproduce:

Symphtom:

Forward a S/MIME signed mail to a mailman mailinglist AND signe it with S/MIME again and send the forwarded mail as attachment. Now open the mail and Thunderbird crashes.

I don't know if this problem causes other security risks. Unless you have checked this I would mark this problem as harmful security problem. Feel free to "unckeck" this and open this bug for world access/read.

Affected TB versions round about above v60. TB crashes on Linux and Windows, too. Other architectures not tested.

We have tested the Horde Webgroupware and Thunderbird as sending/composing MUA but makes no different.

Fast test to reproduce:

Open the attached eml file. :-/

Workarounds:

1st: Do not signe the fwd mail to the list OR
2nd: Do not send the fwd mail as attachment

Actual results:

TB crashes during opening the mail.

Expected results:

TB should show/open the mail and not crash... (:

Comment 1

[Kai Engert [:KaiE:]]

Thanks a lot for your report, confirming the crash with the provided attachment on latest TB 68.x

Comment 2

[Kai Engert [:KaiE:]]

We crash with a null pointer dereference, so the crash probably isn't exploitable.

The crash is in MimeObject_write, trying to look at obj->options->state->first_data_written_p, but obj->options->state is NULL.
Call stack is:
MimeObject_write
MimeInlineTextPlain_parse_eof
MimeInlineText_finalize
mime_free

I think this means we have already processed/cleaned up this MIME object earlier, because the only place that clears obj->options->state is in MimeObject_finalize.

Then I saw we're running into two assertions:
once: [26617, Main Thread] ###!!! ASSERTION: shouldn't already have hdrs for multipart: '!mult->hdrs', file /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp, line 584
three times: [26617, Main Thread] ###!!! ASSERTION: should only have one child: 'cont->nchildren == 1', file /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp, line 649

Those assertions are in MimeMultipartSigned_emit_child.
I see that we execute that function twice for the same MIME object, which probably shouldn't happen.

The call stack of those two calls are:

#0  0x00007fffea9d3543 in MimeMultipartSigned_emit_child(MimeObject*) (obj=0x7fffd95b8660) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp:584
#1  0x00007fffea9d2d75 in MimeMultipartSigned_parse_eof(MimeObject*, bool) (obj=0x7fffd95b8660, abort_p=false) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp:121
#2  0x00007fffea9d5e94 in MimeMultipart_close_child(MimeObject*) (object=0x7fffd5d03f00) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemult.cpp:508
#3  0x00007fffea9d44b2 in MimeMultipart_parse_line(char const*, int, MimeObject*) (line=0x7fffd5f2d800 "--", '=' <repeats 15 times>, "5250413269614387544==\n", length=39, obj=0x7fffd5d03f00) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemult.cpp:141
#4  0x00007fffea9a131a in convert_and_send_buffer(char*, int, bool, int (*)(char*, unsigned int, void*), void*) (buf=0x7fffd5f2d800 "--", '=' <repeats 15 times>, "5250413269614387544==\n", length=39, convert_newlines_p=true, per_line_fn=
    0x7fffea9d4250 <MimeMultipart_parse_line(char const*, int, MimeObject*)>, closure=0x7fffd5d03f00) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimebuf.cpp:133
#5  0x00007fffea9a10e4 in mime_LineBuffer(char const*, int32_t, char**, int32_t*, uint32_t*, bool, int32_t (*)(char*, uint32_t, void*), void*)
    (net_buffer=0x7fffd9f37c00 "--", '=' <repeats 15 times>, "5250413269614387544==\n\n", net_buffer_size=39, bufferP=0x7fffd5d03f38, buffer_sizeP=0x7fffd5d03f48, buffer_fpP=0x7fffd5d03f50, convert_newlines_p=true, per_line_fn=
    0x7fffea9d4250 <MimeMultipart_parse_line(char const*, int, MimeObject*)>, closure=0x7fffd5d03f00) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimebuf.cpp:207
#6  0x00007fffea9d6ed6 in MimeObject_parse_buffer(char const*, int, MimeObject*) (buffer=0x7fffd9f37c00 "--", '=' <repeats 15 times>, "5250413269614387544==\n\n", size=39, obj=0x7fffd5d03f00) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimeobj.cpp:223
#7  0x00007fffea9d0050 in MimeMessage_parse_line(char const*, int, MimeObject*) (aLine=0x7fffd9f37c00 "--", '=' <repeats 15 times>, "5250413269614387544==\n\n", aLength=39, obj=0x7fffd5cb6400) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsg.cpp:180
#8  0x00007fffea9a131a in convert_and_send_buffer(char*, int, bool, int (*)(char*, unsigned int, void*), void*) (buf=0x7fffd9f37c00 "--", '=' <repeats 15 times>, "5250413269614387544==\n\n", length=39, convert_newlines_p=true, per_line_fn=
    0x7fffea9cfc50 <MimeMessage_parse_line(char const*, int, MimeObject*)>, closure=0x7fffd5cb6400) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimebuf.cpp:133
#9  0x00007fffea9a10e4 in mime_LineBuffer(char const*, int32_t, char**, int32_t*, uint32_t*, bool, int32_t (*)(char*, uint32_t, void*), void*)
    (net_buffer=0x7fffd55b1e64 "--", '=' <repeats 15 times>, "5250413269614387544==\r\nContent-Type: text/plain; charset=\"iso-8859-1\"\r\nMIME-Version: 1.0\r\nContent-Transfer-Encoding: quoted-printable\r\nContent-Disposition: inline\r\n\r\n", '_' <repeats 17 times>..., net_buffer_size=374, bufferP=0x7fffd5cb6438, buffer_sizeP=0x7fffd5cb6448, buffer_fpP=0x7fffd5cb6450, convert_newlines_p=true, per_line_fn=0x7fffea9cfc50 <MimeMessage_parse_line(char const*, int, MimeObject*)>, closure=0x7fffd5cb6400)
    at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimebuf.cpp:207
#10 0x00007fffea9d6ed6 in MimeObject_parse_buffer(char const*, int, MimeObject*)
    (buffer=0x7fffd55ad000 "Return-Path: <[e-mail]>\r\nReceived: from pmailfr-hx.ph-freiburg.de ([unix socket])\r\n\t by pmailfr-hx (Cyrus 3.0.12) with LMTPA;\r\n\t Thu, 23 Jan 2020 11:00:28 +0100\r\nX-Cyrus-Sessi"..., size=20442, obj=0x7fffd5cb6400)
    at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimeobj.cpp:223
#11 0x00007fffea9c840b in mime_display_stream_write(nsMIMESession*, char const*, int32_t)
    (stream=0x7fffdc27e400, buf=0x7fffd55ad000 "Return-Path: <[e-mail]>\r\nReceived: from pmailfr-hx.ph-freiburg.de ([unix socket])\r\n\t by pmailfr-hx (Cyrus 3.0.12) with LMTPA;\r\n\t Thu, 23 Jan 2020 11:00:28 +0100\r\nX-Cyrus-Sessi"..., size=20442)
    at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemoz2.cpp:853


#0  0x00007fffea9d3543 in MimeMultipartSigned_emit_child(MimeObject*) (obj=0x7fffd95b8660) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp:584
#1  0x00007fffea9d2d75 in MimeMultipartSigned_parse_eof(MimeObject*, bool) (obj=0x7fffd95b8660, abort_p=false) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsig.cpp:121
#2  0x00007fffea9d543c in MimeMultipart_parse_eof(MimeObject*, bool) (obj=0x7fffd5d03f00, abort_p=false) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemult.cpp:633
#3  0x00007fffea9a59e7 in MimeContainer_parse_eof(MimeObject*, bool) (object=0x7fffd5cb6400, abort_p=false) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimecont.cpp:93
#4  0x00007fffea9d02ac in MimeMessage_parse_eof(MimeObject*, bool) (obj=0x7fffd5cb6400, abort_p=false) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemsg.cpp:492
#5  0x00007fffea9c84e3 in mime_display_stream_complete(nsMIMESession*) (stream=0x7fffdc27e400) at /home/user/moz/comm-esr68/mozilla/comm/mailnews/mime/src/mimemoz2.cpp:867

In short, we reach the code to flush twice:

• mime_display_stream_write -> ... MimeMultipart_close_child -> MimeMultipartSigned_parse_eof -> emit
• mime_display_stream_complete -> ... MimeMultipart_parse_eof -> MimeMultipartSigned_parse_eof -> emit

I think MimeMultipartSigned isn't prepared to be emitted twice, but doesn't protect itself against it.

It checks for a flag closed_p, which is documented as: prevent multiple calls to parse_eof.

So we should probably set this variable to true - but where?

Very few places set it currently. Some MIME code sets it on failure. The primary place to set it is MimeObject_parse_eof, which is usually called by all _parse_eof functions, that each call their superclass->parse_eof.

I see we don't reach that when calling MimeMultipartSigned_parse_eof. We get a failure from MimeMultipartSigned_emit_child, and on failure, it doesn't set closed_p like other places do!

I'll test if that fixes the crash. Also, I'll check why MimeMultipartSigned_emit_child returns a failure.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: 1611105-v1.patch

This patch fixes the crash for me.

I mentioned that by MimeMultipartSigned_emit_child returns failure. That's expected. MimeMultCMS_init finds that we already have another S/MIME signature part in the message, and we refuse to process duplicate S/MIME signatures - and disable display of all signature information for this message.

Comment 4

[Kai Engert [:KaiE:]]

Documenting the nesting structure of the attached message:

Content-Type: multipart/mixed; boundary="===============5250413269614387544=="
--===============5250413269614387544==
    Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-256; boundary="------------ms060003030207090106080807"
    --------------ms060003030207090106080807
        Content-Type: multipart/mixed;
         boundary="------------D359E395F3E33596DCD69412"
        --------------D359E395F3E33596DCD69412
            Content-Type: text/plain; charset=utf-8; format=flowed
        --------------D359E395F3E33596DCD69412
            Content-Type: message/rfc822;
            Content-Type: multipart/signed; protocol="application/pkcs7-signature"; micalg=sha-256; boundary="------------ms090706050908080107090509"
            --------------ms090706050908080107090509
                Content-Type: text/plain; charset=utf-8; format=flowed
                --------------ms090706050908080107090509
                Content-Type: application/pkcs7-signature; name="smime.p7s"
            --------------ms090706050908080107090509--
        --------------D359E395F3E33596DCD69412--
    --------------ms060003030207090106080807
        Content-Type: application/pkcs7-signature; name="smime.p7s"
    --------------ms060003030207090106080807--
--===============5250413269614387544==
    Content-Type: text/plain; charset="iso-8859-1"
--===============5250413269614387544==--

Comment 5

[Wayne Mery (:wsmwk)]

What's the most likely crash signature that would be generated on crash-stats?

is nsMsgComposeSecure::MimeFinishMultipartSigned possible? bp-6e43c589-290e-41cd-803a-c650f0191101

Comment 6

[Kai Engert [:KaiE:]]

(In reply to Wayne Mery (:wsmwk) from comment #5)

What's the most likely crash signature that would be generated on crash-stats?

MimeObject_write
MimeInlineTextPlain_parse_eof
MimeInlineText_finalize
mime_free

is nsMsgComposeSecure::MimeFinishMultipartSigned possible? bp-6e43c589-290e-41cd-803a-c650f0191101

Seems unlikely to be related. This bug is about processing a received message.
That crash report is about sending out a new message that we create.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9122665 [details] [diff] [review]
1611105-v1.patch

Review of attachment 9122665 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx!

Comment 8

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/comm-central/rev/237a41008a61db0eaa252f3c0bea04c89c455fb7

Comment 9

[Wayne Mery (:wsmwk)]

What's the most likely crash signature that would be generated on crash-stats?

is nsMsgComposeSecure::MimeFinishMultipartSigned a possibility? bp-6e43c589-290e-41cd-803a-c650f0191101

Comment 10

[Kai Engert [:KaiE:]]

Wayne, you submitted your question twice. See comment 6 for my response.

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 73.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/39542aae79eca484cab6f77058105ea47006d5ea

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to Kai Engert (:KaiE:) from comment #6)

(In reply to Wayne Mery (:wsmwk) from comment #5)

What's the most likely crash signature that would be generated on crash-stats?

MimeObject_write
MimeInlineTextPlain_parse_eof
MimeInlineText_finalize
mime_free

so perhaps ....
MIME_MimeObject_write bp-34006993-76ba-4904-a6da-b3fc20200203
0 xul.dll MIME_MimeObject_write(MimeObject*, char const*, int, bool) comm/mailnews/mime/src/mimei.cpp:200
1 xul.dll static int MimeInlineTextPlain_parse_eof(struct MimeObject*, bool) comm/mailnews/mime/src/mimetpla.cpp:234
2 xul.dll static void MimeInlineText_finalize(struct MimeObject*) comm/mailnews/mime/src/mimetext.cpp:159
3 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
4 xul.dll static void MimeContainer_finalize(struct MimeObject*) comm/mailnews/mime/src/mimecont.cpp:67
5 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
6 xul.dll static void MimeContainer_finalize(struct MimeObject*) comm/mailnews/mime/src/mimecont.cpp:67
7 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
8 xul.dll static void MimeContainer_finalize(struct MimeObject*) comm/mailnews/mime/src/mimecont.cpp:67
9 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
10 xul.dll mime_display_stream_complete comm/mailnews/mime/src/mimemoz2.cpp:892
11 xul.dll nsStreamConverter::OnStopRequest(nsIRequest*, nsresult) comm/mailnews/mime/src/nsStreamConverter.cpp:922
12 xul.dll nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsresult) uriloader/base/nsURILoader.cpp:360

xul.dll | mime_free | MimeContainer_finalize bp-80e48557-1eb9-4dcb-ae22-e38040200114
0 xul.dll xul.dll@0x2142ed
1 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
2 xul.dll static void MimeContainer_finalize(struct MimeObject*) comm/mailnews/mime/src/mimecont.cpp:67
3 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
4 xul.dll static void MimeContainer_finalize(struct MimeObject*) comm/mailnews/mime/src/mimecont.cpp:67
5 xul.dll mime_free comm/mailnews/mime/src/mimei.cpp:255
6 xul.dll mime_display_stream_complete comm/mailnews/mime/src/mimemoz2.cpp:892
7 xul.dll nsStreamConverter::OnStopRequest(nsIRequest*, nsresult) comm/mailnews/mime/src/nsStreamConverter.cpp:922
8 xul.dll nsDocumentOpenInfo::OnStopRequest(nsIRequest*, nsresult) uriloader/base/nsURILoader.cpp:360
9 xul.dll nsMsgProtocol::OnStopRequest(nsIRequest*, nsresult) comm/mailnews/base/util/nsMsgProtocol.cpp:384

Comment 13

[Kai Engert [:KaiE:]]

Although the code that causes this crash has always been there, the fix from bug 1240290 causes this crash scenario to be reached frequently with certain S/MIME messages.

Comment 14

[Kai Engert [:KaiE:]]

Rob, can you please land into comm-esr68? Thanks.

Comment 15

[Rob Lemley [:rjl]]

c-esr68:
https://hg.mozilla.org/releases/comm-esr68/rev/5e3454394b88