978564 - JS-XMPP: 'iq' ids are not checked, roster pushes are not verified

Status: NEW

(Chat Core :: XMPP, defect)

Description

[Thijs Alkemade]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

I've been looking into XMPP implementations and whether they verify the source of iq replies. See http://mail.jabber.org/pipermail/jdev/2014-January/089824.html and http://mail.jabber.org/pipermail/jdev/2014-January/089838.html for more discussion.

https://hg.instantbird.org/instantbird/file/1d0601149149/chat/protocols/xmpp/xmpp-session.jsm#l108 appears to contain no code that checks whether the received iq's 'from' attribute matches the expected 'to' attribute. This can lead to spoofing of iq replies (spoofing rosters, vcards, etc.). The fact that ids are incrementing counters makes this very easy to do.

More importantly, https://hg.instantbird.org/instantbird/file/1d0601149149/chat/protocols/xmpp/xmpp.jsm#l805 does not check for roster pushes whether they come from the server or not. Any incoming iq of type 'set' with an <query xmlns='jabber:iq:roster'> child is handled as if it were a push from the server. This makes it possible for anyone to add new contacts to your contact list.

Comment 1

[Patrick Cloke [:clokep]]

Does [1] also imply we should not just be incrementing the ID or is that an additional aspect that makes this insecure?

Thanks for finding this / pointing this out!

[1] http://mail.jabber.org/pipermail/jdev/2014-January/089838.html

Comment 2

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #1)
> Does [1] also imply we should not just be incrementing the ID

We should not. That was an implementation shortcut that I've meant to replace by something more robust for a while, but never got time to actually clean-up.

One thing I don't really know about these ids is how important it is that they are all unique. If we just use a random value, there's a risk some ids may collide.

Comment 3

[Thijs Alkemade]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to Patrick Cloke [:clokep] from comment #1)
> > Does [1] also imply we should not just be incrementing the ID
> 
> We should not. That was an implementation shortcut that I've meant to
> replace by something more robust for a while, but never got time to actually
> clean-up.
> 
> One thing I don't really know about these ids is how important it is that
> they are all unique. If we just use a random value, there's a risk some ids
> may collide.

I agree that incrementing counters in ids are bad. They can reveal how many messages you've send, which may reveal whether you are present or not, even to people not subscribed to your presence. They may even, when someone is looking at the id values of multiple people, give information about who is talking to whom.

However, I don't think uniqueness is quite as important as a concern. In fact, if you play a bit loose with the rules of RFC6120, you really only care that the id is not one for which a reply is already expected.

So in pseudo-code, you could do something like:

do {
    id = random_bytes(8); // Generate an 64 bit random integer
} while (this._handlers[id] != null);

I plan to bring all of this up during the XMPP WG meeting at IETF 89 tomorrow.

Comment 4

[Florian Quèze [:florian]]

(In reply to Thijs Alkemade from comment #3)

> However, I don't think uniqueness is quite as important as a concern. In
> fact, if you play a bit loose with the rules of RFC6120, you really only
> care that the id is not one for which a reply is already expected.

https://tools.ietf.org/html/rfc6120#section-8.1.3 says:
   It is up to the originating entity whether the value of the 'id'
   attribute is unique only within its current stream or unique
   globally.

> So in pseudo-code, you could do something like:
> 
> do {
>     id = random_bytes(8); // Generate an 64 bit random integer
> } while (this._handlers[id] != null);

This makes sense and is easy to implement. My question about uniqueness was more: are we sure that our iq stanzas won't get rejected by servers if the ids collide with ids used hours before?

> I plan to bring all of this up during the XMPP WG meeting at IETF 89
> tomorrow.

Would be excellent to get this point clarified in the spec! :-)

Comment 5

[Thijs Alkemade]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to Thijs Alkemade from comment #3)
> 
> > However, I don't think uniqueness is quite as important as a concern. In
> > fact, if you play a bit loose with the rules of RFC6120, you really only
> > care that the id is not one for which a reply is already expected.
> 
> https://tools.ietf.org/html/rfc6120#section-8.1.3 says:
>    It is up to the originating entity whether the value of the 'id'
>    attribute is unique only within its current stream or unique
>    globally.
> 

I'm going to bring up exactly this part tomorrow, because I think this paragraph is bad. I don't think there's any server out there that cares about the id values you use, they all just pass it along to its destination (there's no reason the server would need to remember your ids from an hour before).

Comment 6

[Florian Quèze [:florian]]

(In reply to Thijs Alkemade from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)

> > https://tools.ietf.org/html/rfc6120#section-8.1.3 says:
> >    It is up to the originating entity whether the value of the 'id'
> >    attribute is unique only within its current stream or unique
> >    globally.
> > 
> 
> I'm going to bring up exactly this part tomorrow, because I think this
> paragraph is bad.

