Skip to content

fix: resolve race condition in decodeFrame handling and improve encryption integrity #2182

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
purpshell merged 2 commits into from
Jan 20, 2026
+472 −83
Merged

fix: resolve race condition in decodeFrame handling and improve encryption integrity #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d58e9e4
fix: resolve race condition in decodeFrame handling and improve encry…
jlucaso1 Dec 14, 2025
1c7f376
chore: pr feedback
jlucaso1 Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
216 changes: 133 additions & 83 deletions src/Utils/noise-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,48 @@ import { decodeBinaryNode } from '../WABinary'
import { aesDecryptGCM, aesEncryptGCM, Curve, hkdf, sha256 } from './crypto'
import type { ILogger } from './logger'

const generateIV = (counter: number) => {
const iv = new ArrayBuffer(12)
new DataView(iv).setUint32(8, counter)
const IV_LENGTH = 12

const EMPTY_BUFFER = Buffer.alloc(0)

const generateIV = (counter: number): Uint8Array => {
const iv = new ArrayBuffer(IV_LENGTH)
new DataView(iv).setUint32(8, counter)
return new Uint8Array(iv)
}

class TransportState {
private readCounter = 0
private writeCounter = 0

private readonly iv = new Uint8Array(IV_LENGTH)

constructor(
private readonly encKey: Buffer,
private readonly decKey: Buffer
) {}

encrypt(plaintext: Uint8Array): Uint8Array {
const c = this.writeCounter++
this.iv[8] = (c >>> 24) & 0xff
this.iv[9] = (c >>> 16) & 0xff
this.iv[10] = (c >>> 8) & 0xff
this.iv[11] = c & 0xff
Comment on lines +31 to +36
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The writeCounter and readCounter in TransportState will overflow after 2^32 messages (approximately 4 billion). While this is unlikely in practice, the code should handle overflow gracefully. When the counter reaches MAX_UINT32, it will wrap to 0, potentially reusing IVs and breaking GCM security. Consider adding overflow detection or documenting the maximum message limit.

Copilot uses AI. Check for mistakes.

return aesEncryptGCM(plaintext, this.encKey, this.iv, EMPTY_BUFFER)
}

decrypt(ciphertext: Uint8Array): Buffer {
const c = this.readCounter++
this.iv[8] = (c >>> 24) & 0xff
this.iv[9] = (c >>> 16) & 0xff
this.iv[10] = (c >>> 8) & 0xff
this.iv[11] = c & 0xff

return aesDecryptGCM(ciphertext, this.decKey, this.iv, EMPTY_BUFFER) as Buffer
Comment on lines +24 to +48
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TransportState encrypt and decrypt methods are not thread-safe. The counter increment (readCounter++, writeCounter++) and IV modification operations are separate steps that can be interleaved by concurrent calls. If two encrypt calls execute concurrently, they could both read the same counter value before either increments it, resulting in the same IV being used for two different messages. This violates the security requirements of GCM mode (IV must be unique per message) and could lead to complete compromise of encryption. The same issue exists for concurrent decrypt calls, which would cause decryption failures and incorrect counter state.

Suggested change
private readonly iv = new Uint8Array(IV_LENGTH)
constructor(
private readonly encKey: Buffer,
private readonly decKey: Buffer
) {}
encrypt(plaintext: Uint8Array): Uint8Array {
const c = this.writeCounter++
this.iv[8] = (c >>> 24) & 0xff
this.iv[9] = (c >>> 16) & 0xff
this.iv[10] = (c >>> 8) & 0xff
this.iv[11] = c & 0xff
return aesEncryptGCM(plaintext, this.encKey, this.iv, EMPTY_BUFFER)
}
decrypt(ciphertext: Uint8Array): Buffer {
const c = this.readCounter++
this.iv[8] = (c >>> 24) & 0xff
this.iv[9] = (c >>> 16) & 0xff
this.iv[10] = (c >>> 8) & 0xff
this.iv[11] = c & 0xff
return aesDecryptGCM(ciphertext, this.decKey, this.iv, EMPTY_BUFFER) as Buffer
// Removed shared iv buffer; IV will be generated per call
constructor(
private readonly encKey: Buffer,
private readonly decKey: Buffer
) {}
private encryptMutex = new Mutex();
private decryptMutex = new Mutex();
async encrypt(plaintext: Uint8Array): Promise<Uint8Array> {
return this.encryptMutex.runExclusive(() => {
const c = this.writeCounter++;
const iv = generateIV(c);
return aesEncryptGCM(plaintext, this.encKey, iv, EMPTY_BUFFER);
});
}
async decrypt(ciphertext: Uint8Array): Promise<Buffer> {
return this.decryptMutex.runExclusive(() => {
const c = this.readCounter++;
const iv = generateIV(c);
return aesDecryptGCM(ciphertext, this.decKey, iv, EMPTY_BUFFER) as Buffer;
});

Copilot uses AI. Check for mistakes.
}
}

export const makeNoiseHandler = ({
keyPair: { private: privateKey, public: publicKey },
NOISE_HEADER,
Expand All @@ -27,72 +62,113 @@ export const makeNoiseHandler = ({
}) => {
logger = logger.child({ class: 'ns' })

const data = Buffer.from(NOISE_MODE)
let hash = data.byteLength === 32 ? data : sha256(data)
let salt = hash
let encKey = hash
let decKey = hash
let counter = 0
let sentIntro = false

let inBytes: Buffer = Buffer.alloc(0)

let transport: TransportState | null = null
let isWaitingForTransport = false
let pendingOnFrame: ((buff: Uint8Array | BinaryNode) => void) | null = null

let introHeader: Buffer
if (routingInfo) {
introHeader = Buffer.alloc(7 + routingInfo.byteLength + NOISE_HEADER.length)
introHeader.write('ED', 0, 'utf8')
introHeader.writeUint8(0, 2)
introHeader.writeUint8(1, 3)
introHeader.writeUint8(routingInfo.byteLength >> 16, 4)
introHeader.writeUint16BE(routingInfo.byteLength & 65535, 5)
introHeader.set(routingInfo, 7)
introHeader.set(NOISE_HEADER, 7 + routingInfo.byteLength)
} else {
introHeader = Buffer.from(NOISE_HEADER)
}

const authenticate = (data: Uint8Array) => {
if (!isFinished) {
if (!transport) {
hash = sha256(Buffer.concat([hash, data]))
}
}

const encrypt = (plaintext: Uint8Array) => {
const result = aesEncryptGCM(plaintext, encKey, generateIV(writeCounter), hash)

writeCounter += 1
const encrypt = (plaintext: Uint8Array): Uint8Array => {
if (transport) {
return transport.encrypt(plaintext)
}

const result = aesEncryptGCM(plaintext, encKey, generateIV(counter++), hash)
authenticate(result)
return result
}

const decrypt = (ciphertext: Uint8Array) => {
// before the handshake is finished, we use the same counter
// after handshake, the counters are different
const iv = generateIV(isFinished ? readCounter : writeCounter)
const result = aesDecryptGCM(ciphertext, decKey, iv, hash)

if (isFinished) {
readCounter += 1
} else {
writeCounter += 1
const decrypt = (ciphertext: Uint8Array): Uint8Array => {
if (transport) {
return transport.decrypt(ciphertext)
}

const result = aesDecryptGCM(ciphertext, decKey, generateIV(counter++), hash)
authenticate(ciphertext)
return result
}

const localHKDF = async (data: Uint8Array) => {
const key = await hkdf(Buffer.from(data), 64, { salt, info: '' })
return [key.slice(0, 32), key.slice(32)]
return [key.subarray(0, 32), key.subarray(32)]
}

const mixIntoKey = async (data: Uint8Array) => {
const [write, read] = await localHKDF(data)
salt = write!
encKey = read!
decKey = read!
readCounter = 0
writeCounter = 0
counter = 0
}

const finishInit = async () => {
isWaitingForTransport = true
const [write, read] = await localHKDF(new Uint8Array(0))
encKey = write!
decKey = read!
hash = Buffer.from([])
readCounter = 0
writeCounter = 0
isFinished = true
transport = new TransportState(write!, read!)
isWaitingForTransport = false

logger.trace('Noise handler transitioned to Transport state')

if (pendingOnFrame) {
logger.trace({ length: inBytes.length }, 'Flushing buffered frames after transport ready')
await processData(pendingOnFrame)
pendingOnFrame = null
}
Comment on lines 75 to +144
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finishInit function has a race condition between setting isWaitingForTransport to true and creating the TransportState. If decodeFrame is called during this window, it will buffer data and set pendingOnFrame. Then when finishInit completes and calls processData with the pendingOnFrame callback, a concurrent decodeFrame call could also be executing processData simultaneously, leading to race conditions on the shared inBytes buffer and transport.decrypt calls with incorrect counter values.

Copilot uses AI. Check for mistakes.
}

const data = Buffer.from(NOISE_MODE)
let hash = data.byteLength === 32 ? data : sha256(data)
let salt = hash
let encKey = hash
let decKey = hash
let readCounter = 0
let writeCounter = 0
let isFinished = false
let sentIntro = false
const processData = async (onFrame: (buff: Uint8Array | BinaryNode) => void) => {
let size: number | undefined

while (true) {
if (inBytes.length < 3) return

size = (inBytes[0]! << 16) | (inBytes[1]! << 8) | inBytes[2]!
Comment on lines +148 to +153
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable 'size' is declared outside the while loop but only assigned inside. This is unnecessary and potentially confusing. Declare 'size' inside the loop where it's used to improve code clarity and scope management.

Suggested change
let size: number | undefined
while (true) {
if (inBytes.length < 3) return
size = (inBytes[0]! << 16) | (inBytes[1]! << 8) | inBytes[2]!
while (true) {
if (inBytes.length < 3) return
const size = (inBytes[0]! << 16) | (inBytes[1]! << 8) | inBytes[2]!

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The frame size calculation uses bitwise OR operations but doesn't validate that the result is within reasonable bounds. A malicious or corrupted frame could specify a size larger than available memory (e.g., 0xFFFFFF = ~16MB). Add validation to ensure 'size' is within acceptable limits (e.g., < 10MB) before attempting to process the frame, to prevent memory exhaustion attacks.

Suggested change
// Validate that size is within reasonable bounds (e.g., < 10MB)
const MAX_FRAME_SIZE = 10 * 1024 * 1024; // 10MB
if (size <= 0 || size > MAX_FRAME_SIZE) {
logger.error({ size }, 'Frame size out of bounds, dropping frame');
throw new Boom('Frame size out of bounds', { statusCode: 400 });
}

Copilot uses AI. Check for mistakes.
if (inBytes.length < size + 3) return

let inBytes = Buffer.alloc(0)
let frame: Uint8Array | BinaryNode = inBytes.subarray(3, size + 3)
inBytes = inBytes.subarray(size + 3)

if (transport) {
const result = transport.decrypt(frame)
frame = await decodeBinaryNode(result)
}

if (logger.level === 'trace') {
logger.trace({ msg: (frame as BinaryNode)?.attrs?.id }, 'recv frame')
}

onFrame(frame)
}
}

authenticate(NOISE_HEADER)
authenticate(publicKey)
Expand Down Expand Up @@ -152,67 +228,41 @@ export const makeNoiseHandler = ({
return keyEnc
},
encodeFrame: (data: Buffer | Uint8Array) => {
if (isFinished) {
data = encrypt(data)
if (transport) {
data = transport.encrypt(data)
}

let header: Buffer

if (routingInfo) {
header = Buffer.alloc(7)
header.write('ED', 0, 'utf8')
header.writeUint8(0, 2)
header.writeUint8(1, 3)
header.writeUint8(routingInfo.byteLength >> 16, 4)
header.writeUint16BE(routingInfo.byteLength & 65535, 5)
header = Buffer.concat([header, routingInfo, NOISE_HEADER])
} else {
header = Buffer.from(NOISE_HEADER)
}

const introSize = sentIntro ? 0 : header.length
const frame = Buffer.alloc(introSize + 3 + data.byteLength)
const dataLen = data.byteLength
const introSize = sentIntro ? 0 : introHeader.length
const frame = Buffer.allocUnsafe(introSize + 3 + dataLen)
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer.allocUnsafe is used here for performance, but it doesn't zero-initialize the buffer. While the entire buffer is subsequently filled (introHeader, frame length bytes, and data), if there's any logic error that leaves gaps, uninitialized memory could leak. Consider using Buffer.alloc for security-sensitive operations, or add a comment explaining why allocUnsafe is safe here (all bytes are explicitly set).

Suggested change
const frame = Buffer.allocUnsafe(introSize + 3 + dataLen)
const frame = Buffer.alloc(introSize + 3 + dataLen)

Copilot uses AI. Check for mistakes.

if (!sentIntro) {
frame.set(header)
frame.set(introHeader)
sentIntro = true
}

frame.writeUInt8(data.byteLength >> 16, introSize)
frame.writeUInt16BE(65535 & data.byteLength, introSize + 1)
frame[introSize] = (dataLen >>> 16) & 0xff
frame[introSize + 1] = (dataLen >>> 8) & 0xff
frame[introSize + 2] = dataLen & 0xff

frame.set(data, introSize + 3)

return frame
},
decodeFrame: async (newData: Buffer | Uint8Array, onFrame: (buff: Uint8Array | BinaryNode) => void) => {
// the binary protocol uses its own framing mechanism
// on top of the WS frames
// so we get this data and separate out the frames
const getBytesSize = () => {
if (inBytes.length >= 3) {
return (inBytes.readUInt8() << 16) | inBytes.readUInt16BE(1)
}
if (isWaitingForTransport) {
inBytes = Buffer.concat([inBytes, newData])
pendingOnFrame = onFrame
return
Comment on lines +253 to +256
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pendingOnFrame variable can only store a single callback, but if multiple decodeFrame calls occur while isWaitingForTransport is true, each subsequent call will overwrite the previous pendingOnFrame callback. This means only the last callback will be invoked when finishInit completes, and frames buffered by earlier calls will be lost. The implementation should either queue all callbacks or prevent concurrent calls during transport initialization.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@Santosl2 Santosl2 Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make sense? @jlucaso1

Copy link
Collaborator Author

@jlucaso1 jlucaso1 Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific one no. Javascript is single thread so we are safe with this

}

inBytes = Buffer.concat([inBytes, newData])

logger.trace(`recv ${newData.length} bytes, total recv ${inBytes.length} bytes`)

let size = getBytesSize()
while (size && inBytes.length >= size + 3) {
let frame: Uint8Array | BinaryNode = inBytes.slice(3, size + 3)
inBytes = inBytes.slice(size + 3)

if (isFinished) {
const result = decrypt(frame)
frame = await decodeBinaryNode(result)
}

logger.trace({ msg: (frame as BinaryNode)?.attrs?.id }, 'recv frame')

onFrame(frame)
size = getBytesSize()
if (inBytes.length === 0) {
inBytes = Buffer.from(newData)
} else {
inBytes = Buffer.concat([inBytes, newData])
}

await processData(onFrame)
}
}
}
Loading
Loading