Reject unencapsulated forwarded messages

since we don't support XEP-0297 on its own

CHANGES.md

@@ -3,6 +3,7 @@
 ## 5.0.3 (Unreleased)
 
 - Emit `chatBoxFocused` and `chatBoxBlurred` events for emoji picker input
+- SECURITY FIX: Reject unencapsulated forwarded messages, since we don't support XEP-0297 on its own
 
 ## 5.0.2 (2019-09-11)
 

spec/mam.js

@@ -215,54 +215,54 @@
                         null, ['discoInitialized'], {},
                         async function (done, _converse) {
 
-                        await test_utils.waitForRoster(_converse, 'current', 1);
-                        const contact_jid = mock.cur_names[0].replace(/ /g,'.').toLowerCase() + '@montague.lit';
-                        await test_utils.openChatBoxFor(_converse, contact_jid);
-                        await test_utils.waitUntilDiscoConfirmed(_converse, _converse.bare_jid, null, [Strophe.NS.MAM]);
-                        const sent_IQs = _converse.connection.IQ_stanzas;
-                        const stanza = await u.waitUntil(() => sent_IQs.filter(iq => iq.querySelector(`iq[type="set"] query[xmlns="${Strophe.NS.MAM}"]`)).pop());
-                        const queryid = stanza.querySelector('query').getAttribute('queryid');
-                        let msg = $msg({'id': _converse.connection.getUniqueId(), 'from': 'impersonator@capulet.lit', 'to': _converse.bare_jid})
-                                    .c('result',  {'xmlns': 'urn:xmpp:mam:2', 'queryid':queryid, 'id': _converse.connection.getUniqueId()})
-                                        .c('forwarded', {'xmlns':'urn:xmpp:forward:0'})
-                                            .c('delay', {'xmlns':'urn:xmpp:delay', 'stamp':'2010-07-10T23:08:25Z'}).up()
-                                            .c('message', {
-                                                'xmlns':'jabber:client',
-                                                'to': _converse.bare_jid,
-                                                'id': _converse.connection.getUniqueId(),
-                                                'from': contact_jid,
-                                                'type':'chat'
-                                            }).c('body').t("Meet me at the dance");
-                        spyOn(_converse, 'log');
-                        _converse.connection._dataRecv(test_utils.createRequest(msg));
-                        expect(_converse.log).toHaveBeenCalledWith(`Ignoring alleged MAM message from ${msg.nodeTree.getAttribute('from')}`, Strophe.LogLevel.WARN);
-
-                        msg = $msg({'id': _converse.connection.getUniqueId(), 'to': _converse.bare_jid})
-                                    .c('result',  {'xmlns': 'urn:xmpp:mam:2', 'queryid':queryid, 'id': _converse.connection.getUniqueId()})
-                                        .c('forwarded', {'xmlns':'urn:xmpp:forward:0'})
-                                            .c('delay', {'xmlns':'urn:xmpp:delay', 'stamp':'2010-07-10T23:08:25Z'}).up()
-                                            .c('message', {
-                                                'xmlns':'jabber:client',
-                                                'to': _converse.bare_jid,
-                                                'id': _converse.connection.getUniqueId(),
-                                                'from': contact_jid,
-                                                'type':'chat'
-                                            }).c('body').t("Thrice the brinded cat hath mew'd.");
-                        _converse.connection._dataRecv(test_utils.createRequest(msg));
-
-                        const iq_result = $iq({'type': 'result', 'id': stanza.getAttribute('id')})
-                            .c('fin', {'xmlns': 'urn:xmpp:mam:2'})
-                                .c('set',  {'xmlns': 'http://jabber.org/protocol/rsm'})
-                                    .c('first', {'index': '0'}).t('23452-4534-1').up()
-                                    .c('last').t('09af3-cc343-b409f').up()
-                                    .c('count').t('16');
-                        _converse.connection._dataRecv(test_utils.createRequest(iq_result));
-
-                        const view = _converse.chatboxviews.get(contact_jid);
-                        await new Promise((resolve, reject) => view.once('messageInserted', resolve));
-                        expect(view.model.messages.length).toBe(1);
-                        expect(view.model.messages.at(0).get('message')).toBe("Thrice the brinded cat hath mew'd.");
-                        done();
+                    await test_utils.waitForRoster(_converse, 'current', 1);
+                    const contact_jid = mock.cur_names[0].replace(/ /g,'.').toLowerCase() + '@montague.lit';
+                    await test_utils.openChatBoxFor(_converse, contact_jid);
+                    await test_utils.waitUntilDiscoConfirmed(_converse, _converse.bare_jid, null, [Strophe.NS.MAM]);
+                    const sent_IQs = _converse.connection.IQ_stanzas;
+                    const stanza = await u.waitUntil(() => sent_IQs.filter(iq => iq.querySelector(`iq[type="set"] query[xmlns="${Strophe.NS.MAM}"]`)).pop());
+                    const queryid = stanza.querySelector('query').getAttribute('queryid');
+                    let msg = $msg({'id': _converse.connection.getUniqueId(), 'from': 'impersonator@capulet.lit', 'to': _converse.bare_jid})
+                                .c('result',  {'xmlns': 'urn:xmpp:mam:2', 'queryid':queryid, 'id': _converse.connection.getUniqueId()})
+                                    .c('forwarded', {'xmlns':'urn:xmpp:forward:0'})
+                                        .c('delay', {'xmlns':'urn:xmpp:delay', 'stamp':'2010-07-10T23:08:25Z'}).up()
+                                        .c('message', {
+                                            'xmlns':'jabber:client',
+                                            'to': _converse.bare_jid,
+                                            'id': _converse.connection.getUniqueId(),
+                                            'from': contact_jid,
+                                            'type':'chat'
+                                        }).c('body').t("Meet me at the dance");
+                    spyOn(_converse, 'log');
+                    _converse.connection._dataRecv(test_utils.createRequest(msg));
+                    expect(_converse.log).toHaveBeenCalledWith(`Ignoring alleged MAM message from ${msg.nodeTree.getAttribute('from')}`, Strophe.LogLevel.WARN);
+
+                    msg = $msg({'id': _converse.connection.getUniqueId(), 'to': _converse.bare_jid})
+                                .c('result',  {'xmlns': 'urn:xmpp:mam:2', 'queryid':queryid, 'id': _converse.connection.getUniqueId()})
+                                    .c('forwarded', {'xmlns':'urn:xmpp:forward:0'})
+                                        .c('delay', {'xmlns':'urn:xmpp:delay', 'stamp':'2010-07-10T23:08:25Z'}).up()
+                                        .c('message', {
+                                            'xmlns':'jabber:client',
+                                            'to': _converse.bare_jid,
+                                            'id': _converse.connection.getUniqueId(),
+                                            'from': contact_jid,
+                                            'type':'chat'
+                                        }).c('body').t("Thrice the brinded cat hath mew'd.");
+                    _converse.connection._dataRecv(test_utils.createRequest(msg));
+
+                    const iq_result = $iq({'type': 'result', 'id': stanza.getAttribute('id')})
+                        .c('fin', {'xmlns': 'urn:xmpp:mam:2'})
+                            .c('set',  {'xmlns': 'http://jabber.org/protocol/rsm'})
+                                .c('first', {'index': '0'}).t('23452-4534-1').up()
+                                .c('last').t('09af3-cc343-b409f').up()
+                                .c('count').t('16');
+                    _converse.connection._dataRecv(test_utils.createRequest(iq_result));
+
+                    const view = _converse.chatboxviews.get(contact_jid);
+                    await new Promise((resolve, reject) => view.once('messageInserted', resolve));
+                    expect(view.model.messages.length).toBe(1);
+                    expect(view.model.messages.at(0).get('message')).toBe("Thrice the brinded cat hath mew'd.");
+                    done();
                 }));
 
 

