From afae576cc2bffc52fb5473b8d05ec135f4014254 Mon Sep 17 00:00:00 2001 From: DOSAYGO Engineering Date: Sat, 12 Apr 2025 08:11:37 -0400 Subject: [PATCH] Fix CVE-2024-21526 by adding arg checks in binding.open and removing asserts Re: https://github.com/advisories/GHSA-w5fc-gj3h-26rx From the link: > All versions of the package speaker are vulnerable to Denial of Service (DoS) when providing unexpected input types to the channels property of the Speaker object makes it possible to reach an assert macro. Exploiting this vulnerability can lead to a process crash. The changes should prevent the process crashed vuln noted in the CVE. - **Callback Info Check**: Replaced `assert(napi_get_cb_info...)` with a check for `napi_ok` and verified that `argc >= 4`. If this fails, an error is thrown and `NULL` is returned. - **Memory Allocation**: Added a null check for `malloc` of `Speaker`. If allocation fails, a memory error is thrown. - **Channels, Rate, Format**: Replaced `assert` calls with checks for `napi_get_value_int32`. Added input validation (e.g., `channels <= 0` or `rate <= 0` are considered invalid). On failure, the `speaker` is freed and an error is thrown. - **Device String Handling**: Retained the `is_string` check. Replaced `assert` for `napi_get_value_string_utf8` with proper error handling. Added memory allocation checks for `speaker->device`, and ensured both `speaker` and `device` are freed on failure. - **Handle Creation**: Combined checks for `napi_create_object` and `napi_wrap`. If either fails, resources are cleaned up and an error is thrown. - **Cleanup**: Ensured all error paths properly free allocated memory (`speaker->device` and `speaker`) to prevent memory leaks. - **Defensive Vibes**: Added thorough validation at every step. Invalid inputs or failures result in clear error messages to avoid crashes or undefined behavior. - **JavaScript checks**: Also added a JS check for .channels before calling `binding.open` as another layer. The fork this comes from also updates dependencies ([@dosyago-sec/node-speaker](https://github.com/dosyago-sec/node-speaker) - so it can be used in [BrowserBox - remote browser isolation and text client for the web](https://github.com/BrowserBox/BrowserBox)), but this is omitted here to avoid affecting existing tests and to minimize changes to the original repo for this PR. --- index.js | 7 ++++- package.json | 4 +-- src/binding.c | 73 +++++++++++++++++++++++++++++++++++++++++---------- 3 files changed, 67 insertions(+), 17 deletions(-) diff --git a/index.js b/index.js index 5650132..ca44f6d 100644 --- a/index.js +++ b/index.js @@ -98,7 +98,12 @@ class Speaker extends Writable { this.blockAlign = this.bitDepth / 8 * this.channels // initialize the audio handle - // TODO: open async? + const chan = parseInt(this.channels || 2); + if ( Number.isNaN(chan) ) { + this.channels = 2; + } else { + this.channels = chan; + } this.audio_handle = binding.open(this.channels, this.sampleRate, format, this.device) this.emit('open') diff --git a/package.json b/package.json index 61ebe82..327da47 100644 --- a/package.json +++ b/package.json @@ -1,9 +1,9 @@ { "name": "speaker", - "version": "0.5.5", + "version": "0.5.6", "license": "(MIT AND LGPL-2.1-only)", "description": "Output PCM audio data to the speakers", - "author": "Nathan Rajlich (http://tootallnate.net)", + "author": "Nathan Rajlich (http://tootallnate.net), @o0101 and Grok", "repository": "TooTallNate/node-speaker", "main": "index.js", "types": "index.d.ts", diff --git a/src/binding.c b/src/binding.c index 721f85a..cd5634a 100644 --- a/src/binding.c +++ b/src/binding.c @@ -39,46 +39,91 @@ void finalize(napi_env env, void* data, void* hint) { napi_value speaker_open(napi_env env, napi_callback_info info) { size_t argc = 4; napi_value args[4]; - assert(napi_get_cb_info(env, info, &argc, args, NULL, NULL) == napi_ok); + if (napi_get_cb_info(env, info, &argc, args, NULL, NULL) != napi_ok || argc < 4) { + napi_throw_error(env, "ERR_INVALID_ARGS", "Expected 4 arguments"); + return NULL; + } Speaker *speaker = malloc(sizeof(Speaker)); + if (!speaker) { + napi_throw_error(env, "ERR_MEMORY", "Failed to allocate speaker memory"); + return NULL; + } memset(speaker, 0, sizeof(Speaker)); audio_output_t *ao = &speaker->ao; - assert(napi_get_value_int32(env, args[0], &ao->channels) == napi_ok); /* channels */ - int32_t _rate; - assert(napi_get_value_int32(env, args[1], &_rate) == napi_ok); /* sample rate */ - ao->rate = _rate; - assert(napi_get_value_int32(env, args[2], &ao->format) == napi_ok); /* MPG123_ENC_* format */ + int32_t channels; + if (napi_get_value_int32(env, args[0], &channels) != napi_ok || channels <= 0) { + free(speaker); + napi_throw_error(env, "ERR_INVALID_CHANNELS", "Invalid or missing channels"); + return NULL; + } + ao->channels = channels; + + int32_t rate; + if (napi_get_value_int32(env, args[1], &rate) != napi_ok || rate <= 0) { + free(speaker); + napi_throw_error(env, "ERR_INVALID_RATE", "Invalid or missing sample rate"); + return NULL; + } + ao->rate = rate; + + int32_t format; + if (napi_get_value_int32(env, args[2], &format) != napi_ok) { + free(speaker); + napi_throw_error(env, "ERR_INVALID_FORMAT", "Invalid or missing format"); + return NULL; + } + ao->format = format; if (is_string(env, args[3])) { size_t device_string_size; - assert(napi_get_value_string_utf8(env, args[3], NULL, 0, &device_string_size) == napi_ok); - speaker->device = malloc(++device_string_size); - assert(napi_get_value_string_utf8(env, args[3], speaker->device, device_string_size, NULL) == napi_ok); - assert(speaker->device[device_string_size - 1] == 0); + if (napi_get_value_string_utf8(env, args[3], NULL, 0, &device_string_size) != napi_ok) { + free(speaker); + napi_throw_error(env, "ERR_DEVICE_STRING", "Failed to get device string size"); + return NULL; + } + speaker->device = malloc(device_string_size + 1); + if (!speaker->device) { + free(speaker); + napi_throw_error(env, "ERR_MEMORY", "Failed to allocate device string memory"); + return NULL; + } + if (napi_get_value_string_utf8(env, args[3], speaker->device, device_string_size + 1, NULL) != napi_ok) { + free(speaker->device); + free(speaker); + napi_throw_error(env, "ERR_DEVICE_STRING", "Failed to get device string"); + return NULL; + } ao->device = speaker->device; } /* init_output() */ int r = mpg123_output_module_info.init_output(ao); - if (r != 0) { + free(speaker->device); + free(speaker); napi_throw_error(env, "ERR_OPEN", "Failed to initialize output device"); return NULL; } /* open() */ r = ao->open(ao); - if (r != 0) { + free(speaker->device); + free(speaker); napi_throw_error(env, "ERR_OPEN", "Failed to open output device"); return NULL; } napi_value handle; - assert(napi_create_object(env, &handle) == napi_ok); - assert(napi_wrap(env, handle, speaker, finalize, NULL, NULL) == napi_ok); + if (napi_create_object(env, &handle) != napi_ok || + napi_wrap(env, handle, speaker, finalize, NULL, NULL) != napi_ok) { + free(speaker->device); + free(speaker); + napi_throw_error(env, "ERR_WRAP", "Failed to create speaker handle"); + return NULL; + } return handle; }