Denial of Service Vulnerabilities

[Thijs Alkemade]

On Sat, Mar 16, 2013 at 8:04 PM, Daniel Atallah
<daniel.atallah at gmail.com> wrote:
> This appears to work as advertised - a couple notes:
>
> iq.c:397 - shouldn't this send an error and return?
> The behavior now will be to expose the iq to "jabber-watched-iq" and
> then to send a "feature-not-implemented" error.

Not exactly. It will trigger "jabber-watched-iq", but
"feature-not-implemented" will only be invoked for 'get' or 'set'
iq's. The if-statement on line 379 guarantees this iq is of type
'result' or 'error'.

I think it's fine to pass it to "jabber-watched-iq": any iq with an id
not already in the iq_callbacks-table will be passed to it anyway, and
those are basically the same thing.

Sending and error reply would violate this part of RFC 6120 8.2.3 (4):
"An entity that receives a stanza of type "result" or "error" MUST NOT
respond to the stanza by sending a further IQ response of type
"result" or "error"".

> jabber_id_equal(JabberStream *js, JabberID *jid_in1, JabberID
> *jid_in2) - jid_in1 and jid_in2 should be const
> Behavior of this function is a bit weird - it doesn't seem appropriate
> to assume that it should use the user's jid if one of those is NULL.
> Wouldn't it be more appropriate to explicitly pass the user's jid when
> it should be used for the comparison?

As I noted in the comment there, this takes into account the rules
described in RFC 6120: an outgoing stanza without a 'to', and an
incoming stanza without a 'from' are treated as if coming from "the
server acting on behalf of my own account".

I thought it would be nice to do it this way, as it can basically
directly pass the result of jabber_id_new(xmlnode_get_attrib(...)) in.
It saves allocating new JabberID's to store the user's bare JID (those
aren't available on a JabberStream*).

It could be a bit prettier, true. It does a JID->char*->JID conversion
to go from a full JID to the bare JID, which shouldn't be necessary.
I'll try to improve it.

Thijs