1806631 - XMPP implementation incorrectly addresses IQ Ping responses

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

Steps to reproduce:

To detect if the Thundebird XMPP client is still connected, a server or service can send it a 'ping' request, as specified in [XEP-0199: XMPP Ping] (https://xmpp.org/extensions/xep-0199.html)

Thunderbird supports this protocol. However, when generating a response, it incorrectly addresses the response. Instead of addressing the response to the originator of the request, it addresses it to the server that Thunderbird is connected to.

When it was the server that issued the request, then this will work as expected. However, if it is any entity (including other clients, and other services), then the requester will never receive a response.

This appears to be a simple oversight in the code. The handling of the request is very similar to handling "Version" requests, that correctly addresses the response.

This issue is related to, but not a duplicate of bug 1806630

Comment 1

[guus.der.kinderen]

Attachment: Patch with suggested fix for bug 1806631

In a slight deviation from the description of this bug, this patch will not simply respond to any IQ ping request. Instead, it will honor the pre-existing limitation that only servers can ping the client (I assume that this is a privacy-related limitation).

With the provided patch, the processing of requests from other sources falls back to the default handling of non-processed IQ requests. Note that the patch provided for bug 1806631 applies a modification to that code.

Comment 2

[Patrick Cloke [:clokep]]

(In reply to guus.der.kinderen from comment #1)

Instead, it will honor the pre-existing limitation that only servers can ping the client (I assume that this is a privacy-related limitation).

This seems legal according to XEP-0199 section 7:

If a connected resource receives a ping request but it does not want to reveal its network availability to the sender for any reason (e.g., because the sender is not authorized to know the connected resource's availability), then it too MUST reply with a <service-unavailable/> error.

I'm not sure I feel strongly about whether we should respond to the client-to-client ping or not. We do for IRC. Bug 1128741 added the if-statement to check against the server name (see "Part 2" / attachment 8559169 [details] [diff] [review]), but there's not really any conversation on there for why. Martin -- any opinions?

Comment 3

[guus.der.kinderen]

It is probably not right that the processing of IQ Ping and IQ Software Version are acting differently, with regards to who's allowed to perform the query. If the reason to limit that functionality is indeed privacy (I can't see another reason), then it makes little sense to differentiate between IQ Ping, IQ Software Version, and others.

Each of these functionalities should probably be responding (and not responding) to the same collection of peers. Peers that can be allowed to receive responses should probably include:

• the service that the client is connecting to (eg: example.org) which is what Ping is currently doing.
• any chatrooms that the client joined (eg: [e-mail])
• anyone that is subscribed to the presence of the user (people 'on your roster').

This quickly gets a lot more complicated than the original issue though.

Comment 4

[Martin Giger [:freaktechnik]]

I would generally be in favor of doing the least complex solution. In my opinion the XMPP code is already fairly fragile. I'm also afraid that there will always be a way to work around the ping block by using some other part of the protocol. While in IRC the client-to-client communication is just for some "silly extras nobody uses", from my understanding it's much more core to XMPP.

bug 978564 seems potentially related since it's also about IQ handling.

Comment 5

[guus.der.kinderen]

If you'd want to go with the least complex option (and allow for some leakage that can be used to see the presence information of a user), then we could drop the address check in the 'Ping' handling. Simply respond to every 'Ping' request, no matter the requester. That's in line with the 'Version' handling anyway.

I'd not consider #978564 to be closely related to this, as that described a mechanism to avoid spoofing. #978564 Focuses on 'who do we accept an answer from', while the root decision in this issue is: 'who do we allow to ask us a question.' It makes sense to me to tackle both issues separately.

Comment 6

[Patrick Cloke [:clokep]]

(In reply to guus.der.kinderen from comment #5)

If you'd want to go with the least complex option (and allow for some leakage that can be used to see the presence information of a user), then we could drop the address check in the 'Ping' handling. Simply respond to every 'Ping' request, no matter the requester. That's in line with the 'Version' handling anyway.

Sorry for the delay in responding -- I think this makes sense to do for now (drop the server check and be consistent with version queries). If we're going to tighten this for privacy reasons we should do it holistically and ensure it works.

Comment 7

[guus.der.kinderen]

(In reply to Patrick Cloke [:clokep] from comment #6)

Sorry for the delay in responding -- I think this makes sense to do for now (drop the server check and be consistent with version queries). If we're going to tighten this for privacy reasons we should do it holistically and ensure it works.

No problem. Do you need a new patch? The Ubuntu-snap based HG instance on my laptop and I are not great friends. If you wouldn't mind munging that one if-statement that's there manually, that would have my preference.

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Updated patch which changes to use the proper from field and responding to all ping queries.

Hopefully this looks reasonable, I didn't see any issues while running this patch.

Comment 9

[guus.der.kinderen]

That looks good to me. Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/495f4e71ec5e
Properly respond to XMPP Ping IQs. r=clokep

Comment 11

[Patrick Cloke [:clokep]]

Note that I've requested uplift of bug 1806630, but I don't think the impact of this bug is enough to uplift to ESR. Please shout if folks disagree, however.