Bug 61433 - (CVE-2013-1769) Crashes when trying to hash caps containing pathological data forms
(CVE-2013-1769)
Crashes when trying to hash caps containing pathological data forms
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: gabble
git master
Other All
: medium normal
Assigned To: Will Thompson
Will Thompson
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-25 09:19 UTC by Will Thompson
Modified: 2013-06-17 21:00 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[0.12 1/6] Turn off deprecation warnings, this is a stable branch (1.42 KB, patch)
2013-02-25 11:46 UTC, Simon McVittie
Details | Splinter Review
[0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl (591 bytes, patch)
2013-02-25 11:47 UTC, Simon McVittie
Details | Splinter Review
[0.12 2a/6] tests: Fix compilation error with libsasl2 (2.08 KB, patch)
2013-02-25 11:48 UTC, Simon McVittie
Details | Splinter Review
[0.12 2b/6] sasl test: fix build failure on older libsasl2s (1.16 KB, patch)
2013-02-25 11:51 UTC, Simon McVittie
Details | Splinter Review
[0.12 2/6] Cherry-pick more Wocky commits to fix compilation with newer libsasl (591 bytes, patch)
2013-02-25 11:52 UTC, Simon McVittie
Details | Splinter Review
[0.12 3/6] jingle/outgoing-basics: disregard MembersChangedDetailed flag (1.50 KB, patch)
2013-02-25 11:53 UTC, Simon McVittie
Details | Splinter Review
[0.12 4/6] presence-cache: don't crash if computed_hash == NULL (1.06 KB, patch)
2013-02-25 11:53 UTC, Simon McVittie
Details | Splinter Review
[0.12 5/6] fd.o #61433: caps hash: make all field comparisons NULL-safe (2.08 KB, patch)
2013-02-25 11:54 UTC, Simon McVittie
Details | Splinter Review
[0.12 6/6] Test for #61433 and #39464 (3.95 KB, patch)
2013-02-25 11:54 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Will Thompson 2013-02-25 09:19:38 UTC
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.
Comment 1 Will Thompson 2013-02-25 10:02:08 UTC
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.
Comment 2 Simon McVittie 2013-02-25 11:07:41 UTC
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 :-(
Comment 3 Simon McVittie 2013-02-25 11:46:53 UTC
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.
Comment 4 Simon McVittie 2013-02-25 11:47:41 UTC
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.
Comment 5 Simon McVittie 2013-02-25 11:48:38 UTC
Created attachment 75497 [details] [review]
[0.12 2a/6] tests: Fix compilation error with libsasl2

From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Comment 6 Simon McVittie 2013-02-25 11:51:52 UTC
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.
Comment 7 Simon McVittie 2013-02-25 11:52:41 UTC
Created attachment 75499 [details] [review]
[0.12 2/6] Cherry-pick more Wocky commits to fix compilation with  newer libsasl
Comment 8 Simon McVittie 2013-02-25 11:53:19 UTC
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.
Comment 9 Simon McVittie 2013-02-25 11:53:49 UTC
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]
Comment 10 Simon McVittie 2013-02-25 11:54:08 UTC
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.
Comment 11 Simon McVittie 2013-02-25 11:54:35 UTC
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>
Comment 12 Simon McVittie 2013-02-25 12:23:46 UTC
(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.
Comment 13 Will Thompson 2013-02-26 09:27:02 UTC
(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…
Comment 14 Will Thompson 2013-02-27 08:58:48 UTC
We have been assigned CVE-2013-1769 for this issue by Kurt Seifried of the Red Hat Security Response Team.
Comment 15 Will Thompson 2013-02-27 09:50:58 UTC
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.
Comment 16 Will Thompson 2013-03-01 12:12:52 UTC
Simon: all your 0.12 patches look fine.
Comment 17 Will Thompson 2013-03-04 12:38:11 UTC
This is fixed in 0.16.5 and 0.17.3.
Comment 18 Will Thompson 2013-06-17 21:00:42 UTC
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.