Bug 61433 (CVE-2013-1769) - Crashes when trying to hash caps containing pathological data forms
Summary: Crashes when trying to hash caps containing pathological data forms
Status: RESOLVED FIXED
Alias: CVE-2013-1769
Product: Telepathy
Classification: Unclassified
Component: gabble (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Will Thompson
QA Contact: Will Thompson
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
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.


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.