Skip to content

Conversation

@crisdosaygo
Copy link

Fix CVE-2024-21526 by adding arg checks in binding.open and removing asserts

Re: 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 - so it can be used in BrowserBox - remote browser isolation and text client for the web), but this is omitted here to avoid affecting existing tests and to minimize changes to the original repo for this PR.

…asserts

Re: 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 <Speaker>.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.
@crisdosaygo
Copy link
Author

crisdosaygo commented Apr 12, 2025

Hi @TooTallNate I like this library and submitted this fix as something I could do for you after making similar changes as part of adapting this library for use in my product. I am using speaker to play a cool little retro startup sound for the text-mode browser (see a demo and hear the tune here: https://youtu.be/02Jw8XhWbvc)

This might not be what you want, and you may not have time to look at this PR anyway - no worries at all, just offering it's helpful for you or speaker users.

Thank you, sir! :)

PS - the source of the fork I use in BrowserBox is at: https://github.com/dosyago-sec/node-speaker and published on NPM as: @browserbox/speaker also under the same (MIT and LGPL-2.1-only) license as your original.

Thank you again and good luck with your current stuff!!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant