Ejabberd: XEP-0033 Extended Stanza Addressing - Incorrect behaviour with BCC and child Address elements

Created on 5 Jul 2021 · 4Comments · Source: processone/ejabberd

Environment

ejabberd version: 18.09 (Problematic code still present in master)
Erlang version: erl +V
OS: Linux (Ubuntu 18.04)
Installed from: ejabberd/ecs:18.09

Bug description

XEP-0033 States that an address with type="bcc" should not be stripped when its delivered to the bcc user.
https://xmpp.org/extensions/xep-0033.html#addr-type-bcc

These addressees should receive 'blind carbon copies' of the stanza. This means that the server MUST remove these addresses before the stanza is delivered to anyone _other_ than the given bcc addressee or the multicast service of the bcc addressee.

This is to facilitate extending the address element with additional information (Although this is only a SHOULD not a MUST)
https://xmpp.org/extensions/xep-0033.html#extensibility

As specified herein, the

However ejabberd appears to destroy the entire addresses element, and reconstructs it, without the child elements and stripping the bcc addresses, even when it's being sent to the bcc addressee.

https://github.com/processone/ejabberd/blob/e3e4dae58394446189beeb782852c31f24a6976a/src/mod_multicast.erl#L359
https://github.com/processone/ejabberd/blob/e3e4dae58394446189beeb782852c31f24a6976a/src/mod_multicast.erl#L548

Multicast

Source

McPo

❤1

All 4 comments

The protocol is vague: when there are several BCC destinations, should all of them receive all the BCC address elements (a true carbon copy of the original stanza), or only his own BCC element added (each BCC destination receives a different list of addresses)?

Anyway, what's the benefit in receiving oneself BCC...

Right, the Addresses element is parsed according to the XEP specification, and removed from the stanza. It is later built with the required attributes. As a side effect, that disallows Extensibility... if somebody would really use it.

badlop on 6 Jul 2021

> This means that the server MUST remove these addresses before the stanza is delivered to anyone other than the given bcc addressee
>Each 'bcc' recipient MUST receive only the

I believe the standard is actually fairly clear about this given the above statements.

The standard IMO is clearly stating that a BCC address should only be included, when the message is being delivered to that specific recipient, otherwise whats the point of BCC, and they wouldn't have mentioned this if they intended for all BCC addresses to always be stripped (Which is the current behaviour).

One reason for extensibility is it allows for unique information related to a message to be given to each recipient and for them to know what information relates to them. For example

Imagine a system for distributing employee shift times. You'd be able to reference the time each employee is working that day inside a child element of address.
Another example is a message referencing a URL, you could provide different access tokens for each recipient. Combining this with BCC functionality, would result in the tokens used for the other recipients being stripped.
The reason Im requesting it is for E2EE KeyBag functionality. Essentially we plan on symmetrically encrypting a message body, then encrypting that key with the E2EE key for each recipient. That encrypted key would then be placed as a child element of address, and using BCC the other keys would be stripped.

Re-using the address element has the benefit of reducing stanza size (which could get large in the case of many recipients), it also has the benefit of BCC addresses being stripped for extra "privacy".

It could be argued (if pedantic enough 🙃), that in the extensibility section, while its says that future protocols/implementations MAY extend the address element, they MUST be ignored if not understood by the implementation, and thus ejabberd is breaking standards by stripping and not ignoring the subelements.....

I intend to TRY and work on this (my erlang is poor), would this likely be accepted as a PR? I have discovered the issue with the extensibility section resides within the xmpp library, any fix I implement is unlikely to touch that library, instead I may pass the child element alongside the address element.

McPo on 6 Jul 2021

❤1

Ill get a PR created once Im satisfied theres no obvious knock on effects, however the following patch appears to work as expected.

diff --git a/modules/src/mod_multicast.erl b/modules/src/mod_multicast.erl
index cf639e5..c5d91ea 100644
--- a/modules/src/mod_multicast.erl
+++ b/modules/src/mod_multicast.erl
@@ -353,9 +353,9 @@ perform(From, Packet, _,
        {route_single, Group}) ->
     lists:foreach(
        fun(ToUser) ->
-
+           Group_others = strip_other_bcc(ToUser, Group#group.others),
            route_packet(From, ToUser, Packet,
-                        Group#group.others, Group#group.addresses)
+                        Group_others, Group#group.addresses)
        end, Group#group.dests);
 perform(From, Packet, _,
        {{route_multicast, JID, RLimits}, Group}) ->
@@ -389,6 +389,16 @@ strip_addresses_element(Packet) ->
            throw(eadsele)
     end.

+strip_other_bcc(#dest{jid_jid = ToUserJid}, Group_others) ->
+    lists:filter(
+      fun(#address{jid = JID, type = Type}) ->
+             case {JID, Type} of
+                 {ToUserJid, bcc} -> true;
+                 {_, bcc} -> false;
+                 _ -> true
+             end
+      end, Group_others).
+
 %%%-------------------------
 %%% Split Addresses
 %%%-------------------------
@@ -508,7 +518,6 @@ build_other_xml(Dests) ->
                        case Dest#dest.type of
                          to -> [add_delivered(XML) | R];
                          cc -> [add_delivered(XML) | R];
-                         bcc -> R;
                          _ -> [XML | R]
                        end
                end,

Now gonna try and make the required changes to have the address record maintains its child elements.

McPo on 6 Jul 2021

❤1

Created the following PRs: