search

psi-plus / main

Main repository with patches and required resources
https://psi-plus.com/
GNU Lesser General Public License v2.1
68 stars 20 forks source link

Accidential deletion of account #671

Closed cdroege closed 7 years ago

cdroege commented 7 years ago

After an upgrade to ejabberd 16.12 Psi and Psi+ can delete the current account on the server under certain circumstances:

  1. Add a contact with an invalid JID like: ""
  2. Delete the roster item added in step 1
  3. Account is deleted, when the server is running 16.12 (but not 16.09). See the attached logs

I found 2 things, that are different in ejabberd 16.12 compared to 16.09:

  1. The server replies to the roster item deletion request with a different error
  2. The roster items do not always contain a ask element (this is not in the attached logs). Although it is recommended in the RFC, it does not have to contain this element.

My guess is: The escaping of the JID in Psi does not work correctly (see first iq stanza in both logs), if the server does not send the ask element with the roster items. Then Psi will send a unregister request without any to element, so the server will receive the unregister request and the server will delete the account.

XML log of ejabberd 16.12

[...]
<iq type="set" id="aacba">
<query xmlns="jabber:iq:roster">
<item subscription="remove" jid=""/>
</query>
</iq>

<iq type="get" id="aacda">
<query xmlns="jabber:iq:register"/>
</iq>

<iq from="asdj89ah4234ioiasmd@draugr.de" type="error" xml:lang="en" to="asdj89ah4234ioiasmd@draugr.de/Psi+" id="aacba">
<query xmlns="jabber:iq:roster">
<item subscription="remove" jid=""/>
</query>
<error type="modify" code="400">
<bad-request xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/>
<text xmlns="urn:ietf:params:xml:ns:xmpp-stanzas" xml:lang="en">Bad value of attribute 'jid' in tag &lt;item/&gt; qualified by namespace 'jabber:iq:roster'</text>
</error>
</iq>

<r xmlns="urn:xmpp:sm:3"/>

<a xmlns="urn:xmpp:sm:3" h="24"/>

<iq from="asdj89ah4234ioiasmd@draugr.de" type="result" xml:lang="en" to="asdj89ah4234ioiasmd@draugr.de/Psi+" id="aacda">
<query xmlns="jabber:iq:register">
<username>asdj89ah4234ioiasmd</username>
<registered/>
<password/>
<instructions>Choose a username and password to register with this server</instructions>
</query>
</iq>

<iq type="set" to="draugr.de" id="aacda">
<query xmlns="jabber:iq:register">
<remove/>
</query>
</iq>

<r xmlns="urn:xmpp:sm:3"/>

<a xmlns="urn:xmpp:sm:3" h="25"/>

<iq from="asdj89ah4234ioiasmd@draugr.de/Psi+" type="result" xml:lang="en" to="asdj89ah4234ioiasmd@draugr.de/Psi+" id="aacda"/>

<r xmlns="urn:xmpp:sm:3"/>

<a xmlns="urn:xmpp:sm:3" h="26"/>

<stream:error>
<conflict xmlns="urn:ietf:params:xml:ns:xmpp-streams"/>
<text xmlns="urn:ietf:params:xml:ns:xmpp-streams" xml:lang="en">User removed</text>
</stream:error>

<iq type="set" id="aacea">
<enable xmlns="urn:xmpp:carbons:2"/>
</iq>

XML log of ejabberd 16.09

<iq type="set" id="aadca">
<query xmlns="jabber:iq:roster">
<item subscription="remove" jid="&quot;&quot;"/>
</query>
</iq>

<iq type="get" to="&quot;&quot;" id="aadea">
<query xmlns="jabber:iq:register"/>
</iq>

<iq from="&quot;&quot;" type="error" xml:lang="en" to="asd90jas8dhasgd8as@creep.im/Psi+" id="aadea">
<query xmlns="jabber:iq:register"/>
<error type="cancel" code="404">
<remote-server-not-found xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/>
</error>
</iq>

<iq type="set" to="&quot;&quot;" id="aadea">
<query xmlns="jabber:iq:register">
<remove/>
</query>
</iq>

<r xmlns="urn:xmpp:sm:3"/>

<a xmlns="urn:xmpp:sm:3" h="9"/>

<r xmlns="urn:xmpp:sm:3"/>

<a xmlns="urn:xmpp:sm:3" h="12"/>

<iq from="asd90jas8dhasgd8as@creep.im" type="set" to="asd90jas8dhasgd8as@creep.im/Psi+" id="push12081735906807766993">
<query xmlns="jabber:iq:roster" ver="fbd91abd5446765958c73bd79c204adcad9f9420">
<item subscription="remove" ask="subscribe" jid="&quot;&quot;"/>
</query>
</iq>

<iq type="result" to="asd90jas8dhasgd8as@creep.im" id="push12081735906807766993"/>

<iq from="asd90jas8dhasgd8as@creep.im" type="result" to="asd90jas8dhasgd8as@creep.im/Psi+" id="aadca"/>

<iq from="&quot;&quot;" type="error" xml:lang="en" to="asd90jas8dhasgd8as@creep.im/Psi+" id="aadea">
<query xmlns="jabber:iq:register">
<remove/>
</query>
<error type="cancel" code="404">
<remote-server-not-found xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/>
</error>
</iq>
Ri0n commented 7 years ago

Well according to xep-0106 escaping should be different. I believe we have another task for this. But server's behavior does not seem to be correct too.

Thanks for the report.

zinid commented 7 years ago

@Ri0n why the server's behaviour is incorrect?

Ri0n commented 7 years ago

@zinid well according to server logs difference, Psi sends these weird incorrectly escaped quotes and server silently removes them. I think the server should either drop connection or return some error, or at least keep quotes.

zinid commented 7 years ago

See the logs from 16.12, the server send 'bad-request' error with graceful explanation. Befor 16.12 - yes, the behaviour was incorrect.

Ri0n commented 7 years ago

Ah yes. I see now. I don't know how I haven't noticed this for the first time. So we have two bugs in Psi. incorrect escaping and incorrect errors handling.

zinid commented 7 years ago

Please consider fixing this ASAP, as this is a critical bug: on some servers users get unregistered massively.

Ri0n commented 7 years ago

I can release a new version but it will take time for users to update anyway.

zinid commented 7 years ago

I obviously understand users will not update their clients instantly. However, this is not an excuse not to fixing the bug :)

Ri0n commented 7 years ago

fixed in b9a5f341519c4151cae5534e9125c75abcc091e2