security: fix SAS verification bypass and unauthenticated frame injection; release v5.5.2
A security review of the transport and verification layers. Every item is a fix
to how untrusted peer input is handled; no features changed.
- SAS verification could be bypassed. `verification_both_confirmed` is an
unauthenticated frame on a channel that is not yet trusted, but it was taken as
proof that both sides had compared their codes — so a peer who completed the
signalling exchange could send it right after the data channel opened and drive
the other side to a "verified" session while the user never looked at the code.
It is now only an acknowledgement: refused unless this side already confirmed
locally, and _setVerifiedStatus() independently rejects any SAS-based
transition without a local confirmation. Holding ECDH-derived keys was never
proof of identity — a MITM has those too.
- Unauthenticated frames could be injected into the chat. A bare
{type:"message"} frame, a raw non-JSON frame and a binary frame were each
decoded and rendered, bypassing decryption, the HMAC check and the verification
gate; the injected text was indistinguishable from a genuine message. Chat
content now reaches the UI only through the authenticated enhanced_message
path.
- A peer could supply the verification code. `sas_code` announcements were
adopted verbatim when no local SAS had been derived yet. They may now only
corroborate the locally derived code.
- Anti-replay never ran. The sequence-number and AAD validators were defined on
SecureKeyStorage instead of the connection manager, so every call site failed
with a TypeError and the sliding replay window was dead code. Moved onto the
manager, wired into the live chat path, and a missing or non-numeric sequence
number now fails closed instead of sailing through the range checks.
- File transfers are gated on verification in both directions. Control frames are
written straight to the data channel by the transfer system; sending was
already gated, receiving now is too.
- Tighter CSP: connect-src and img-src no longer allow arbitrary https: hosts
(nothing in the app talks to a third party), plus base-uri 'none'.
- The SAS is no longer written to logs, and is compared in constant time on every
path. Fixed SecureMasterKeyManager.isUnlocked() testing a field renamed long
ago, so it never actually gated anything.
- Fixed the header showing "Secure undefined%": getRealSecurityLevel() became
reachable for the first time by the move above and returned only per-feature
booleans, while the header renders `level` and `score` directly. It now runs
the same verified scoring as every other consumer.
Adds regression tests for the verification gate, inbound frame authentication and
the security-level shape.
This commit is contained in:
@@ -0,0 +1,193 @@
|
||||
// Regression tests for inbound frame authentication.
|
||||
//
|
||||
// Chat content must reach the UI through exactly one door: an `enhanced_message`
|
||||
// that has been AES-GCM decrypted, HMAC-verified and replay-checked. Anything
|
||||
// else arriving on the data channel is unauthenticated peer input and must be
|
||||
// dropped rather than rendered as if it were a real message.
|
||||
|
||||
import assert from 'node:assert/strict';
|
||||
|
||||
globalThis.window = globalThis.window || {};
|
||||
|
||||
const { EnhancedSecureWebRTCManager } = await import('../src/network/EnhancedSecureWebRTCManager.js');
|
||||
const P = EnhancedSecureWebRTCManager.prototype;
|
||||
|
||||
function createReceiver({ isVerified = true } = {}) {
|
||||
const delivered = [];
|
||||
const receiver = {
|
||||
delivered,
|
||||
isVerified,
|
||||
encryptionKey: null,
|
||||
macKey: null,
|
||||
metadataKey: null,
|
||||
securityFeatures: {},
|
||||
onMessage: (m, t) => delivered.push({ message: m, type: t }),
|
||||
deliverMessageToUI(msg, type) { this.onMessage(msg, type); },
|
||||
_secureLog() {},
|
||||
_checkInboundRateLimit: () => true,
|
||||
establishConnection: async () => {},
|
||||
initializeFileTransfer() {},
|
||||
startHeartbeat() {},
|
||||
_notifyVerificationReadyIfPossible() {},
|
||||
initiateVerification() {},
|
||||
setupDataChannel: P.setupDataChannel,
|
||||
_processBinaryDataWithoutMutex: P._processBinaryDataWithoutMutex,
|
||||
_processEnhancedMessageWithoutMutex: P._processEnhancedMessageWithoutMutex
|
||||
};
|
||||
const channel = { readyState: 'open', send() {} };
|
||||
receiver.setupDataChannel(channel);
|
||||
return { receiver, channel };
|
||||
}
|
||||
|
||||
// ── unencrypted {type:'message'} frames are rejected ─────────────────────────
|
||||
{
|
||||
const { receiver, channel } = createReceiver();
|
||||
await channel.onmessage({
|
||||
data: JSON.stringify({ type: 'message', data: 'injected without any crypto' })
|
||||
});
|
||||
assert.deepEqual(receiver.delivered, [], 'plaintext message frames must not reach the UI');
|
||||
}
|
||||
|
||||
// ── raw non-JSON text is rejected ────────────────────────────────────────────
|
||||
{
|
||||
const { receiver, channel } = createReceiver();
|
||||
await channel.onmessage({ data: 'not even json' });
|
||||
assert.deepEqual(receiver.delivered, [], 'raw text frames must not reach the UI');
|
||||
}
|
||||
|
||||
// ── binary frames never reach the UI ─────────────────────────────────────────
|
||||
{
|
||||
const { receiver, channel } = createReceiver();
|
||||
const buf = new TextEncoder().encode('binary injection').buffer;
|
||||
await channel.onmessage({ data: buf });
|
||||
assert.deepEqual(receiver.delivered, [], 'binary frames must not reach the UI');
|
||||
}
|
||||
|
||||
// ── cover traffic is still discarded silently ────────────────────────────────
|
||||
{
|
||||
const { receiver, channel } = createReceiver();
|
||||
const fake = new TextEncoder().encode(JSON.stringify({ type: 'fake', isFakeTraffic: true })).buffer;
|
||||
await channel.onmessage({ data: fake });
|
||||
assert.deepEqual(receiver.delivered, []);
|
||||
}
|
||||
|
||||
// ── the authenticated path still delivers, and enforces anti-replay ──────────
|
||||
{
|
||||
let seq = 0;
|
||||
globalThis.window.EnhancedSecureCryptoUtils = {
|
||||
decryptMessage: async () => ({
|
||||
message: JSON.stringify({ type: 'message', data: `msg-${seq}` }),
|
||||
messageId: `id-${seq}`,
|
||||
sequenceNumber: seq
|
||||
})
|
||||
};
|
||||
|
||||
const delivered = [];
|
||||
const receiver = {
|
||||
encryptionKey: {}, macKey: {}, metadataKey: {},
|
||||
_secureLog() {},
|
||||
_checkInboundRateLimit: () => true,
|
||||
_sanitizeIncomingChatMessage: (m) => m,
|
||||
_sanitizeMessageMeta: P._sanitizeMessageMeta,
|
||||
_validateIncomingSequenceNumber: P._validateIncomingSequenceNumber,
|
||||
replayProtectionEnabled: true,
|
||||
expectedSequenceNumber: 0,
|
||||
replayWindow: new Set(),
|
||||
replayWindowSize: 64,
|
||||
maxSequenceGap: 100,
|
||||
onMessage: (m) => delivered.push(m),
|
||||
deliverMessageToUI: P.deliverMessageToUI,
|
||||
_processEnhancedMessageWithoutMutex: P._processEnhancedMessageWithoutMutex
|
||||
};
|
||||
|
||||
seq = 0;
|
||||
await receiver._processEnhancedMessageWithoutMutex({ type: 'enhanced_message', data: 'enc' });
|
||||
seq = 1;
|
||||
await receiver._processEnhancedMessageWithoutMutex({ type: 'enhanced_message', data: 'enc' });
|
||||
assert.deepEqual(delivered, ['msg-0', 'msg-1'], 'fresh messages must be delivered');
|
||||
|
||||
// Replaying sequence number 0 must be refused.
|
||||
seq = 0;
|
||||
await receiver._processEnhancedMessageWithoutMutex({ type: 'enhanced_message', data: 'enc' });
|
||||
assert.deepEqual(delivered, ['msg-0', 'msg-1'], 'replayed message must be dropped');
|
||||
}
|
||||
|
||||
// ── a missing sequence number fails closed ───────────────────────────────────
|
||||
{
|
||||
const receiver = {
|
||||
replayProtectionEnabled: true,
|
||||
expectedSequenceNumber: 0,
|
||||
replayWindow: new Set(),
|
||||
replayWindowSize: 64,
|
||||
maxSequenceGap: 100,
|
||||
_secureLog() {},
|
||||
_validateIncomingSequenceNumber: P._validateIncomingSequenceNumber
|
||||
};
|
||||
|
||||
assert.equal(receiver._validateIncomingSequenceNumber(undefined, 't'), false);
|
||||
assert.equal(receiver._validateIncomingSequenceNumber(null, 't'), false);
|
||||
assert.equal(receiver._validateIncomingSequenceNumber('3', 't'), false);
|
||||
assert.equal(receiver._validateIncomingSequenceNumber(NaN, 't'), false);
|
||||
assert.equal(receiver.replayWindow.size, 0, 'rejected values must not pollute the window');
|
||||
assert.equal(receiver._validateIncomingSequenceNumber(0, 't'), true);
|
||||
}
|
||||
|
||||
// ── file control frames are gated on verification ────────────────────────────
|
||||
{
|
||||
const handled = [];
|
||||
const makeReceiver = (isVerified) => {
|
||||
const receiver = {
|
||||
isVerified,
|
||||
securityFeatures: {},
|
||||
fileTransferSystem: { handleFileMessage: async (m) => handled.push(m.type) },
|
||||
_secureLog() {},
|
||||
_checkInboundRateLimit: () => true,
|
||||
_enforceVerificationGate: P._enforceVerificationGate,
|
||||
onMessage() {},
|
||||
deliverMessageToUI() {},
|
||||
establishConnection: async () => {},
|
||||
initializeFileTransfer() {},
|
||||
startHeartbeat() {},
|
||||
_notifyVerificationReadyIfPossible() {},
|
||||
initiateVerification() {},
|
||||
setupDataChannel: P.setupDataChannel
|
||||
};
|
||||
const channel = { readyState: 'open', send() {} };
|
||||
receiver.setupDataChannel(channel);
|
||||
return channel;
|
||||
};
|
||||
|
||||
// Unverified peer: file frames must be dropped, not acted on.
|
||||
const unverified = makeReceiver(false);
|
||||
await unverified.onmessage({
|
||||
data: JSON.stringify({ type: 'file_transfer_start', fileId: 'f1', fileName: 'x.png' })
|
||||
});
|
||||
await unverified.onmessage({ data: JSON.stringify({ type: 'file_chunk', fileId: 'f1' }) });
|
||||
assert.deepEqual(handled, [], 'file frames must not be processed before verification');
|
||||
|
||||
// Verified peer: transfers keep working exactly as before.
|
||||
const verified = makeReceiver(true);
|
||||
await verified.onmessage({
|
||||
data: JSON.stringify({ type: 'file_transfer_start', fileId: 'f1', fileName: 'x.png' })
|
||||
});
|
||||
await verified.onmessage({ data: JSON.stringify({ type: 'file_chunk', fileId: 'f1' }) });
|
||||
assert.deepEqual(handled, ['file_transfer_start', 'file_chunk'], 'verified transfers must still work');
|
||||
}
|
||||
|
||||
// ── anti-replay helpers live on the connection, not on key storage ───────────
|
||||
{
|
||||
for (const name of [
|
||||
'_validateIncomingSequenceNumber',
|
||||
'_validateMessageAAD',
|
||||
'getAntiReplayStatus',
|
||||
'configureAntiReplayProtection'
|
||||
]) {
|
||||
assert.equal(
|
||||
typeof P[name],
|
||||
'function',
|
||||
`${name} must be reachable on the WebRTC manager (it reads this.replayWindow)`
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
console.log('Inbound frame authentication tests passed');
|
||||
@@ -66,8 +66,10 @@ const T = EnhancedSecureWebRTCManager.MESSAGE_TYPES;
|
||||
// This is the path real chat uses (dataChannel.onmessage -> _processEnhancedMessageWithoutMutex).
|
||||
{
|
||||
const envelope = JSON.stringify({ type: 'message', data: 'hi there', meta: { mid: 'm7', once: true, ttl: 30 } });
|
||||
// decryptMessage always returns the authenticated sequence number alongside
|
||||
// the plaintext; the receive path uses it for anti-replay validation.
|
||||
globalThis.window.EnhancedSecureCryptoUtils = {
|
||||
decryptMessage: async () => ({ message: envelope })
|
||||
decryptMessage: async () => ({ message: envelope, messageId: 'msg_7', sequenceNumber: 0 })
|
||||
};
|
||||
const calls = [];
|
||||
const manager = {
|
||||
@@ -76,6 +78,12 @@ const T = EnhancedSecureWebRTCManager.MESSAGE_TYPES;
|
||||
_checkInboundRateLimit: () => true,
|
||||
_sanitizeIncomingChatMessage: (m) => m,
|
||||
_sanitizeMessageMeta: P._sanitizeMessageMeta,
|
||||
_validateIncomingSequenceNumber: P._validateIncomingSequenceNumber,
|
||||
replayProtectionEnabled: true,
|
||||
expectedSequenceNumber: 0,
|
||||
replayWindow: new Set(),
|
||||
replayWindowSize: 64,
|
||||
maxSequenceGap: 100,
|
||||
onMessage: (message, type, meta) => calls.push({ message, type, meta }),
|
||||
deliverMessageToUI: P.deliverMessageToUI
|
||||
};
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
// The header renders `level` and `score` straight from whatever the security
|
||||
// getter returns:
|
||||
//
|
||||
// sec.level || 'Secure' -> label
|
||||
// sec.score + '%' -> score badge
|
||||
//
|
||||
// so any getter the header may call must carry both fields. A getter that
|
||||
// returned only per-feature booleans rendered as "Secure undefined%".
|
||||
// These tests pin the shape for every branch of the header's fallback chain.
|
||||
|
||||
import assert from 'node:assert/strict';
|
||||
|
||||
globalThis.window = globalThis.window || {};
|
||||
|
||||
const { EnhancedSecureWebRTCManager } = await import('../src/network/EnhancedSecureWebRTCManager.js');
|
||||
const P = EnhancedSecureWebRTCManager.prototype;
|
||||
|
||||
const SCORED = {
|
||||
level: 'HIGH',
|
||||
score: 90,
|
||||
color: 'green',
|
||||
isRealData: true,
|
||||
passedChecks: 9,
|
||||
totalChecks: 10
|
||||
};
|
||||
|
||||
function createManager(overrides = {}) {
|
||||
return Object.assign({
|
||||
ecdhKeyPair: {},
|
||||
ecdsaKeyPair: {},
|
||||
encryptionKey: {},
|
||||
hmacKey: {},
|
||||
replayProtectionEnabled: true,
|
||||
expectedDTLSFingerprint: 'aa:bb',
|
||||
verificationCode: '1234567',
|
||||
connectionId: 'conn-1',
|
||||
keyFingerprint: 'ff:ee',
|
||||
_secureLog() {},
|
||||
calculateAndReportSecurityLevel: async () => ({ ...SCORED }),
|
||||
getRealSecurityLevel: P.getRealSecurityLevel
|
||||
}, overrides);
|
||||
}
|
||||
|
||||
// ── the scored fields survive, alongside the feature flags ───────────────────
|
||||
{
|
||||
const data = await createManager().getRealSecurityLevel();
|
||||
|
||||
assert.equal(typeof data.score, 'number', 'score must be numeric (renders as `score + "%"`)');
|
||||
assert.equal(typeof data.level, 'string', 'level must be a string (renders as the label)');
|
||||
assert.equal(data.score, 90);
|
||||
assert.equal(data.level, 'HIGH');
|
||||
assert.equal(data.isRealData, true);
|
||||
assert.equal(data.passedChecks, 9);
|
||||
assert.equal(data.totalChecks, 10);
|
||||
|
||||
// Feature flags are still exposed for the detailed security panel.
|
||||
assert.equal(data.ecdhKeyExchange, true);
|
||||
assert.equal(data.sasCode, true);
|
||||
assert.equal(data.replayProtection, true);
|
||||
|
||||
// What the header would actually paint.
|
||||
assert.equal(String(data.level || 'Secure'), 'HIGH');
|
||||
assert.equal(data.score + '%', '90%');
|
||||
}
|
||||
|
||||
// ── not-ready path is flagged, not rendered as a real measurement ────────────
|
||||
{
|
||||
// calculateAndReportSecurityLevel returns null when the session is not yet
|
||||
// connected/verified or the keys are missing.
|
||||
const data = await createManager({
|
||||
calculateAndReportSecurityLevel: async () => null
|
||||
}).getRealSecurityLevel();
|
||||
|
||||
assert.equal(data.isRealData, false, 'UI keeps its previous value when isRealData is false');
|
||||
assert.equal(typeof data.score, 'number');
|
||||
assert.equal(typeof data.level, 'string');
|
||||
assert.notEqual(data.score + '%', 'undefined%');
|
||||
}
|
||||
|
||||
// ── the header's other fallback branch keeps the same contract ───────────────
|
||||
{
|
||||
const data = await createManager().calculateAndReportSecurityLevel();
|
||||
assert.equal(typeof data.score, 'number');
|
||||
assert.equal(typeof data.level, 'string');
|
||||
}
|
||||
|
||||
console.log('Security level shape tests passed');
|
||||
@@ -0,0 +1,142 @@
|
||||
// Regression tests for the MITM-protection gate.
|
||||
//
|
||||
// The whole security model rests on one step: the two users compare a Short
|
||||
// Authentication String out-of-band. Everything below asserts that this step
|
||||
// cannot be skipped, faked or supplied by the peer — an attacker who completes
|
||||
// the signalling exchange (and therefore holds valid ECDH-derived keys) must
|
||||
// still be unable to reach a verified session.
|
||||
|
||||
import assert from 'node:assert/strict';
|
||||
|
||||
globalThis.window = globalThis.window || {};
|
||||
globalThis.window.EnhancedSecureCryptoUtils = {
|
||||
constantTimeCompare: (a, b) => a === b
|
||||
};
|
||||
|
||||
const { EnhancedSecureWebRTCManager } = await import('../src/network/EnhancedSecureWebRTCManager.js');
|
||||
const P = EnhancedSecureWebRTCManager.prototype;
|
||||
|
||||
const settle = (ms = 2300) => new Promise((r) => setTimeout(r, ms));
|
||||
|
||||
function createPeerStub(overrides = {}) {
|
||||
const stub = {
|
||||
isVerified: false,
|
||||
localVerificationConfirmed: false,
|
||||
remoteVerificationConfirmed: false,
|
||||
bothVerificationsConfirmed: false,
|
||||
verificationCode: '1234567',
|
||||
// Present right after ECDH — a MITM has these too, which is exactly why
|
||||
// they must not be sufficient to become "verified".
|
||||
encryptionKey: {},
|
||||
macKey: {},
|
||||
keyFingerprint: 'aa:bb:cc',
|
||||
disconnected: false,
|
||||
statusChanges: [],
|
||||
uiMessages: [],
|
||||
|
||||
onStatusChange(s) { this.statusChanges.push(s); },
|
||||
onVerificationStateChange() {},
|
||||
_secureLog() {},
|
||||
deliverMessageToUI(msg) { this.uiMessages.push(String(msg)); },
|
||||
disconnect() { this.disconnected = true; },
|
||||
dataChannel: { readyState: 'open', send() {} },
|
||||
|
||||
handleSystemMessage: P.handleSystemMessage,
|
||||
handleVerificationBothConfirmed: P.handleVerificationBothConfirmed,
|
||||
handleVerificationConfirmed: P.handleVerificationConfirmed,
|
||||
handleSASCode: P.handleSASCode,
|
||||
_validateSASCode: P._validateSASCode,
|
||||
_checkBothVerificationsConfirmed: P._checkBothVerificationsConfirmed,
|
||||
_setVerifiedStatus: P._setVerifiedStatus,
|
||||
_enforceVerificationGate: P._enforceVerificationGate,
|
||||
_notifyVerificationReadyIfPossible() {},
|
||||
handleHeartbeat() {},
|
||||
handlePeerDisconnectNotification() {},
|
||||
handleVerificationRequest() {},
|
||||
handleVerificationResponse() {},
|
||||
processMessageQueue() {}
|
||||
};
|
||||
return Object.assign(stub, overrides);
|
||||
}
|
||||
|
||||
// ── a peer cannot manufacture mutual confirmation ────────────────────────────
|
||||
{
|
||||
const victim = createPeerStub();
|
||||
|
||||
victim.handleSystemMessage({
|
||||
type: 'verification_both_confirmed',
|
||||
data: { timestamp: Date.now(), verificationMethod: 'SAS' }
|
||||
});
|
||||
await settle();
|
||||
|
||||
assert.equal(victim.isVerified, false, 'peer must not be able to force verified state');
|
||||
assert.equal(victim.bothVerificationsConfirmed, false);
|
||||
assert.equal(victim.disconnected, true, 'protocol violation must tear down the session');
|
||||
assert.ok(
|
||||
!victim.statusChanges.includes('verified'),
|
||||
'no verified status may be emitted to the UI'
|
||||
);
|
||||
}
|
||||
|
||||
// ── the same frame IS honoured once the user has confirmed locally ───────────
|
||||
{
|
||||
const victim = createPeerStub({ localVerificationConfirmed: true });
|
||||
|
||||
victim.handleSystemMessage({
|
||||
type: 'verification_both_confirmed',
|
||||
data: { timestamp: Date.now(), verificationMethod: 'SAS' }
|
||||
});
|
||||
await settle();
|
||||
|
||||
assert.equal(victim.isVerified, true, 'legitimate mutual confirmation must still work');
|
||||
assert.equal(victim.remoteVerificationConfirmed, true);
|
||||
assert.equal(victim.disconnected, false);
|
||||
assert.ok(victim.statusChanges.includes('verified'));
|
||||
}
|
||||
|
||||
// ── _setVerifiedStatus refuses SAS transitions without local confirmation ────
|
||||
{
|
||||
const victim = createPeerStub();
|
||||
assert.throws(
|
||||
() => victim._setVerifiedStatus(true, 'MUTUAL_SAS_CONFIRMED', {}),
|
||||
/without local SAS confirmation/,
|
||||
'the low-level setter must fail closed too'
|
||||
);
|
||||
assert.equal(victim.isVerified, false);
|
||||
}
|
||||
|
||||
// ── holding keys alone is never enough ───────────────────────────────────────
|
||||
{
|
||||
const victim = createPeerStub({ encryptionKey: null, macKey: null });
|
||||
assert.throws(
|
||||
() => victim._setVerifiedStatus(true, 'MUTUAL_SAS_CONFIRMED', {}),
|
||||
/without encryption keys/
|
||||
);
|
||||
}
|
||||
|
||||
// ── a peer-announced SAS may corroborate, never supply ───────────────────────
|
||||
{
|
||||
// No locally derived code yet -> the announcement must be refused outright.
|
||||
const victim = createPeerStub({ verificationCode: null });
|
||||
victim.handleSystemMessage({ type: 'sas_code', data: { code: '7654321' } });
|
||||
|
||||
assert.equal(victim.verificationCode, null, 'peer-supplied SAS must not be adopted');
|
||||
assert.equal(victim.disconnected, true);
|
||||
}
|
||||
{
|
||||
// Locally derived code present and matching -> accepted, ours retained.
|
||||
const victim = createPeerStub({ verificationCode: '1234567' });
|
||||
victim.handleSystemMessage({ type: 'sas_code', data: { code: '1234567' } });
|
||||
|
||||
assert.equal(victim.verificationCode, '1234567');
|
||||
assert.equal(victim.disconnected, false);
|
||||
}
|
||||
{
|
||||
// Mismatch -> abort.
|
||||
const victim = createPeerStub({ verificationCode: '1234567' });
|
||||
victim.handleSystemMessage({ type: 'sas_code', data: { code: '7654321' } });
|
||||
|
||||
assert.equal(victim.disconnected, true, 'SAS mismatch must abort the session');
|
||||
}
|
||||
|
||||
console.log('Verification gate tests passed');
|
||||
Reference in New Issue
Block a user