discordjs / opus

Native opus bindings for node
MIT License
192 stars 55 forks source link

[SECURITY] DoS in @discordjs/opus due to invalid encoders/buffers (CVE-2022-25345) #131

Closed espimarisa closed 2 years ago

espimarisa commented 2 years ago

Hey all!

Just opening this issue to make sure the Discord.js team is aware (if y'all aren't already) of CVE-2022-25345 affecting the latest version of @discordjs/opus.

PoC below is taken from the snyk page.

const { OpusEncoder } = require('@discordjs/opus'); 
const encoder = new OpusEncoder(48000, 0); 

try { 
  const encoded = encoder.encode(Buffer.from("aaa")); 
} catch(e) {
  console.log("This will never run") // never executed because of the hard crash }
}
const { OpusEncoder } = require('@discordjs/opus'); 
const encoder = new OpusEncoder(48000, 2); 

try {
  const encoded = encoder.encode(null); 
} catch(e) { 
  console.log("This will never run") // never executed because of the hard crash }
}

Priority should be pretty high in order to mitigate this (and to make pnpm, Dependabot, etc. be quiet).

Additionally, I was not the one to find this issue and I take no credit for it - I am just simply raising an issue to get it resolved quicker hopefully.

Thanks!

kyranet commented 2 years ago

On the one hand, this library was never designed to take user input, but rather, to be checked by the user, which makes this issue mostly impossible under the conditions it was written for. After all, this is a 0-layer wrapper around the Opus C library.

On the other hand, they're checks for JS values, so it's not far-fetched (IMO) to do some checks, all it takes to fix this CVE is a range check here: https://github.com/discordjs/opus/blob/fbd68eeca7637559bf3141b0ae76d48873b8d18a/src/node-opus.cc#L48 And a null check here: https://github.com/discordjs/opus/blob/fbd68eeca7637559bf3141b0ae76d48873b8d18a/src/node-opus.cc#L89 (Why does null make IsBuffer return true... sigh).

Edit: It turns out we need a return, a PR will be made to address this.


Also, the CVE's severity is too high IMHO, it's inconceivable for an application to run under such edge cases, both allowing user input for the channels (specially unsanitized), and allowed user input that isn't a buffer (I would expect most libraries to return buffers even if they're empty, and never null).