314337 - calling addSubcomponent on an already-present VTIMEZONE can lead to crashes and other badness (libical)

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Mostly orthogonal to the issues raised in bug 314334, calling addSubcomponent on a VTIMEZONE will ultimately lead to crashes while serializing.

Scenario:

Caller adds an item or event to the calendar which contains a certain VTIMEZONE.  Later, caller adds a copy of the same VTIMEZONE component directly to the calendar.  At serialization time, the calIComponent code enumerates the list of all timezones referenced by events in the calendar, and then adds them by calling icalcomponent_merge_component on a VCALENDAR containing only a VTIMEZONE component.  During this operation, libical notices that this timezone is already present, and decides that it can just free the passed-in copy.  icalcomponent_free ends up freeing it twice: first with the icaltimezone_free() that is called by icalcomponent_remove_component at <http://lxr.mozilla.org/mozilla/source/calendar/libical/src/libical/icalcomponent.c#248>
and then again directly by calling icalcomponent_free recursively on the very next line.

We might be able to get away with just not supporting calling AddSubcomponent with VTIMEZONE and throwing an exception when someone tries.  I suspect that the reason this bug has not been noticed in libical before is because our merging of calendars containing only timezone components may be something that no other user of libical does.  

Also relevant here (and could perhaps be used as a workaround fix): currently GetReferencedTimezones doesn't notice a VTIMEZONE component that's passed in directly; it only notices stuff that's referenced by DATETIME properties.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Unfortunately, I do not have cycles to work on Calendar stuff these days (just as it's getting to the good part!), so I am a bad owner for these bugs.  To delete the tragically-large chunk of bugspam, search for gregorianabdication.

Comment 2

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!

Comment 3

[Joey Minta]

Major interop blocker.

Comment 4

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 5

[Michael Büttner (no reviews TFN)]

dmose: i'm not sure whether or not i completely understand this issue. icalcomponent_merge_component() takes two VCALENDAR components and merges the second one into the first, the second one will no longer exist after this function returns. but in AddTimezoneComponentToIcal() the VTIMEZONE component is explicetly cloned before being passed to icalcomponent_merge_component(). http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calICSService.cpp#958
Could you please shed some light in here???

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

After looking at talkback, I think this is mostly a theoretical problem if it's a real problem at all.  I can't see any crashes which happened after the addSubcomponent fix that look like they should be attributed to this.

Comment 7

[Matthew (lilmatt) Willis]

Fixing what dmose meant

Comment 8

[Michael Büttner (no reviews TFN)]

i think we should close this one. WONTFIX?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Since many crashes can lead to security bugs, I'd like to be convinced that the code here is not exploitable before we close this bug.  If you'd rather not have it on your bug radar, feel free to re-assign to me.

Comment 10

[Michael Büttner (no reviews TFN)]

Reassigning to dmose...

Comment 11

[Matthew (lilmatt) Willis]

This needs to be either determined to be a non-issue or fixed as part of the timezone work.

Comment 12

[Stefan Sitter [:ssitter]]

This bug is currently blocking 0.5 release. 

Is there a testcase or steps to reproduce the crash in current branch builds? I have not seen talkback reports or bug reports that show this issue.

Comment 13

[Matthew (lilmatt) Willis]

Since we have no real STR and this is theoretical, I'm inclined to minus this for 0.5.  Also reassigning to nobody@m.o.

Comment 14

[Matthew (lilmatt) Willis]

Not going to make the 0.5 train.

Comment 15

[Daniel Veditz [:dveditz]]

Is this bug still valid for the current codebase?

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to Daniel Veditz [:dveditz] from comment #15)
> Is this bug still valid for the current codebase?

Comment 17

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

Given we didn't make any major changes to libical in the past years I assume this is still valid.

Comment 18

[Wayne Mery (:wsmwk)]

Does this still deserve sec-audit?

Comment 19

[Wayne Mery (:wsmwk)]

With libcal going away, perhaps we don't care about this

Comment 20

[Magnus Melin [:mkmelin]]

libical has now been removed - bug 1787097.