Uncatchable error for close message of 123 bytes because of encoding issues

[viyaha]

• I've searched for any related issues and avoided creating a duplicate issue.

Description

If the other side has a problem and closes the websocket-connection with an message this message must not be greater than 123 bytes, otherwise an error will be thrown (that is not catchable by the client code, there needs to be an "unhandled exception handler" for those errors.

The issue is, the message itself is exactly 123 bytes long (and shouldn't throw), but the byte-message will be converted to a string before checking the bytelength of the string. If there are encoding issues some � will be inserted in the string, afterwards the bytelength of the string is greater than 123 and the error is thrown.

String-Conversion:

ws/lib/receiver.js

Line 473 in 114de9e

this.emit('conclude', code, buf.toString());

ByteLength-Check:

ws/lib/sender.js

Lines 117 to 121 in 114de9e

	const length = Buffer.byteLength(data);
	if (length > 123) {
	throw new RangeError('The message must not be greater than 123 bytes');
	}

Reproducible in:

• version: 7.4.4
• Node.js version(s): v14.15.0
• OS version(s): Windows 10 Pro 19042.867

Steps to reproduce:

Easiest way to check this issue is pure node (without the package):

// we're staying with �, just to make it easier, � => "efbfbd" ===> "efbf" is not valid and node will replace it with �, because of the encoding issues
const firstBuffer = Buffer.from("efbf", "hex");
const secondBuffer = Buffer.from("efbfbd", "hex");
const firstString = firstBuffer.toString();
const secondString = secondBuffer.toString();

console.log(firstBuffer.byteLength); // 2
console.log(secondBuffer.byteLength); // 3
console.log(Buffer.byteLength(firstString)); // 3
console.log(Buffer.byteLength(secondString)); // 3

console.log(firstString === secondString); // true � === �

This happends quite easily, if the "closer" doesn't validate the utf8-correctness of the shortened byte-message. This happens if the message just get cuts after 123 bytes => removes a byte from a character.

Expected result:

Either allow it (check the bytelength of the Buffer before converting it to string) or throw the error in the caller stack => The user needs to be able to handle this.

Actual result:

Uncatchable Error (outside of caller stack).

[lpinca]

The close reason must be UTF-8 encoded. This check

ws/lib/sender.js

Lines 117 to 121 in a74dd2e

	const length = Buffer.byteLength(data);
	if (length > 123) {
	throw new RangeError('The message must not be greater than 123 bytes');
	}

is only used when you directly calls ws.close() and throwing that error is intended because it means you are using ws.close() incorrectly.

That code cannot be reached if the other peer sends an invalid close frame because:

1. A control frame must have a payload length of 125 bytes or less. This is checked here

   ws/lib/receiver.js

   Lines 215 to 223 in a74dd2e

   	if (this._payloadLength > 0x7d) {
   	this._loop = false;
   	return error(
   	RangeError,
   	`invalid payload length ${this._payloadLength}`,
   	true,
   	1002
   	);
   	}

   and an eventually an error is emitted
2. If the close frame has a body the first two bytes are the status code and the remaining bytes are close reason. This is checked here

   ws/lib/receiver.js

   Lines 460 to 475 in a74dd2e

   	} else {
   	const code = data.readUInt16BE(0);
   	if (!isValidStatusCode(code)) {
   	return error(RangeError, `invalid status code ${code}`, true, 1002);
   	}
   	const buf = data.slice(2);
   	if (!isValidUTF8(buf)) {
   	return error(Error, 'invalid UTF-8 sequence', true, 1007);
   	}
   	this.emit('conclude', code, buf.toString());
   	this.end();
   	}

   and an error is eventually emitted.

So this line

ws/lib/receiver.js

Line 473 in a74dd2e

this.emit('conclude', code, buf.toString());

can only give you an UTF-8 encoded close reason string whose byte length is 123 bytes or less.

[mika-fischer]

The issue seems to be that we don't have utf-8-validate installed (since it is optional), which makes isValidUTF8 a no-op that always returns true. In our case of invalid UTF-8 buf.toString() can then cause the byte length of the reason to expand beyond 123 bytes due to insertion of unicode replacement characters. The handler for the conclude event then calls close with the reason string that's now too long and that causes the RangeError to be thrown from within the EventEmitter and no way for the user to catch it.

Needless to say this is a pretty serious denial-of-service issue as it will normally bring the whole process down and is trivially exploitable by an attacker.

So at the very least the exception needs to be caught in receiverOnConclude and handled in some manner. Or the reason should be sent back without any transformation. Or the UTF-8 validation should be mandatory. But catching the exception seems prudent in any case...

[lpinca]

Ok, it makes sense. Thank you.

Or the UTF-8 validation should be mandatory.

This. I'll make isValidUTF8 use the fallback code of utf-8-validate if it is not installed.