Open Bug 314337 Opened 14 years ago Updated 3 months ago

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

Categories

(Calendar :: Internal Components, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

People

(Reporter: dmose, Unassigned, NeedInfo)

References

Details

(Keywords: crash, qawanted, sec-audit)

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.
Component: Base → libical
QA Contact: base → libical
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.
Assignee: shaver → nobody
Flags: blocking0.3?
Component: libical → Internal Components
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: libical → base
Major interop blocker.
Flags: blocking0.3? → blocking0.3+
Whiteboard: [swag:3d]
taking this one.
Assignee: nobody → michael.buettner
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???
Whiteboard: [swag:3d] → [swag:3d][needs input from dmose]
Whiteboard: [swag:3d][needs input from dmose] → [swag:3d][needs input from dmose][no l10n impact]
Blocks: 321653
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.
Flags: blocking0.3+ → blocking0.3-
Target Milestone: --- → Sunbird 0.5
Fixing what dmose meant
i think we should close this one. WONTFIX?
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.
Whiteboard: [swag:3d][needs input from dmose][no l10n impact]
Reassigning to dmose...
Assignee: michael.buettner → dmose
This needs to be either determined to be a non-issue or fixed as part of the timezone work.
Flags: blocking-calendar0.5+
Priority: -- → P1
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.
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.
Assignee: dmose → nobody
Flags: blocking-calendar0.5+ → blocking-calendar0.5-
Priority: P1 → --
Not going to make the 0.5 train.
Target Milestone: Sunbird 0.5 → ---
Flags: blocking-calendar0.5-
Severity: normal → critical
Keywords: crash, qawanted
Whiteboard: [sg: audit]
Is this bug still valid for the current codebase?
Keywords: sec-audit
Whiteboard: [sg: audit]
(In reply to Daniel Veditz [:dveditz] from comment #15)
> Is this bug still valid for the current codebase?
Flags: needinfo?(philipp)
Given we didn't make any major changes to libical in the past years I assume this is still valid.
Flags: needinfo?(philipp)

Does this still deserve sec-audit?

Flags: needinfo?(kaie)
Summary: calling addSubcomponent on an already-present VTIMEZONE can lead to crashes and other badness → calling addSubcomponent on an already-present VTIMEZONE can lead to crashes and other badness (libical)
You need to log in before you can comment on or make changes to this bug.