diff --git a/apps/integration-tests/src/end-to-end.integration.test.ts b/apps/integration-tests/src/end-to-end.integration.test.ts index b7fd767..33dd482 100644 --- a/apps/integration-tests/src/end-to-end.integration.test.ts +++ b/apps/integration-tests/src/end-to-end.integration.test.ts @@ -3,7 +3,7 @@ import { type ConnectionMode, type IKeyManager, type IKVStore, type KeyPair, type SessionRequest, SessionStore, WebSocketTransport } from "@metamask/mobile-wallet-protocol-core"; import { DappClient, type OtpRequiredPayload } from "@metamask/mobile-wallet-protocol-dapp-client"; import { WalletClient } from "@metamask/mobile-wallet-protocol-wallet-client"; -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; import { type Proxy, Toxiproxy } from "toxiproxy-node-client"; import * as t from "vitest"; import WebSocket from "ws"; @@ -32,6 +32,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); diff --git a/apps/load-tests/src/client/key-manager.ts b/apps/load-tests/src/client/key-manager.ts index a0d183d..932e6c1 100644 --- a/apps/load-tests/src/client/key-manager.ts +++ b/apps/load-tests/src/client/key-manager.ts @@ -21,6 +21,10 @@ export class MockKeyManager implements IKeyManager { }; } + validatePeerKey(_key: Uint8Array): void { + // No-op: load tests don't use real crypto + } + async encrypt(plaintext: string, _theirPublicKey: Uint8Array): Promise { return Buffer.from(plaintext, "utf8").toString("base64"); } diff --git a/apps/rn-demo/lib/KeyManager.ts b/apps/rn-demo/lib/KeyManager.ts index 3c67649..32b5bad 100644 --- a/apps/rn-demo/lib/KeyManager.ts +++ b/apps/rn-demo/lib/KeyManager.ts @@ -1,5 +1,5 @@ import type { IKeyManager, KeyPair } from "@metamask/mobile-wallet-protocol-core"; -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; export class KeyManager implements IKeyManager { generateKeyPair(): KeyPair { @@ -7,6 +7,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); diff --git a/apps/web-demo/src/lib/KeyManager.ts b/apps/web-demo/src/lib/KeyManager.ts index 3c67649..32b5bad 100644 --- a/apps/web-demo/src/lib/KeyManager.ts +++ b/apps/web-demo/src/lib/KeyManager.ts @@ -1,5 +1,5 @@ import type { IKeyManager, KeyPair } from "@metamask/mobile-wallet-protocol-core"; -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; export class KeyManager implements IKeyManager { generateKeyPair(): KeyPair { @@ -7,6 +7,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); diff --git a/packages/core/CHANGELOG.md b/packages/core/CHANGELOG.md index ae1a5c6..516e9d7 100644 --- a/packages/core/CHANGELOG.md +++ b/packages/core/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Add `validatePeerKey` method to `IKeyManager` interface for peer public key validation at handshake and resume time ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) + +### Fixed + +- Guard against `NaN` in session expiry timestamps ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) + ## [0.3.1] ### Fixed diff --git a/packages/core/src/base-client.integration.test.ts b/packages/core/src/base-client.integration.test.ts index e536cee..ef0273a 100644 --- a/packages/core/src/base-client.integration.test.ts +++ b/packages/core/src/base-client.integration.test.ts @@ -1,7 +1,7 @@ /** biome-ignore-all lint/suspicious/noExplicitAny: test code */ /** biome-ignore-all lint/complexity/useLiteralKeys: test code */ -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; import { v4 as uuid } from "uuid"; import * as t from "vitest"; import WebSocket from "ws"; @@ -43,6 +43,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); @@ -170,7 +174,11 @@ t.describe("BaseClient", () => { id: "session-to-disconnect", channel, keyPair: new KeyManager().generateKeyPair(), - theirPublicKey: new Uint8Array(33), + theirPublicKey: (() => { + const k = new Uint8Array(33); + k[0] = 0x02; + return k; + })(), expiresAt: Date.now() + 60000, }; @@ -198,7 +206,11 @@ t.describe("BaseClient", () => { id: "expired-session", channel, keyPair: new KeyManager().generateKeyPair(), - theirPublicKey: new Uint8Array(33), + theirPublicKey: (() => { + const k = new Uint8Array(33); + k[0] = 0x02; + return k; + })(), expiresAt: Date.now() - 1000, // Expired 1 second ago }; @@ -220,7 +232,11 @@ t.describe("BaseClient", () => { id: "expired-resume-session", channel, keyPair: new KeyManager().generateKeyPair(), - theirPublicKey: new Uint8Array(33), + theirPublicKey: (() => { + const k = new Uint8Array(33); + k[0] = 0x02; + return k; + })(), expiresAt: Date.now() + 60000, // Valid session }; diff --git a/packages/core/src/base-client.ts b/packages/core/src/base-client.ts index 7173fcf..ede243f 100644 --- a/packages/core/src/base-client.ts +++ b/packages/core/src/base-client.ts @@ -92,6 +92,7 @@ export abstract class BaseClient extends EventEmitter { try { const session = await this.sessionstore.get(sessionId); if (!session) throw new SessionError(ErrorCode.SESSION_NOT_FOUND, "Session not found or expired"); + this.keymanager.validatePeerKey(session.theirPublicKey); this.session = session; await this.transport.connect(); diff --git a/packages/core/src/domain/errors.ts b/packages/core/src/domain/errors.ts index d3d2515..a308fbe 100644 --- a/packages/core/src/domain/errors.ts +++ b/packages/core/src/domain/errors.ts @@ -15,6 +15,7 @@ export enum ErrorCode { // Crypto errors DECRYPTION_FAILED = "DECRYPTION_FAILED", + INVALID_KEY = "INVALID_KEY", // Handshake errors REQUEST_EXPIRED = "REQUEST_EXPIRED", diff --git a/packages/core/src/domain/key-manager.ts b/packages/core/src/domain/key-manager.ts index fa0fd90..cfebc7e 100644 --- a/packages/core/src/domain/key-manager.ts +++ b/packages/core/src/domain/key-manager.ts @@ -7,4 +7,5 @@ export interface IKeyManager { generateKeyPair(): KeyPair; encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise; decrypt(encryptedB64: string, myPrivateKey: Uint8Array): Promise; + validatePeerKey(key: Uint8Array): void; } diff --git a/packages/core/src/session-store/index.test.ts b/packages/core/src/session-store/index.test.ts index 91df175..094516a 100644 --- a/packages/core/src/session-store/index.test.ts +++ b/packages/core/src/session-store/index.test.ts @@ -114,6 +114,27 @@ t.describe("SessionStore", () => { t.expect(raw).toBeNull(); }); + t.test("should reject a session with NaN expiresAt on set", async () => { + const session = createSession("nan-set", Number.NaN); + await t.expect(sessionstore.set(session)).rejects.toThrow("Cannot save expired session"); + }); + + t.test("should treat a session with NaN expiresAt as expired on get", async () => { + const key = "session:nan-get"; + const data = { + id: "nan-get", + channel: "test-channel", + keyPair: { publicKeyB64: Buffer.from(new Uint8Array(33).fill(1)).toString("base64"), privateKeyB64: Buffer.from(new Uint8Array(32).fill(1)).toString("base64") }, + theirPublicKeyB64: Buffer.from(new Uint8Array(33).fill(2)).toString("base64"), + expiresAt: "not-a-number", + }; + await kvstore.set(key, JSON.stringify(data)); + await (kvstore as MockKVStore)["store"].set("sessions:master-list", JSON.stringify(["nan-get"])); + + const retrieved = await sessionstore.get("nan-get"); + t.expect(retrieved).toBeNull(); + }); + t.test("should garbage collect expired sessions", async () => { const valid = createSession("valid", Date.now() + 10000); const expired = createSession("expired", Date.now() - 10000); diff --git a/packages/core/src/session-store/index.ts b/packages/core/src/session-store/index.ts index 67497a6..a50bda6 100644 --- a/packages/core/src/session-store/index.ts +++ b/packages/core/src/session-store/index.ts @@ -42,8 +42,8 @@ export class SessionStore implements ISessionStore { * @param session - The session to set. */ async set(session: Session): Promise { - // Check if session is expired - if (session.expiresAt < Date.now()) { + // Check if session is expired (also rejects NaN) + if (Number.isNaN(session.expiresAt) || session.expiresAt < Date.now()) { throw new SessionError(ErrorCode.SESSION_SAVE_FAILED, "Cannot save expired session"); } @@ -80,14 +80,12 @@ export class SessionStore implements ISessionStore { try { const data: SerializableSession = JSON.parse(raw); - // Check if session is expired - if (data.expiresAt < Date.now()) { - // Session expired, clean it up + // Check if session is expired (handles NaN, non-number, and past timestamps) + if (typeof data.expiresAt !== "number" || !(data.expiresAt >= Date.now())) { await this.delete(id); return null; } - // Deserialize back to Session const session: Session = { id: data.id, channel: data.channel, diff --git a/packages/core/src/transport/websocket/shared-centrifuge.test.ts b/packages/core/src/transport/websocket/shared-centrifuge.test.ts index c1c6aa2..291c665 100644 --- a/packages/core/src/transport/websocket/shared-centrifuge.test.ts +++ b/packages/core/src/transport/websocket/shared-centrifuge.test.ts @@ -7,7 +7,7 @@ t.describe("SharedCentrifuge Unit Tests", () => { // Clean up any shared contexts after each test // @ts-expect-error - accessing private property for cleanup const contexts = SharedCentrifuge.contexts; - for (const [url, context] of contexts.entries()) { + for (const [_url, context] of contexts.entries()) { // Clear any pending reconnect promises context.reconnectPromise = null; // Remove all event listeners to prevent unhandled errors diff --git a/packages/dapp-client/CHANGELOG.md b/packages/dapp-client/CHANGELOG.md index 38e10d5..06b7386 100644 --- a/packages/dapp-client/CHANGELOG.md +++ b/packages/dapp-client/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Validate peer public keys during handshake in both trusted and untrusted connection flows ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) + ## [0.2.2] ### Added diff --git a/packages/dapp-client/src/client.integration.test.ts b/packages/dapp-client/src/client.integration.test.ts index 1277d00..3d064ff 100644 --- a/packages/dapp-client/src/client.integration.test.ts +++ b/packages/dapp-client/src/client.integration.test.ts @@ -1,6 +1,6 @@ /** biome-ignore-all lint/suspicious/noExplicitAny: test code */ import { type IKeyManager, type IKVStore, type KeyPair, type SessionRequest, SessionStore, WebSocketTransport } from "@metamask/mobile-wallet-protocol-core"; -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; import * as t from "vitest"; import WebSocket from "ws"; import { DappClient } from "./client"; @@ -26,6 +26,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); diff --git a/packages/dapp-client/src/client.ts b/packages/dapp-client/src/client.ts index b68c21d..d76c084 100644 --- a/packages/dapp-client/src/client.ts +++ b/packages/dapp-client/src/client.ts @@ -118,6 +118,7 @@ export class DappClient extends BaseClient { const context: IConnectionHandlerContext = { transport: this.transport, sessionstore: this.sessionstore, + keymanager: this.keymanager, get session() { return self.session; }, diff --git a/packages/dapp-client/src/domain/connection-handler-context.ts b/packages/dapp-client/src/domain/connection-handler-context.ts index d6545f2..ab83202 100644 --- a/packages/dapp-client/src/domain/connection-handler-context.ts +++ b/packages/dapp-client/src/domain/connection-handler-context.ts @@ -1,4 +1,4 @@ -import type { ClientState, HandshakeOfferPayload, ISessionStore, ITransport, ProtocolMessage, Session } from "@metamask/mobile-wallet-protocol-core"; +import type { ClientState, HandshakeOfferPayload, IKeyManager, ISessionStore, ITransport, ProtocolMessage, Session } from "@metamask/mobile-wallet-protocol-core"; import type { OtpRequiredPayload } from "../client"; /** @@ -14,6 +14,7 @@ export interface IConnectionHandlerContext { // Core Dependencies readonly transport: ITransport; readonly sessionstore: ISessionStore; + readonly keymanager: IKeyManager; // Events emit(event: "otp_required", payload: OtpRequiredPayload): void; diff --git a/packages/dapp-client/src/handlers/trusted-connection-handler.test.ts b/packages/dapp-client/src/handlers/trusted-connection-handler.test.ts index ce04d49..eaad7fe 100644 --- a/packages/dapp-client/src/handlers/trusted-connection-handler.test.ts +++ b/packages/dapp-client/src/handlers/trusted-connection-handler.test.ts @@ -22,6 +22,12 @@ function createMockDappHandlerContext(): IConnectionHandlerContext { list: vi.fn(), delete: vi.fn(), }, + keymanager: { + generateKeyPair: vi.fn(), + encrypt: vi.fn(), + decrypt: vi.fn(), + validatePeerKey: vi.fn(), + }, emit: vi.fn(), once: vi.fn(), off: vi.fn(), @@ -56,7 +62,7 @@ t.describe("TrustedConnectionHandler", () => { }; mockOffer = { channelId: "secure-channel", - publicKeyB64: "cHVia2V5", + publicKeyB64: "Aqurq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur", }; // No OTP for trusted flow }); diff --git a/packages/dapp-client/src/handlers/trusted-connection-handler.ts b/packages/dapp-client/src/handlers/trusted-connection-handler.ts index 1de6392..e1a9ac4 100644 --- a/packages/dapp-client/src/handlers/trusted-connection-handler.ts +++ b/packages/dapp-client/src/handlers/trusted-connection-handler.ts @@ -82,10 +82,12 @@ export class TrustedConnectionHandler implements IConnectionHandler { * @returns The complete session object ready for use */ private _createFinalSession(session: Session, offer: HandshakeOfferPayload): Session { + const theirPublicKey = base64ToBytes(offer.publicKeyB64); + this.context.keymanager.validatePeerKey(theirPublicKey); return { ...session, channel: `session:${offer.channelId}`, - theirPublicKey: base64ToBytes(offer.publicKeyB64), + theirPublicKey, }; } diff --git a/packages/dapp-client/src/handlers/untrusted-connection-handler.test.ts b/packages/dapp-client/src/handlers/untrusted-connection-handler.test.ts index cf26e60..0126a9b 100644 --- a/packages/dapp-client/src/handlers/untrusted-connection-handler.test.ts +++ b/packages/dapp-client/src/handlers/untrusted-connection-handler.test.ts @@ -23,6 +23,12 @@ function createMockDappHandlerContext(): IConnectionHandlerContext { list: vi.fn(), delete: vi.fn(), }, + keymanager: { + generateKeyPair: vi.fn(), + encrypt: vi.fn(), + decrypt: vi.fn(), + validatePeerKey: vi.fn(), + }, emit: vi.fn(), once: vi.fn(), off: vi.fn(), @@ -57,7 +63,7 @@ t.describe("UntrustedConnectionHandler", () => { }; mockOffer = { channelId: "secure-channel", - publicKeyB64: "cHVia2V5", + publicKeyB64: "Aqurq6urq6urq6urq6urq6urq6urq6urq6urq6urq6ur", otp: "123456", deadline: Date.now() + 1000, }; diff --git a/packages/dapp-client/src/handlers/untrusted-connection-handler.ts b/packages/dapp-client/src/handlers/untrusted-connection-handler.ts index 376fdcf..ea9564d 100644 --- a/packages/dapp-client/src/handlers/untrusted-connection-handler.ts +++ b/packages/dapp-client/src/handlers/untrusted-connection-handler.ts @@ -115,10 +115,12 @@ export class UntrustedConnectionHandler implements IConnectionHandler { * @returns The complete session object ready for use */ private _createFinalSession(session: Session, offer: HandshakeOfferPayload): Session { + const theirPublicKey = base64ToBytes(offer.publicKeyB64); + this.context.keymanager.validatePeerKey(theirPublicKey); return { ...session, channel: `session:${offer.channelId}`, - theirPublicKey: base64ToBytes(offer.publicKeyB64), + theirPublicKey, }; } diff --git a/packages/wallet-client/CHANGELOG.md b/packages/wallet-client/CHANGELOG.md index 9b841bb..4d8f5ef 100644 --- a/packages/wallet-client/CHANGELOG.md +++ b/packages/wallet-client/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Validate peer public keys during session creation ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) +- Fix client stuck in CONNECTING state when session creation fails ([#70](https://github.com/MetaMask/mobile-wallet-protocol/pull/70)) + ## [0.2.2] ### Added diff --git a/packages/wallet-client/src/client.integration.test.ts b/packages/wallet-client/src/client.integration.test.ts index e2b7717..08d63b9 100644 --- a/packages/wallet-client/src/client.integration.test.ts +++ b/packages/wallet-client/src/client.integration.test.ts @@ -1,7 +1,7 @@ /** biome-ignore-all lint/suspicious/noExplicitAny: test code */ import { type IKeyManager, type IKVStore, type KeyPair, type SessionRequest, SessionStore, WebSocketTransport } from "@metamask/mobile-wallet-protocol-core"; import { bytesToBase64 } from "@metamask/utils"; -import { decrypt, encrypt, PrivateKey } from "eciesjs"; +import { decrypt, encrypt, PrivateKey, PublicKey } from "eciesjs"; import * as t from "vitest"; import WebSocket from "ws"; import { WalletClient } from "./client"; @@ -27,6 +27,10 @@ export class KeyManager implements IKeyManager { return { privateKey: new Uint8Array(privateKey.secret), publicKey: privateKey.publicKey.toBytes(true) }; } + validatePeerKey(key: Uint8Array): void { + PublicKey.fromHex(Buffer.from(key).toString("hex")); + } + async encrypt(plaintext: string, theirPublicKey: Uint8Array): Promise { const plaintextBuffer = Buffer.from(plaintext, "utf8"); const encryptedBuffer = encrypt(theirPublicKey, plaintextBuffer); diff --git a/packages/wallet-client/src/client.ts b/packages/wallet-client/src/client.ts index 575041c..1187a26 100644 --- a/packages/wallet-client/src/client.ts +++ b/packages/wallet-client/src/client.ts @@ -82,8 +82,6 @@ export class WalletClient extends BaseClient { const request = options.sessionRequest; if (Date.now() > request.expiresAt) throw new SessionError(ErrorCode.REQUEST_EXPIRED, "Session request expired"); - const session = this._createSession(request); - const self = this; const context: IConnectionHandlerContext = { transport: this.transport, @@ -110,10 +108,15 @@ export class WalletClient extends BaseClient { const handler: IConnectionHandler = request.mode === "trusted" ? new TrustedConnectionHandler(context) : new UntrustedConnectionHandler(context); try { + const session = this._createSession(request); await handler.execute(session, request); } catch (error) { this.emit("error", error); - await this.disconnect(); + if (this.session) { + await this.disconnect(); + } else { + this.state = ClientState.DISCONNECTED; + } throw error; } } @@ -154,11 +157,13 @@ export class WalletClient extends BaseClient { * @returns A new `Session` object */ private _createSession(request: SessionRequest): Session { + const theirPublicKey = base64ToBytes(request.publicKeyB64); + this.keymanager.validatePeerKey(theirPublicKey); return { id: request.id, channel: `session:${uuid()}`, // Create a new, unique channel for secure communication keyPair: this.keymanager.generateKeyPair(), - theirPublicKey: base64ToBytes(request.publicKeyB64), + theirPublicKey, expiresAt: Date.now() + DEFAULT_SESSION_TTL, }; }