Great. I think this paragraph (or some similar wording in a previous spec) is what led to many of the counter-based implementations you found in http://mail.jabber.org/pipermail/jdev/2014-January/089838.html

Comment 7

[aleth [:aleth]]

Bug(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to Patrick Cloke [:clokep] from comment #1)
> > Does [1] also imply we should not just be incrementing the ID

This part was fixed in bug 1128741, but the remainder of the bug description is still todo.

Comment 8

[aleth [:aleth]]

Attachment: rosterspoof.diff

This adds the check to avoid roster push spoofing.

The to/from check on IQ stanzas is nontrivial due to edge cases and various servers misbehaving, see the link given in the description for details.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8626768 [details] [diff] [review]
rosterspoof.diff

Review of attachment 8626768 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +1205,5 @@
> +        // user's bare JID <user@domainpart>.
> +        let from = aStanza.attributes["from"];
> +        if (from && from != this._jid.node + "@" + this._jid.domain) {
> +          this.WARN("Ignoring potentially spoofed roster push.");
> +          return;

It is unclear to me whether this should be a return or a continue. What is being iterated over from getChildren("query")?

Comment 10

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #9)
> > +        // user's bare JID <user@domainpart>.
> > +        let from = aStanza.attributes["from"];
> > +        if (from && from != this._jid.node + "@" + this._jid.domain) {
> > +          this.WARN("Ignoring potentially spoofed roster push.");
> > +          return;
> 
> It is unclear to me whether this should be a return or a continue. What is
> being iterated over from getChildren("query")?

The iq stanza looks something like
     <iq id='a78b4q6ha463'
          to='[e-mail]/chamber'
          type='set'>
       <query xmlns='jabber:iq:roster'>
         <item jid='[e-mail]'/>
       </query>
     </iq>

In principle it may have multiple query elements.
But if we don't trust one of them, we shouldn't trust the whole stanza.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8626768 [details] [diff] [review]
rosterspoof.diff

(In reply to aleth [:aleth] from comment #10)
> In principle it may have multiple query elements.
> But if we don't trust one of them, we shouldn't trust the whole stanza.

Seems reasonable to me.

Is that the last bit of feedback from this bug?

Comment 12

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #11)
> Is that the last bit of feedback from this bug?

No, there is something left to do here. We should check we are being as careful about incoming iq stanzas as we can be. For ones handled by callbacks, we should be OK due to the improved id values, but iirc the current code doesn't handle everything via callback that it could (and maybe should).

Comment 13

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/f113c4e42ec1

Comment 14

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #12)
> (In reply to Patrick Cloke [:clokep] from comment #11)
> > Is that the last bit of feedback from this bug?
> 
> No, there is something left to do here. We should check we are being as
> careful about incoming iq stanzas as we can be. For ones handled by
> callbacks, we should be OK due to the improved id values, but iirc the
> current code doesn't handle everything via callback that it could (and maybe
> should).

I checked and the current onIQStanza handler (which receives whatever iq stanzas are not handled by callbacks) is now safe.

We could implement from/to checking for callbacks, but as mentioned in [1] it's got potential edge case issues in practice. Libpurple currently uses the following algorithm

 * Verify that the 'from' attribute of an IQ reply is a valid match for
 * a given IQ request. The expected behavior is outlined in section
 * 8.1.2.1 of the XMPP CORE spec (RFC 6120). We consider the reply to
 * be a valid match if any of the following is true:
 * - Request 'to' matches reply 'from' (including the case where
 *   neither are set).
 * - Request 'to' was my JID (bare or full) and reply 'from' is empty.
 * - Request 'to' was empty and reply 'from' is my JID. The spec says
 *   we should only allow bare JID, but we also allow full JID for
 *   compatibility with some servers.
 * - Request 'to' was empty and reply 'from' is server JID. Not allowed by
 *   any spec, but for compatibility with some servers.

The RFC section referenced in this comment does not actually mention client-side checking, so I suppose it's up to us whether we think it's needed, given that our ids are uuids and therefore hard to guess. I have a hard time coming up with a scenario where someone can correctly guess the id of a request we sent out and spoof a reply while not also being able to correctly spoof 'from'.

Thoughts?

Comment 15

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #14)
> I have a
> hard time coming up with a scenario where someone can correctly guess the id
> of a request we sent out and spoof a reply while not also being able to
> correctly spoof 'from'.

It sounds like you'd need to be a MITM to do this, checking "from" would just be an additional check which would be easy to get around. I'm unsure. I don't have strong feelings either way. I'd like Florian to think about this.

Comment 16

[aleth [:aleth]]

ping