1282130 - (CVE-2016-5823) SEGV in icalproperty_new_clone

Status: RESOLVED FIXED

Description

[Daniel Veditz [:dveditz]]

Attachment: segv.ics

From oss-security where Brandon Perry reports

Attached is a test case for causing a crash in libical 0.47 (shipped with Thunderbird) and this was also tested against 1.0 (various versions shipped with various email clients).


=================================================================
==24662==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x0000004fbb80 bp 0x7ffd68d966f0 sp 0x7ffd68d96520 T0)
    #0 0x4fbb7f in icalproperty_new_clone (/root/tmp/new_parse/parse_string047_asan+0x4fbb7f)
    #1 0x4f44e6 in icalparser_add_line (/root/tmp/new_parse/parse_string047_asan+0x4f44e6)
    #2 0x4efabe in icalparser_parse (/root/tmp/new_parse/parse_string047_asan+0x4efabe)
    #3 0x4f9c1f in icalparser_parse_string (/root/tmp/new_parse/parse_string047_asan+0x4f9c1f)
    #4 0x4eb7ef in main (/root/tmp/new_parse/parse_string047_asan+0x4eb7ef)
    #5 0x7fb657683a3f in __libc_start_main /build/glibc-ryFjv0/glibc-2.21/csu/libc-start.c:289
    #6 0x444ae8 in _start (/root/tmp/new_parse/parse_string047_asan+0x444ae8)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ??:0 icalproperty_new_clone
==24662==ABORTING

Comment 1

[Daniel Veditz [:dveditz]]

Confirming Thunderbird crash: bp-eb513b56-05e6-4c39-a35b-8baa92160624

This may be the same as bug 616214 which rkent seems to have diagnosed, although we didn't have a reproducing testcase.

If someone sends you mail with this .ics Thunderbird will crash when it's processed, and then continue to crash on each startup as it again tries to process the mail. To get out of that state I had to use a browser to delete the mail from my provider's webmail interface. This could turn into a pretty nasty DOS as many recipients might not have (or think of) an alternate way to access their mail account and delete the mail, or even know which one was causing the problem if several mails have come in since the last check.

This is a Denial of Service attack, but not otherwise exploitable, and since it's been published to oss-security we might as well not hide it.

Comment 2

[[:MakeMyDay]]

Referencing the posting [1], which also includes some complaints about responsiveness on other reported bugs.

Have this and the other mentioned bugs been (also) reported upstream [2], so we could backport already implemented fixes if any? If so, a reference would be helpful.

[1] http://www.openwall.com/lists/oss-security/2016/06/24/2
[2] https://github.com/libical/libical

Comment 3

[Brandon Perry]

I first reported my first heap overread (with small details since it was potentially sensitive) to the libical github, but the issue sat for a few days before with no response before I began coming to Mozilla with the bugs since they seemed to also affect thunderbird.

Comment 4

[Decathlon]

Kent James found the problem in bug 616214 comment 3.
Currently libical has the fix as described by Kent James:
https://github.com/libical/libical/blob/master/src/libical/icalproperty.c#L99

I've applied the fix with another patch that I had to test on the try server:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=beaf9369870208ea3739a9db0b19ac143db79c4b
https://hg.mozilla.org/try-comm-central/rev/32b5675bed1c7ca59112b217382ec147fe9b83dc

with the patch, Thunderbird doesn't crash when importing the test calendar attached here and an error message with a dialog appears instead:


Unable to read from file:C:\...\segv.ics
[Exception... "Component returned failure code: 0x804a0101 [calIIcalComponent.toString]"  nsresult: "0x804a0101 (<unknown>)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///C:/.../extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calIcsParser.js :: ip_processIcalComponent :: line 44"  data: no]


after that Lightning asks anyway the calendar where importing the event in, and maybe this shouldn't happen, but I think this is another issue.

Comment 5

[[:MakeMyDay]]

Attachment: SegvInLibicalIcalpropertyClone-V1.diff

This patch implements the upstream solution from libical and makes the effectively logged error message more meaningful. For the import use case, it adds a adds prompting an error message and prevents showing the calendar chooser if no item would be imported for what reason ever.

Philipp, what is your preference regarding the new string for esr uplift? Do you want to take it as is with falling back to en-US, with a hard coded string or without the prompt section (and let it fail silently)?

As this bug is public anyway, are there any concerns to land the patch once reviewed? Or should we wait until next esr release date?

Comment 7

[Philipp Kewisch [:Fallen] [:📆]]

Comment on attachment 8769485 [details] [diff] [review]
SegvInLibicalIcalpropertyClone-V1.diff

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

LGTM. For beta/aurora/esr I think it is best to use a hardcoded message, but only show it in the error console. a+ with these changes. Given this bug is public anyway I don't see a need to postpone the checkin, but maybe you could use a commit message that doesn't scream security issue, e.g. "Bug 1282130 - Improve error message for failing ICS import".

Comment 8

[[:MakeMyDay]]

https://hg.mozilla.org/comm-central/rev/dce2850d96b9a891bb078367bcf6e07c5cc5d585
https://hg.mozilla.org/releases/comm-aurora/rev/92018d3dc161b198867d57c8f488107b0ad24724
https://hg.mozilla.org/releases/comm-beta/rev/0f6724873dc9f6e2fa80a202ecf6045f4ed37679
https://hg.mozilla.org/releases/comm-esr45/rev/290ae951711590403aa72af478613b682b3ae8da