Skip to content

Commit

Permalink
client: Do not allow PLAIN on insecure connection
Browse files Browse the repository at this point in the history
Also add connection.isSecure() method

Fixes #1040
  • Loading branch information
sonnyp committed Jan 10, 2025
1 parent c5b54c8 commit f9eba3d
Show file tree
Hide file tree
Showing 11 changed files with 107 additions and 20 deletions.
2 changes: 1 addition & 1 deletion packages/client-core/lib/Client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
9 changes: 4 additions & 5 deletions packages/client-core/test/Client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(<foo />)).toEqual(<foo />);

entity.jid = null;
entity.socket.isSecure = () => true;
entity.isSecure = () => true;
expect(entity.header(<foo />)).toEqual(<foo />);

entity.jid = new JID("foo@bar/example");
entity.socket.isSecure = () => false;
entity.isSecure = () => false;
expect(entity.header(<foo />)).toEqual(<foo />);

entity.jid = new JID("foo@bar/example");
entity.socket.isSecure = () => true;
entity.isSecure = () => true;
expect(entity.header(<foo />)).toEqual(<foo from="foo@bar" />);
});
15 changes: 15 additions & 0 deletions packages/client/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
32 changes: 20 additions & 12 deletions packages/client/lib/createOnAuthenticate.js
Original file line number Diff line number Diff line change
@@ -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);
}
38 changes: 38 additions & 0 deletions packages/client/test/getMechanism.js
Original file line number Diff line number Diff line change
@@ -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);
});
4 changes: 4 additions & 0 deletions packages/connection/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ class Connection extends EventEmitter {
this.root = null;
}

isSecure() {
return !!this.socket?.isSecure();
}

async _streamError(condition, children) {
try {
await this.send(
Expand Down
14 changes: 14 additions & 0 deletions packages/connection/test/isSecure.js
Original file line number Diff line number Diff line change
@@ -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);
});
2 changes: 1 addition & 1 deletion packages/sasl/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export default function sasl({ streamFeatures, saslFactory }, onAuthenticate) {
});
}

await onAuthenticate(done, mechanisms);
await onAuthenticate(done, mechanisms, null, entity);

await entity.restart();
});
Expand Down
7 changes: 6 additions & 1 deletion packages/sasl2/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions packages/test/mockSocket.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ class MockSocket extends net.Socket {
cb?.();
});
}
isSecure() {
return true;
}
}

export default function mockSocket() {
Expand Down
1 change: 1 addition & 0 deletions rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export default [
name: "XMPP",
},
plugins: [
terser(),
babel({ babelHelpers: "runtime" }),
nodePolyfills(),
nodeResolve({ preferBuiltins: false, browser: true }),
Expand Down

0 comments on commit f9eba3d

Please sign in to comment.