WebSocket handshake segfaults on master: crypto_hash_many() gets sizeof(hash) where the hash type belongs

[cainbryce]

Summary

On master, every WebSocket upgrade segfaults before a single RFB byte moves. ws_handshake() calls crypto_hash_many() with the old argument list, so the digest size lands in the enum crypto_hash_type parameter, crypto_hash_new() matches no switch case, and the calloc'd update pointer is called while still NULL.

Plain TCP is unaffected.

Versions

wayvnc:  v0.11-dev-3423d09 (master)
neatvnc: v1.1-dev-d0d64f2 (master)   # git describe: v1.0.0-18-gd0d64f2
aml:     v1.0.0

Built with meson setup build -Dbuildtype=debug, crypto backend nettle, on Arch Linux.

Reproduce

Start wayvnc with a ws: listener and connect anything that speaks the WebSocket handshake:

$ wayvnc --output headless-1 ws:127.0.0.1:5900
Info: Listening for connections on 127.0.0.1:5900
Info: New client connection from 127.0.0.1: 0x564dd4b7f440
Segmentation fault (core dumped)

A bare HTTP upgrade is enough — no VNC client required:

$ printf 'GET / HTTP/1.1\r\nHost: 127.0.0.1:5900\r\nUpgrade: websocket\r\nConnection: Upgrade\r\nSec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\nSec-WebSocket-Version: 13\r\n\r\n' | nc 127.0.0.1 5900

Reproduces on every connection. Presence or absence of Sec-WebSocket-Protocol: binary makes no difference. The same server binary serves plain TCP fine (DES auth completes, full framebuffer delivered, process survives), so it is specific to the WebSocket path.

Stack trace

Thread 1 "wayvnc" received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
#0  0x0000000000000000 in ??? ()
#1  0x00007ffff7cab25c in crypto_hash_append (self=0x5555555e84c0,
        src=0x5555555e1180 "dGhlIHNhbXBsZSBub25jZQ==", len=24)
        at ../src/crypto/nettle/hash.c:75
#2  0x00007ffff7cab34e in crypto_hash_many (dst=0x7fffffff2390, type=20,
        src=0x7fffffff2140) at ../src/crypto/nettle/hash.c:97
#3  0x00007ffff7cb392e in ws_handshake (output=0x7fffffff2720, output_maxlen=512,
        input=0x5555555ec718 "GET / HTTP/1.1\r\n...")
        at ../src/stream/ws/handshake.c:82
#4  0x00007ffff7cb573c in stream_ws_read_handshake (...) at ../src/stream/ws/ws.c:202
#5  0x00007ffff7cb58a9 in stream_ws_read (...) at ../src/stream/ws/ws.c:227
#6  0x00007ffff7c9d182 in stream_read (...) at ../src/stream/interface.c:61
#7  0x00007ffff7c8f66a in on_client_event (...) at ../src/server.c:2341
...
rip  0x0  0x0
#1  ... at ../src/crypto/nettle/hash.c:75
75		self->update(&self->ctx, len, src);

Note type=20 in frame #2.

Cause

f97805d ("crypto: Remove hash digest size argument") changed the signature:

-void crypto_hash_many(uint8_t* dst, size_t dst_len, enum crypto_hash_type type,
+void crypto_hash_many(uint8_t* dst, enum crypto_hash_type type,

At the ws_handshake() call site it removed the wrong argument — CRYPTO_HASH_SHA1 instead of sizeof(hash):

 	uint8_t hash[20];
-	crypto_hash_many(hash, sizeof(hash), CRYPTO_HASH_SHA1,
-			(struct crypto_data_entry[]){
+	crypto_hash_many(hash, sizeof(hash), (struct crypto_data_entry[]){

So type is now sizeof(hash) == 20. Both parameters are integers, so the compiler doesn't complain. enum crypto_hash_type only defines 0–3, and crypto_hash_new() has no default: case:

struct crypto_hash* crypto_hash_new(enum crypto_hash_type type)
{
	struct crypto_hash* self = calloc(1, sizeof(*self));
	...
	switch (type) {
	case CRYPTO_HASH_INVALID: ...
	case CRYPTO_HASH_MD5:     ...
	case CRYPTO_HASH_SHA1:    ...
	case CRYPTO_HASH_SHA256:  ...
	}
	return self;   /* type=20: falls through, update/digest still NULL from calloc */
}

crypto_hash_append() then calls self->update(...) — a NULL function pointer.

The other call sites updated in that commit (src/auth/rsa-aes.c, src/auth/apple-dh.c) look correct; this is the only one that lost the type argument.

Fix

-	crypto_hash_many(hash, sizeof(hash), (struct crypto_data_entry[]){
+	crypto_hash_many(hash, CRYPTO_HASH_SHA1, (struct crypto_data_entry[]){

Happy to open a PR if useful.

A default: case in crypto_hash_new() that logs and returns NULL — or PANIC, as CRYPTO_HASH_INVALID already does — would turn this class of mistake into a diagnosable error rather than a jump to address zero.

Possibly unrelated note

I originally hit a segfault with identical symptoms (SIGSEGV/SEGV_MAPERR on every WebSocket upgrade, plain TCP healthy) on the packaged neatvnc 1.0.1 / wayvnc 0.10.1 from Arch. v1.0.1 is not an ancestor of master and does not contain f97805d, and its ws_handshake() still passes CRYPTO_HASH_SHA1 correctly — so that is probably a different cause and I have not root-caused it. Mentioning only in case the WebSocket path has a second problem on the release branch; I can open a separate issue with details if that would help.

[any1]

Fixed.

Arch maintainers have applied patches for nettle 4 compat.

[cainbryce]

Excellent! Much thanks. 😎👍