Uninitialized Memory Exposure

[Ilshidur]

According to Snyk, stringstream has a vulnerability when run on Node.js 4.x and below.

The bug comes from this line : https://github.com/mhart/StringStream/blob/v0.0.5/stringstream.js#L32
More details can be found here : https://hackerone.com/reports/321670

Thank you for the package, by the way !

[knoxcard]

@Ilshidur - love how you remembered to say thanks, I do the same with my PR's. Important to show appreciation to maintainer(s) for their efforts.

[KittyGiraudel]

Hello.

This is also reported by nsp and currently shows up in npm and nsp audits.

Is there an easy way to fix this? :)

[KittyGiraudel]

From what I understand from Snyk’s report, it should be fixable by stringifying data in case it’s a number?

  if (this.fromEncoding) {
    if (Buffer.isBuffer(data)) data = data.toString()
+   if (typeof data === 'number') data = String(data)
    data = new Buffer(data, this.fromEncoding)
  }

Or did I miss something?

[mhart]

It's about the second argument to the Buffer constructor – just need to enforce that it's undefined/null or a string (or not a number)

[KittyGiraudel]

Given the hackerone report, this doesn’t seem to be about the second argument passed to the Buffer constructor.

The problem arises when a number is passed in the stream. [...] On Node.js 4.x and below (4.x is still supported), this exposes uninitialized memory, which could contain sensitive data.

It’s about the fact that on Node 4.x and below, passing a number as a first argument to the Buffer constructor initialises a buffer of a certain length instead of filling it with data.

Isn’t it?

[mhart]

You're absolutely right – my bad. Sorry, I got confused when I was reviewing #8 – which misses the Buffer case

[mhart]

@hugogiraudel #9 should fix it – let me know if you spot any issues with it

[KittyGiraudel]

Perfect, thank you! :)

[mhart]

This has been fixed in [email protected] – thanks for the report and following up all 👍

[marian-r]

@mhart NSP doesn't recognize this as patched https://nodesecurity.io/advisories/664. Has that been patched and if yes how could we mark it as patched?

[mhart]

Yes it's been patched – that's what this issue is about. I don't know anything about nodesecurity.io – maybe @Ilshidur who reported this does?

[Ilshidur]

@mhart

I don't really know about the validation process of nsp and Snyk. Sadly, I couldn't find any indication about marking this package as patched.

However, I sometimes witnessed package authors notifying the Snyk team about their fix.
I guess the only way to find out is to contact the nsp/snyk team.

As you are the maintainer of lots of projects (some of which I love !), I can contact the respective teams about this fix if you want.

[mhart]

Looks like it was @ChALkeR who reported it on hackerone.com – with @lirantal confirming

If one of them could close it, that would be easiest