| Summary: | Crashes when trying to hash caps containing pathological data forms | ||
|---|---|---|---|
| Product: | Telepathy | Reporter: | Will Thompson <will> |
| Component: | gabble | Assignee: | Will Thompson <will> |
| Status: | RESOLVED FIXED | QA Contact: | Will Thompson <will> |
| Severity: | normal | ||
| Priority: | medium | CC: | guillaume.desmottes, jonny.lamb, smcv, will |
| Version: | git master | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
[0.12 1/6] Turn off deprecation warnings, this is a stable branch
[0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl [0.12 2a/6] tests: Fix compilation error with libsasl2 [0.12 2b/6] sasl test: fix build failure on older libsasl2s [0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl [0.12 3/6] jingle/outgoing-basics: disregard MembersChangedDetailed flag [0.12 4/6] presence-cache: don't crash if computed_hash == NULL [0.12 5/6] fd.o #61433: caps hash: make all field comparisons NULL-safe [0.12 6/6] Test for #61433 and #39464 |
||
Here's the fix in Wocky: http://cgit.collabora.com/git/user/wjt/wocky/log/?h=61433-caps-hash-crash Here's a test in Wocky: http://cgit.collabora.com/git/user/wjt/wocky/log/?h=61433-caps-hash-crash-test Here's the Gabble 0.16 branch which pulls in just the fixes to Wocky: http://cgit.collabora.com/git/user/wjt/telepathy-gabble/log/?h=61433-caps-hash-crash Here's a branch on top of that which adds a test to Gabble, too: http://cgit.collabora.com/git/user/wjt/telepathy-gabble/log/?h=61433-caps-hash-crash-test Ubuntu Oneiric has 0.13.5 to which the same patches should apply. Debian testing and Ubuntu's current LTS has 0.16.x so a release on that series will do. Maemo Harmattan has 0.12.6 which has the same bug, but the code in that version lives in Gabble, not Wocky, so dataforms_cmp and caps_hash_compute in src/caps-hash.c would need equivalent fixes. Do we have a CVE ID for this, or a plan to get one? Kurt Seifried of the Red Hat security team <secalert@redhat.com> does most of the allocations on oss-security, and has offered to allocate CVEs before advisories go out. Patches all look good to me, r+. Someone will have to backport this to 0.9.x for Debian stable :-( Created attachment 75495 [details] [review] [0.12 1/6] Turn off deprecation warnings, this is a stable branch Also do the same for our Wocky and Yell submodules, by applying the corresponding patches to them. --- Wocky and Yell patches not attached, but they're the same change. Created attachment 75496 [details] [review] [0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl --- I'll attach them in a moment. Created attachment 75497 [details] [review] [0.12 2a/6] tests: Fix compilation error with libsasl2 From: Sjoerd Simons <sjoerd.simons@collabora.co.uk> Created attachment 75498 [details] [review] [0.12 2b/6] sasl test: fix build failure on older libsasl2s From: Jonny Lamb <jonny.lamb@collabora.co.uk> A second try. Signed-off-by: Jonny Lamb <jonny.lamb@collabora.co.uk> Backported: Simon McVittie <simon.mcvittie@collabora.co.uk> --- I screwed up the attribution on the version referenced by Attachment #75496 [details], sorry. This one is correct. Created attachment 75499 [details] [review] [0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl Created attachment 75500 [details] [review] [0.12 3/6] jingle/outgoing-basics: disregard MembersChangedDetailed flag From: Will Thompson <will.thompson@collabora.co.uk> In 0.15, 65c8b8f makes this test always expect the Members_Changed_Detailed flag to be present in the channel's group flags. But 0.14 doesn't depend on a new enough telepathy-glib to be sure of that; so here we just mask that flag. Created attachment 75501 [details] [review] [0.12 4/6] presence-cache: don't crash if computed_hash == NULL From: David Laban <david.laban@collabora.co.uk> [Necessary to fix #61433. -smcv] Created attachment 75502 [details] [review] [0.12 5/6] fd.o #61433: caps hash: make all field comparisons NULL-safe This is less targeted than wjt's version in Wocky - some of these are probably guaranteed to be non-NULL anyway, but it's quicker to g_strcmp0 everywhere. Created attachment 75503 [details] [review] [0.12 6/6] Test for #61433 and #39464 From: Will Thompson <will.thompson@collabora.co.uk> Backported: Simon McVittie <simon.mcvittie@collabora.co.uk> (In reply to comment #9) > [0.12 4/6] presence-cache: don't crash if computed_hash == NULL > [0.12 5/6] fd.o #61433: caps hash: make all field comparisons NULL-safe These two also apply to Debian stable's 0.9.15. (In reply to comment #10) > Created attachment 75502 [details] [review] [review] > [0.12 5/6] fd.o #61433: caps hash: make all field comparisons NULL-safe > > This is less targeted than wjt's version in Wocky - some of these are > probably guaranteed to be non-NULL anyway, but it's quicker to > g_strcmp0 everywhere. Inspired by this, I've found some other cases which crash in the Wocky code… We have been assigned CVE-2013-1769 for this issue by Kurt Seifried of the Red Hat Security Response Team. The new tip of http://cgit.collabora.com/git/user/wjt/wocky/log/?h=61433-caps-hash-crash fixes two crashes caused by FORM_TYPE fields without <value>s. http://cgit.collabora.com/git/user/wjt/wocky/log/?h=61433-caps-hash-crash-test has a test for that patch. It also has a test for the case where FORM_TYPE has 2 <value>s, which XEP-0115 says should make the algorithm fail. Wocky currently ignores everything after the first one because raw_value_contents is not actually the raw value contents, so that test fails. This separate bug doesn't have any security impact that I can see. If Wocky were to include the extra <value>s in the hash calculation, it might be possible to collide with the hash of another form. If this: <field name='FORM_TYPE' type='hidden'> <value>bar</value> <value>foo</value> </field> <field name='alpha' type='text-single'> <value>omega</value> </field> was erroneously encoded by Wocky as "…bar<foo<alpha<omega<…", this would collide with: <field name='FORM_TYPE' type='hidden'> <value>bar</value> </field> <field name='foo' type='text-single'> <value>alpha</value> <value>omega</value> </field> which legitimately is encoded as "…bar<foo<alpha<omega<…". But Wocky does not do this. Simon: all your 0.12 patches look fine. This is fixed in 0.16.5 and 0.17.3. I believe that telepathy-gabble 0.8 is also affected. Simon's backported fixes for the 0.12 branch (specifically attachment 75501 [details] [review] and attachment 75502 [details] [review]) should apply pretty cleanly and fix the bug in the 0.8 branch too. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Calculating a XEP-0115 hash involves sorting the fields of data forms in the contact's capabilities according to their field name. wocky_caps_hash_compute_from_lists() does this using this function: gint wocky_data_form_field_cmp (const WockyDataFormField *left, const WockyDataFormField *right) { return strcmp (left->var, right->var); } This will obviously crash if either ->var is NULL, but WockyDataForm validates forms it parses so this shouldn't happen, right? Unfortunately, http://xmpp.org/extensions/xep-0004.html#protocol-field says: > If the <field/> element type is anything other than "fixed" (see below), it > MUST possess a 'var' attribute that uniquely identifies the field in the > context of the form (if it is "fixed", it MAY possess a 'var' attribute). And WockyDataForm implements this correctly: var = wocky_node_get_attribute (node, "var"); if (var == NULL && type != WOCKY_DATA_FORM_FIELD_TYPE_FIXED) { DEBUG ("field node doesn't have a 'var' attribute; ignoring"); return FALSE; } So we have a remotely-triggered DoS: send Gabble a <presence> with a caps hash; include a form with an anonymous fixed field in the reply; boom. Since anyone can send presence to anyone else, and Gabble always looks up any caps it sees in any presences it receives. (Note that this is a presence leak, too; another bug, I think.) I have some fixes; patches to follow. I will also suggest that http://xmpp.org/extensions/xep-0115.html be updated to mention this.