From f9eba3df76661f6cdfe792a9728d296e3c7b0aa4 Mon Sep 17 00:00:00 2001 From: Sonny Piers Date: Fri, 10 Jan 2025 14:29:27 +0100 Subject: [PATCH] client: Do not allow PLAIN on insecure connection Also add connection.isSecure() method Fixes https://github.com/xmppjs/xmpp.js/issues/1040 --- packages/client-core/lib/Client.js | 2 +- packages/client-core/test/Client.js | 9 +++-- packages/client/README.md | 15 ++++++++ packages/client/lib/createOnAuthenticate.js | 32 ++++++++++------- packages/client/test/getMechanism.js | 38 +++++++++++++++++++++ packages/connection/index.js | 4 +++ packages/connection/test/isSecure.js | 14 ++++++++ packages/sasl/index.js | 2 +- packages/sasl2/index.js | 7 +++- packages/test/mockSocket.js | 3 ++ rollup.config.js | 1 + 11 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 packages/client/test/getMechanism.js create mode 100644 packages/connection/test/isSecure.js diff --git a/packages/client-core/lib/Client.js b/packages/client-core/lib/Client.js index bd9a18a7..b0bdcbc3 100644 --- a/packages/client-core/lib/Client.js +++ b/packages/client-core/lib/Client.js @@ -47,7 +47,7 @@ class Client extends Connection { // after the confidentiality and integrity of the stream are protected via TLS // or an equivalent security layer. // https://xmpp.org/rfcs/rfc6120.html#rfc.section.4.7.1 - const from = this.socket?.isSecure() && this.jid?.bare().toString(); + const from = this.isSecure() && this.jid?.bare().toString(); if (from) headerElement.attrs.from = from; return this.Transport.prototype.header(headerElement, ...args); } diff --git a/packages/client-core/test/Client.js b/packages/client-core/test/Client.js index e510e9cb..77913608 100644 --- a/packages/client-core/test/Client.js +++ b/packages/client-core/test/Client.js @@ -32,21 +32,20 @@ test("header", () => { const entity = new Client(); entity.Transport = Transport; - entity.socket = {}; entity.jid = null; - entity.socket.isSecure = () => false; + entity.isSecure = () => false; expect(entity.header()).toEqual(); entity.jid = null; - entity.socket.isSecure = () => true; + entity.isSecure = () => true; expect(entity.header()).toEqual(); entity.jid = new JID("foo@bar/example"); - entity.socket.isSecure = () => false; + entity.isSecure = () => false; expect(entity.header()).toEqual(); entity.jid = new JID("foo@bar/example"); - entity.socket.isSecure = () => true; + entity.isSecure = () => true; expect(entity.header()).toEqual(); }); diff --git a/packages/client/README.md b/packages/client/README.md index 2ac26d1f..f3a61d35 100644 --- a/packages/client/README.md +++ b/packages/client/README.md @@ -265,6 +265,21 @@ Returns a promise that resolves once all the stanzas have been sent. If you need to send a stanza to multiple recipients we recommend using [Extended Stanza Addressing](https://xmpp.org/extensions/xep-0033.html) instead. +### isSecure + +Returns whether the connection is considered secured. + +```js +console.log(xmpp.isSecure()); +``` + +Considered secure: + +- localhost, 127.0.0.1, ::1 +- encrypted channels (wss, xmpps, starttls) + +This method returns false if there is no connection. + ### xmpp.reconnect See [@xmpp/reconnect](/packages/reconnect). diff --git a/packages/client/lib/createOnAuthenticate.js b/packages/client/lib/createOnAuthenticate.js index 907a180b..8d8c4ea1 100644 --- a/packages/client/lib/createOnAuthenticate.js +++ b/packages/client/lib/createOnAuthenticate.js @@ -1,23 +1,31 @@ const ANONYMOUS = "ANONYMOUS"; +const PLAIN = "PLAIN"; export default function createOnAuthenticate(credentials, userAgent) { - return async function onAuthenticate(authenticate, mechanisms, fast) { + return async function onAuthenticate(authenticate, mechanisms, fast, entity) { if (typeof credentials === "function") { await credentials(authenticate, mechanisms, fast); return; } - if ( - !credentials?.username && - !credentials?.password && - mechanisms.includes(ANONYMOUS) - ) { - await authenticate(credentials, ANONYMOUS, userAgent); - return; - } + credentials.token ??= await fast?.fetch(); - credentials.token = await fast?.fetch?.(); - - await authenticate(credentials, mechanisms[0], userAgent); + const mechanism = getMechanism({ mechanisms, entity, credentials }); + await authenticate(credentials, mechanism, userAgent); }; } + +export function getMechanism({ mechanisms, entity, credentials }) { + if ( + !credentials?.username && + !credentials?.password && + !credentials?.token && + mechanisms.includes(ANONYMOUS) + ) { + return ANONYMOUS; + } + + if (entity.isSecure()) return mechanisms[0]; + + return mechanisms.find((mechanism) => mechanism !== PLAIN); +} diff --git a/packages/client/test/getMechanism.js b/packages/client/test/getMechanism.js new file mode 100644 index 00000000..36deac0d --- /dev/null +++ b/packages/client/test/getMechanism.js @@ -0,0 +1,38 @@ +import { getMechanism } from "../lib/createOnAuthenticate.js"; + +it("returns ANONYMOUS if available and there are no credentials", () => { + expect( + getMechanism({ + credentials: {}, + mechanisms: ["PLAIN", "ANONYMOUS"], + }), + ).toBe("ANONYMOUS"); +}); + +it("returns the first mechanism if the connection is secure", () => { + expect( + getMechanism({ + credentials: { username: "foo", password: "bar" }, + mechanisms: ["PLAIN", "SCRAM-SHA-1"], + entity: { isSecure: () => true }, + }), + ).toBe("PLAIN"); +}); + +it("does not return PLAIN if the connection is not secure", () => { + expect( + getMechanism({ + credentials: { username: "foo", password: "bar" }, + mechanisms: ["PLAIN", "SCRAM-SHA-1"], + entity: { isSecure: () => false }, + }), + ).toBe("SCRAM-SHA-1"); + + expect( + getMechanism({ + credentials: { username: "foo", password: "bar" }, + mechanisms: ["PLAIN"], + entity: { isSecure: () => false }, + }), + ).toBe(undefined); +}); diff --git a/packages/connection/index.js b/packages/connection/index.js index 93c4d3c7..e641b133 100644 --- a/packages/connection/index.js +++ b/packages/connection/index.js @@ -22,6 +22,10 @@ class Connection extends EventEmitter { this.root = null; } + isSecure() { + return !!this.socket?.isSecure(); + } + async _streamError(condition, children) { try { await this.send( diff --git a/packages/connection/test/isSecure.js b/packages/connection/test/isSecure.js new file mode 100644 index 00000000..e76571d5 --- /dev/null +++ b/packages/connection/test/isSecure.js @@ -0,0 +1,14 @@ +import Connection from "../index.js"; + +test("isSecure()", () => { + const conn = new Connection(); + + conn.socket = null; + expect(conn.isSecure()).toBe(false); + + conn.socket = { isSecure: () => false }; + expect(conn.isSecure()).toBe(false); + + conn.socket = { isSecure: () => true }; + expect(conn.isSecure()).toBe(true); +}); diff --git a/packages/sasl/index.js b/packages/sasl/index.js index 7c47312a..0fad0407 100644 --- a/packages/sasl/index.js +++ b/packages/sasl/index.js @@ -84,7 +84,7 @@ export default function sasl({ streamFeatures, saslFactory }, onAuthenticate) { }); } - await onAuthenticate(done, mechanisms); + await onAuthenticate(done, mechanisms, null, entity); await entity.restart(); }); diff --git a/packages/sasl2/index.js b/packages/sasl2/index.js index d81ddfed..f6d2c86a 100644 --- a/packages/sasl2/index.js +++ b/packages/sasl2/index.js @@ -106,7 +106,12 @@ export default function sasl2({ streamFeatures, saslFactory }, onAuthenticate) { throw new SASLError("SASL: No compatible mechanism available."); } - await onAuthenticate(done, mechanisms, fast_available && fast); + await onAuthenticate( + done, + mechanisms, + fast_available ? fast : null, + entity, + ); async function done(credentials, mechanism, userAgent) { if (fast_available) { diff --git a/packages/test/mockSocket.js b/packages/test/mockSocket.js index 09f25c72..5bb0c455 100644 --- a/packages/test/mockSocket.js +++ b/packages/test/mockSocket.js @@ -6,6 +6,9 @@ class MockSocket extends net.Socket { cb?.(); }); } + isSecure() { + return true; + } } export default function mockSocket() { diff --git a/rollup.config.js b/rollup.config.js index fd709d68..8e905d2c 100644 --- a/rollup.config.js +++ b/rollup.config.js @@ -14,6 +14,7 @@ export default [ name: "XMPP", }, plugins: [ + terser(), babel({ babelHelpers: "runtime" }), nodePolyfills(), nodeResolve({ preferBuiltins: false, browser: true }),