fix iq spoofing

[sstrigler]

Save 'to' attribute when sending iqs along with 'id' and check against 'from' when receiving and handling reply.

Addresses issues discussed at http://mail.jabber.org/pipermail/jdev/2014-January/089831.html

untested

[valeriansaliou]

We are highly interested in this patch being merged to JSJaC for Jappix security purpose ;)

[sstrigler]

It's not done yet but hope to be able to finish it on Friday. Can you help testing it?

Am 05.02.2014 um 20:02 schrieb Valérian Saliou notifications@github.com:

We are highly interested in this patch being merged to JSJaC for Jappix security purpose ;)

— Reply to this email directly or view it on GitHub.

[valeriansaliou]

Sure, I'm happy to test that! Just let me know once you got something testable.

[sstrigler]

updated pull request, please test! (didn't do any testing, sorry)

[valeriansaliou]

Thanks, will submit this to Jappix @master for tests ;)

[valeriansaliou]

Okay, just updated Jappix's JSJaC.js code. Works fine overall, but it seems that some responses cannot pass through properly. Seems that those which don't pass are the ones coming either from the server or the user's JID.

Maybe you could simply ignore this process if a response doesn't contain any 'from' attribute or is coming from the current user? If you test Jappix@master, you'll see that your avatar doesn't get retrieved properly due to the XMPP private XML storage query to timeout since JSJaC rejects response.

[valeriansaliou]

Okay, forget about private XML storage, the reason is the following: the PubSub service of Metronome/Proosdy doesn't reply with a 'from' attribute when stanza is from the connected user's JID.

See it by yourself (I added an error log for any rejected packet, you should do so):

ERROR >> rejecting unknown response with id 6 3 
TRACE >> <iq xmlns="jabber:client" id="6" type="result" to="jappix@jappix.com/Jappix (1395512368661)"><pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="urn:xmpp:microblog:0"><item xmlns="http://jabber.org/protocol/pubsub" id="3bb884b1dedd081131b5ee372572e981"><entry xmlns="http://www.w3.org/2005/Atom"><title/><author><name>Jappix</name><uri>xmpp:jappix@jappix.com</uri></author><content type="text">Hello World!</content><published>2014-01-15T08:55:07Z</published><updated>2014-01-15T08:55:07Z</updated><link rel="alternate" href="xmpp:jappix@jappix.com?;node=urn%3Axmpp%3Amicroblog%3A0;item=3bb884b1dedd081131b5ee372572e981"/><link rel="replies" title="comments" href="xmpp:pubsub.jappix.com?;node=urn%3Axmpp%3Amicroblog%3A0%3Acomments%2F3bb884b1dedd081131b5ee372572e981"/><geoloc xmlns="http://jabber.org/protocol/geoloc"><lat>48.3807678</lat><lon>-4.6165874</lon><country>France</country><countrycode>FR</countrycode><region>Bretagne</region><postalcode>29280</postalcode><locality>Plouzané</locality><street>Allée du Lourc'h</street><text>Allée du Lourc'h, 29280 Plouzané, France</text><uri>http://maps.google.com/?q=48.3807678,-4.6165874</uri></geoloc></entry></item></items></pubsub></iq>

[sstrigler]

22 mar 2014 kl. 19:22 skrev Valérian Saliou notifications@github.com:

Okay, forget about private XML storage, the reason is the following: the PubSub service of Metronome/Proosdy doesn't reply with a 'from' attribute when stanza is from the connected user's JID.

What did the corresponding request ('get'?) look like? See it by yourself (I added an error log for any rejected packet, you should do so):

Ok. Good point.

[valeriansaliou]

Well it was simply a request sent to the server without any 'to' attribute, thus meaning in my case to='valerian@jappix.com'.

Prosody doesn't reply with any 'from' attribute in the case we request something to the user's JID. Thus, no 'from' is equivalent to the bare JID 'valerian@jappix.com'. Hence why JSJaC cannot find the according registered stanza ID, since it doesn't know the JID.

You just need to do:

var jid = packet.getFrom() || (con.username + '@' + con.server);

[sstrigler]

I still don't see where things go wrong. When registering the handler I do

var jid = this.getTo() || this.domain;

In your case it would get set to 'this.domain', right? As the 'to' is missing. When receiving I'm doing

var jid = this.getFrom() || this.domain;

And should result in the same behavior. But probably we need a more tolerant handling here anyway but I don't see how it is related to your case.

With more tolerant I mean that from='localhost' and from='me@localhost' should probably map to the same. Need to read upon this...

[sstrigler]

I've changed both now to map to 'this.jid'.

[valeriansaliou]

Mhh yeah I know, that's weird. Since I didn't check the request stanza, I suspect the request to hold the user JID in the 'to' attribute, but the response doesn't contain any 'from' attribute so JSJaC defaulted to the server domain, which doesn't match the emitter 'to' attribute, obviously.

Anyway, now works fine with the "|| this.jid" change. Consider that this was the correct fallback. The only case where the 'from' attribute could be empty was if it matched the user's JID, and not server's domain (as you previously defined). If the response is from the server's domain, the 'from' attribute will always be filled with a value (it's in the XMPP specs).

I think this is now safe to be merged ;)

[sstrigler]

thanks!