spec/muc_messages.js

@@ -11,6 +11,42 @@
 
     describe("A Groupchat Message", function () {
 
+        it("is rejected if it's an unencapsulated forwarded message",
+            mock.initConverse(
+                null, ['rosterGroupsFetched', 'chatBoxesFetched'], {},
+                async function (done, _converse) {
+
+            const muc_jid = 'lounge@montague.lit';
+            await test_utils.openAndEnterChatRoom(_converse, muc_jid, 'romeo');
+            const impersonated_jid = `${muc_jid}/alice`;
+            const received_stanza = u.toStanza(`
+                <message to='${_converse.jid}' from='${muc_jid}/mallory' type='groupchat' id='${_converse.connection.getUniqueId()}'>
+                    <forwarded xmlns='urn:xmpp:forward:0'>
+                        <delay xmlns='urn:xmpp:delay' stamp='2019-07-10T23:08:25Z'/>
+                        <message from='${impersonated_jid}'
+                                id='0202197'
+                                to='${_converse.bare_jid}'
+                                type='groupchat'
+                                xmlns='jabber:client'>
+                            <body>Yet I should kill thee with much cherishing.</body>
+                        </message>
+                    </forwarded>
+                </message>
+            `);
+            const view = _converse.api.chatviews.get(muc_jid);
+            await view.model.onMessage(received_stanza);
+            spyOn(_converse, 'log');
+            _converse.connection._dataRecv(test_utils.createRequest(received_stanza));
+            expect(_converse.log).toHaveBeenCalledWith(
+                'onMessage: Ignoring unencapsulated forwarded groupchat message',
+                Strophe.LogLevel.WARN
+            );
+            expect(view.el.querySelectorAll('.chat-msg').length).toBe(0);
+            expect(view.model.messages.length).toBe(0);
+            done();
+        }));
+
+
         it("is specially marked when you are mentioned in it",
             mock.initConverse(
                 null, ['rosterGroupsFetched', 'chatBoxesFetched'], {},
@@ -165,7 +201,7 @@
             expect(_converse.log).toHaveBeenCalledWith(
                 'onMessage: Ignoring XEP-0280 "groupchat" message carbon, '+
                 'according to the XEP groupchat messages SHOULD NOT be carbon copied',
-                Strophe.LogLevel.ERROR
+                Strophe.LogLevel.WARN
             );
             expect(view.el.querySelectorAll('.chat-msg').length).toBe(0);
             expect(view.model.messages.length).toBe(0);

src/headless/converse-chatboxes.js

@@ -959,15 +959,14 @@ converse.plugins.add('converse-chatboxes', {
                             stanza.getElementsByTagName(_converse.ACTIVE).length && _converse.ACTIVE ||
                             stanza.getElementsByTagName(_converse.GONE).length && _converse.GONE;
 
-                const is_single_emoji = text ? await u.isSingleEmoji(text) : false;
                 const replaced_id = this.getReplaceId(stanza)
                 const msgid = replaced_id || stanza.getAttribute('id') || original_stanza.getAttribute('id');
                 const attrs = Object.assign({
                     'chat_state': chat_state,
                     'is_archived': this.isArchived(original_stanza),
                     'is_delayed': !!delay,
+                    'is_single_emoji': text ? await u.isSingleEmoji(text) : false,
                     'is_spoiler': !!spoiler,
-                    'is_single_emoji': is_single_emoji,
                     'message': text,
                     'msgid': msgid,
                     'references': this.getReferencesFromStanza(stanza),
@@ -1145,6 +1144,27 @@ converse.plugins.add('converse-chatboxes', {
                 chatbox.messages.create(attrs);
             },
 
+            /**
+             * Reject an incoming message by replying with an error message of type "cancel".
+             * @private
+             * @method _converse.ChatBox#rejectMessage
+             * @param { XMLElement } stanza - The incoming message stanza
+             * @param { XMLElement } text - Text explaining why the message was rejected
+             */
+            rejectMessage (stanza, text) {
+                _converse.api.send(
+                    $msg({
+                        'to': stanza.getAttribute('from'),
+                        'type': 'error',
+                        'id': stanza.getAttribute('id')
+                    }).c('error', {'type': 'cancel'})
+                        .c('not-allowed', {xmlns:"urn:ietf:params:xml:ns:xmpp-stanzas"}).up()
+                        .c('text', {xmlns:"urn:ietf:params:xml:ns:xmpp-stanzas"}).t(text)
+                );
+                _converse.log(`Rejecting message stanza with the following reason: ${text}`, Strophe.LogLevel.WARN);
+                _converse.log(stanza, Strophe.LogLevel.WARN);
+            },
+
             /**
              * Handler method for all incoming single-user chat "message" stanzas.
              * @private
@@ -1152,46 +1172,62 @@ converse.plugins.add('converse-chatboxes', {
              * @param { XMLElement } stanza - The incoming message stanza
              */
             async onMessage (stanza) {
+                const original_stanza = stanza;
                 let to_jid = stanza.getAttribute('to');
                 const to_resource = Strophe.getResourceFromJid(to_jid);
 
                 if (_converse.filter_by_resource && (to_resource && to_resource !== _converse.resource)) {
-                    _converse.log(
+                    return _converse.log(
                         `onMessage: Ignoring incoming message intended for a different resource: ${to_jid}`,
                         Strophe.LogLevel.INFO
                     );
-                    return true;
                 } else if (utils.isHeadlineMessage(_converse, stanza)) {
-                    // XXX: Ideally we wouldn't have to check for headline
-                    // messages, but Prosody sends headline messages with the
+                    // XXX: Prosody sends headline messages with the
                     // wrong type ('chat'), so we need to filter them out here.
-                    _converse.log(
+                    return _converse.log(
                         `onMessage: Ignoring incoming headline message from JID: ${stanza.getAttribute('from')}`,
                         Strophe.LogLevel.INFO
                     );
-                    return true;
                 }
 
-                let is_carbon = false;
-                const forwarded = stanza.querySelector('forwarded');
-                const original_stanza = stanza;
+                const bare_forward = sizzle(`message > forwarded[xmlns="${Strophe.NS.FORWARD}"]`, stanza).length;
+                if (bare_forward) {
+                    return this.rejectMessage(
+                        stanza,
+                        'Forwarded messages not part of an encapsulating protocol are not supported'
+                    );
+                }
+                let from_jid = stanza.getAttribute('from') || _converse.bare_jid;
+                const is_carbon = u.isCarbonMessage(stanza);
+                if (is_carbon) {
+                    if (from_jid === _converse.bare_jid) {
+                        const selector = `[xmlns="${Strophe.NS.CARBONS}"] > forwarded[xmlns="${Strophe.NS.FORWARD}"] > message`;
+                        stanza = sizzle(selector, stanza).pop();
+                        to_jid = stanza.getAttribute('to');
+                        from_jid = stanza.getAttribute('from');
+                    } else {
+                        // Prevent message forging via carbons: https://xmpp.org/extensions/xep-0280.html#security
+                        return this.rejectMessage(stanza, 'Rejecting carbon from invalid JID');
+                    }
+                }
 
-                if (forwarded !== null) {
-                    const xmlns = Strophe.NS.CARBONS;
-                    is_carbon = sizzle(`received[xmlns="${xmlns}"]`, original_stanza).length > 0;
-                    if (is_carbon && original_stanza.getAttribute('from') !== _converse.bare_jid) {
-                        // Prevent message forging via carbons
-                        // https://xmpp.org/extensions/xep-0280.html#security
-                        return true;
+                const is_mam = u.isMAMMessage(stanza);
+                if (is_mam) {
+                    if (from_jid === _converse.bare_jid) {
+                        const selector = `[xmlns="${Strophe.NS.MAM}"] > forwarded[xmlns="${Strophe.NS.FORWARD}"] > message`;
+                        stanza = sizzle(selector, stanza).pop();
+                        to_jid = stanza.getAttribute('to');
+                        from_jid = stanza.getAttribute('from');
+                    } else {
+                        return _converse.log(
+                            `onMessage: Ignoring alleged MAM message from ${stanza.getAttribute('from')}`,
+                            Strophe.LogLevel.WARN
+                        );
                     }
-                    stanza = forwarded.querySelector('message');
-                    to_jid = stanza.getAttribute('to');
                 }
 
-                const from_jid = stanza.getAttribute('from');
                 const from_bare_jid = Strophe.getBareJidFromJid(from_jid);
                 const is_me = from_bare_jid === _converse.bare_jid;
-
                 if (is_me && to_jid === null) {
                     return _converse.log(
                         `Don't know how to handle message stanza without 'to' attribute. ${stanza.outerHTML}`,
@@ -1211,7 +1247,6 @@ converse.plugins.add('converse-chatboxes', {
                 const chatbox = this.getChatBox(contact_jid, {'nickname': roster_nick}, has_body);
 
                 if (chatbox) {
-                    const is_mam = sizzle(`message > result[xmlns="${Strophe.NS.MAM}"]`, original_stanza).length > 0;
                     const message = await chatbox.getDuplicateMessage(stanza);
                     if (message) {
                         chatbox.updateMessage(message, original_stanza);

src/headless/converse-muc.js

@@ -1536,22 +1536,37 @@ converse.plugins.add('converse-muc', {
              */
             async onMessage (stanza) {
                 const original_stanza = stanza;
-                const is_carbon = sizzle(`received[xmlns="${Strophe.NS.CARBONS}"]`, original_stanza).length > 0;
+                const bare_forward = sizzle(`message > forwarded[xmlns="${Strophe.NS.FORWARD}"]`, stanza).length;
+                if (bare_forward) {
+                    return _converse.log(
+                        'onMessage: Ignoring unencapsulated forwarded groupchat message',
+                        Strophe.LogLevel.WARN
+                    );
+                }
+                const is_carbon = u.isCarbonMessage(stanza);
                 if (is_carbon) {
                     // XEP-280: groupchat messages SHOULD NOT be carbon copied, so we're discarding it.
-                    _converse.log(
+                    return _converse.log(
                         'onMessage: Ignoring XEP-0280 "groupchat" message carbon, '+
                         'according to the XEP groupchat messages SHOULD NOT be carbon copied',
-                        Strophe.LogLevel.ERROR);
-                    return;
+                        Strophe.LogLevel.WARN
+                    );
                 }
+                const is_mam = u.isMAMMessage(stanza);
+                if (is_mam) {
+                    if (original_stanza.getAttribute('from') === this.get('jid')) {
+                        const selector = `[xmlns="${Strophe.NS.MAM}"] > forwarded[xmlns="${Strophe.NS.FORWARD}"] > message`;
+                        stanza = sizzle(selector, stanza).pop();
+                    } else {
+                        return _converse.log(
+                            `onMessage: Ignoring alleged MAM groupchat message from ${stanza.getAttribute('from')}`,
+                            Strophe.LogLevel.WARN
+                        );
+                    }
+                }
+
                 this.createInfoMessages(stanza);
                 this.fetchFeaturesIfConfigurationChanged(stanza);
-                const forwarded = sizzle(`forwarded[xmlns="${Strophe.NS.FORWARD}"]`, stanza).pop();
-
-                if (forwarded) {
-                    stanza = forwarded.querySelector('message');
-                }
 
                 const message = await this.getDuplicateMessage(original_stanza);
                 if (message) {

src/headless/utils/core.js

@@ -50,6 +50,16 @@ u.toStanza = function (string) {
     return node.firstElementChild;
 }
 
+u.isMAMMessage = function (stanza) {
+    return sizzle(`message > result[xmlns="${Strophe.NS.MAM}"]`, stanza).length > 0;
+}
+
+u.isCarbonMessage = function (stanza) {
+    const xmlns = Strophe.NS.CARBONS;
+    return sizzle(`message > received[xmlns="${xmlns}"]`, stanza).length > 0 ||
+            sizzle(`message > sent[xmlns="${xmlns}"]`, stanza).length > 0;
+}
+
 u.getLongestSubstring = function (string, candidates) {
     function reducer (accumulator, current_value) {
         if (string.startsWith(current_value)) {