/pidgin/main: 93d4bff19574: Prevent spoofing of iq replies by ve...
Mark Doliner
mark at kingant.net
Tue Jan 28 10:38:11 EST 2014
Changeset: 93d4bff19574e8ee25f99b12478e813c43d04562
Author: Mark Doliner <mark at kingant.net>
Date: 2014-01-12 22:51 -0800
Branch: release-2.x.y
URL: https://hg.pidgin.im/pidgin/main/rev/93d4bff19574
Description:
Prevent spoofing of iq replies by verifying that the 'from' address
matches the 'to' address of the iq request.
This full extent of this problem was realized by Thijs Alkemade, while
investigating a problem reported to us by Fabian Yamaguchi and Christian
Wressnegger of the University of Goettingen)
This change was created by Thijs Alkemade, with small shuffling and
variable renaming by me.
diffstat:
ChangeLog | 4 ++
libpurple/protocols/jabber/iq.c | 71 ++++++++++++++++++++++++++++++------
libpurple/protocols/jabber/iq.h | 2 +
libpurple/protocols/jabber/jabber.c | 2 +-
libpurple/protocols/jabber/jutil.c | 52 +++++++++++++++++++++++++++
libpurple/protocols/jabber/jutil.h | 11 +++++
6 files changed, 128 insertions(+), 14 deletions(-)
diffs (truncated from 311 to 300 lines):
diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -69,6 +69,10 @@ version 2.10.8:
(Discovered by Sourcefire VRT) (CVE-2014-NNNN)
XMPP:
+ * Prevent spoofing of iq replies by verifying that the 'from' address
+ matches the 'to' address of the iq request. (Discovered by Fabian
+ Yamaguchi and Christian Wressnegger of the University of Goettingen)
+ (CVE-2014-NNNN)
* Fix possible crash or other erratic behavior when selecting a very
small file for your own buddy icon.
* Fix crash if the user tries to initiate a voice/video session with a
diff --git a/libpurple/protocols/jabber/iq.c b/libpurple/protocols/jabber/iq.c
--- a/libpurple/protocols/jabber/iq.c
+++ b/libpurple/protocols/jabber/iq.c
@@ -49,6 +49,18 @@
static GHashTable *iq_handlers = NULL;
static GHashTable *signal_iq_handlers = NULL;
+struct _JabberIqCallbackData {
+ JabberIqCallback *callback;
+ gpointer data;
+ JabberID *to;
+};
+
+void jabber_iq_callbackdata_free(JabberIqCallbackData *jcd)
+{
+ jabber_id_free(jcd->to);
+ g_free(jcd);
+}
+
JabberIq *jabber_iq_new(JabberStream *js, JabberIqType type)
{
JabberIq *iq;
@@ -98,11 +110,6 @@ JabberIq *jabber_iq_new_query(JabberStre
return iq;
}
-typedef struct _JabberCallbackData {
- JabberIqCallback *callback;
- gpointer data;
-} JabberCallbackData;
-
void
jabber_iq_set_callback(JabberIq *iq, JabberIqCallback *callback, gpointer data)
{
@@ -125,15 +132,17 @@ void jabber_iq_set_id(JabberIq *iq, cons
void jabber_iq_send(JabberIq *iq)
{
- JabberCallbackData *jcd;
+ JabberIqCallbackData *jcd;
g_return_if_fail(iq != NULL);
jabber_send(iq->js, iq->node);
if(iq->id && iq->callback) {
- jcd = g_new0(JabberCallbackData, 1);
+ jcd = g_new0(JabberIqCallbackData, 1);
jcd->callback = iq->callback;
jcd->data = iq->callback_data;
+ jcd->to = jabber_id_new(xmlnode_get_attrib(iq->node, "to"));
+
g_hash_table_insert(iq->js->iq_callbacks, g_strdup(iq->id), jcd);
}
@@ -276,18 +285,30 @@ void jabber_iq_remove_callback_by_id(Jab
void jabber_iq_parse(JabberStream *js, xmlnode *packet)
{
- JabberCallbackData *jcd;
+ JabberIqCallbackData *jcd;
xmlnode *child, *error, *x;
const char *xmlns;
const char *iq_type, *id, *from;
JabberIqType type = JABBER_IQ_NONE;
gboolean signal_return;
+ JabberID *from_id;
from = xmlnode_get_attrib(packet, "from");
id = xmlnode_get_attrib(packet, "id");
iq_type = xmlnode_get_attrib(packet, "type");
/*
+ * Ensure the 'from' attribute is valid. No point in handling a stanza
+ * of which we don't understand where it came from.
+ */
+ from_id = jabber_id_new(from);
+
+ if (from && !from_id) {
+ purple_debug_error("jabber", "Received an iq with an invalid from: %s\n", from);
+ return;
+ }
+
+ /*
* child will be either the first tag child or NULL if there is no child.
* Historically, we used just the 'query' subchild, but newer XEPs use
* differently named children. Grabbing the first child is (for the time
@@ -312,6 +333,7 @@ void jabber_iq_parse(JabberStream *js, x
if (type == JABBER_IQ_NONE) {
purple_debug_error("jabber", "IQ with invalid type ('%s') - ignoring.\n",
iq_type ? iq_type : "(null)");
+ jabber_id_free(from_id);
return;
}
@@ -342,20 +364,38 @@ void jabber_iq_parse(JabberStream *js, x
purple_debug_error("jabber", "IQ of type '%s' missing id - ignoring.\n",
iq_type);
+ jabber_id_free(from_id);
return;
}
signal_return = GPOINTER_TO_INT(purple_signal_emit_return_1(purple_connection_get_prpl(js->gc),
"jabber-receiving-iq", js->gc, iq_type, id, from, packet));
- if (signal_return)
+ if (signal_return) {
+ jabber_id_free(from_id);
return;
+ }
/* First, lets see if a special callback got registered */
if(type == JABBER_IQ_RESULT || type == JABBER_IQ_ERROR) {
if((jcd = g_hash_table_lookup(js->iq_callbacks, id))) {
- jcd->callback(js, from, type, id, packet, jcd->data);
- jabber_iq_remove_callback_by_id(js, id);
- return;
+ if(jabber_id_equal(js, jcd->to, from_id)) {
+ jcd->callback(js, from, type, id, packet, jcd->data);
+ jabber_iq_remove_callback_by_id(js, id);
+ jabber_id_free(from_id);
+ return;
+ } else {
+ char *expected_to;
+
+ if (jcd->to) {
+ expected_to = jabber_id_get_full_jid(jcd->to);
+ } else {
+ expected_to = jabber_id_get_bare_jid(js->user);
+ }
+
+ purple_debug_error("jabber", "Got a result iq with id %s from %s instead of expected %s!\n", id, from ? from : "(null)", expected_to);
+
+ g_free(expected_to);
+ }
}
}
@@ -372,12 +412,15 @@ void jabber_iq_parse(JabberStream *js, x
if (signal_ref > 0) {
signal_return = GPOINTER_TO_INT(purple_signal_emit_return_1(purple_connection_get_prpl(js->gc), "jabber-watched-iq",
js->gc, iq_type, id, from, child));
- if (signal_return)
+ if (signal_return) {
+ jabber_id_free(from_id);
return;
+ }
}
if(jih) {
jih(js, from, type, id, child);
+ jabber_id_free(from_id);
return;
}
}
@@ -404,6 +447,8 @@ void jabber_iq_parse(JabberStream *js, x
jabber_iq_send(iq);
}
+
+ jabber_id_free(from_id);
}
void jabber_iq_register_handler(const char *node, const char *xmlns, JabberIqHandler *handlerfunc)
diff --git a/libpurple/protocols/jabber/iq.h b/libpurple/protocols/jabber/iq.h
--- a/libpurple/protocols/jabber/iq.h
+++ b/libpurple/protocols/jabber/iq.h
@@ -36,6 +36,7 @@ typedef enum {
#include "connection.h"
typedef struct _JabberIq JabberIq;
+typedef struct _JabberIqCallbackData JabberIqCallbackData;
/**
* A JabberIqHandler is called to process an incoming IQ stanza.
@@ -96,6 +97,7 @@ JabberIq *jabber_iq_new_query(JabberStre
void jabber_iq_parse(JabberStream *js, xmlnode *packet);
+void jabber_iq_callbackdata_free(JabberIqCallbackData *jcd);
void jabber_iq_remove_callback_by_id(JabberStream *js, const char *id);
void jabber_iq_set_callback(JabberIq *iq, JabberIqCallback *cb, gpointer data);
void jabber_iq_set_id(JabberIq *iq, const char *id);
diff --git a/libpurple/protocols/jabber/jabber.c b/libpurple/protocols/jabber/jabber.c
--- a/libpurple/protocols/jabber/jabber.c
+++ b/libpurple/protocols/jabber/jabber.c
@@ -988,7 +988,7 @@ jabber_stream_new(PurpleAccount *account
js->user_jb->subscription |= JABBER_SUB_BOTH;
js->iq_callbacks = g_hash_table_new_full(g_str_hash, g_str_equal,
- g_free, g_free);
+ g_free, (GDestroyNotify)jabber_iq_callbackdata_free);
js->chats = g_hash_table_new_full(g_str_hash, g_str_equal,
g_free, (GDestroyNotify)jabber_chat_free);
js->next_id = g_random_int();
diff --git a/libpurple/protocols/jabber/jutil.c b/libpurple/protocols/jabber/jutil.c
--- a/libpurple/protocols/jabber/jutil.c
+++ b/libpurple/protocols/jabber/jutil.c
@@ -508,6 +508,34 @@ jabber_id_free(JabberID *jid)
}
}
+
+gboolean
+jabber_id_equal(JabberStream *js, const JabberID *jid1, const JabberID *jid2)
+{
+ const JabberID *j1, *j2;
+ JabberID *bare_user_jid;
+ gboolean equal;
+
+ /* If an outgoing stanza has no 'to', or an incoming has no 'from',
+ * then those are "the server acting as my account". This function will
+ * handle that correctly.
+ */
+ if (!jid1 && !jid2)
+ return TRUE;
+
+ bare_user_jid = jabber_id_to_bare_jid(js->user);
+ j1 = jid1 ? jid1 : bare_user_jid;
+ j2 = jid2 ? jid2 : bare_user_jid;
+
+ equal = purple_strequal(j1->node, j2->node) &&
+ purple_strequal(j1->domain, j2->domain) &&
+ purple_strequal(j1->resource, j2->resource);
+
+ jabber_id_free(bare_user_jid);
+
+ return equal;
+}
+
char *jabber_get_domain(const char *in)
{
JabberID *jid = jabber_id_new(in);
@@ -536,6 +564,17 @@ char *jabber_get_resource(const char *in
return out;
}
+JabberID *
+jabber_id_to_bare_jid(const JabberID *jid)
+{
+ JabberID *result = g_new0(JabberID, 1);
+
+ result->node = g_strdup(jid->node);
+ result->domain = g_strdup(jid->domain);
+
+ return result;
+}
+
char *
jabber_get_bare_jid(const char *in)
{
@@ -561,6 +600,19 @@ jabber_id_get_bare_jid(const JabberID *j
NULL);
}
+char *
+jabber_id_get_full_jid(const JabberID *jid)
+{
+ g_return_val_if_fail(jid != NULL, NULL);
+
+ return g_strconcat(jid->node ? jid->node : "",
+ jid->node ? "@" : "",
+ jid->domain,
+ jid->resource ? "/" : "",
+ jid->resource ? jid->resource : "",
+ NULL);
+}
+
gboolean
jabber_jid_is_domain(const char *jid)
{
diff --git a/libpurple/protocols/jabber/jutil.h b/libpurple/protocols/jabber/jutil.h
--- a/libpurple/protocols/jabber/jutil.h
+++ b/libpurple/protocols/jabber/jutil.h
@@ -44,12 +44,23 @@ typedef enum {
#include "jabber.h"
JabberID* jabber_id_new(const char *str);
+
+/**
+ * Compare two JIDs for equality.
+ *
+ * Warning: If either JID is NULL then this function uses the user's
+ * bare JID, instead!
+ */
+gboolean jabber_id_equal(JabberStream *js, const JabberID *jid1, const JabberID *jid2);
+
More information about the Commits
mailing list