Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixes out-of-bounds read vulnerability - CWE-125 #8

Closed
wants to merge 1 commit into from

Conversation

mmedal
Copy link

@mmedal mmedal commented May 17, 2018

See https://hackerone.com/reports/321670 for vulnerability details. Ideally this would be fixed with usage of Buffer.from, but we add simple type-checking to keep compatibility with node<4.

@mmedal
Copy link
Author

mmedal commented May 17, 2018

Fixes #7

@@ -27,6 +27,9 @@ StringStream.prototype.write = function(data) {
this.emit('error', err)
return false
}
if (typeof data !== 'string') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right. data can be a Buffer.

Did you mean to check this.fromEncoding instead?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I'm wrong – in any case, a better check is to ensure that data isn't a number

Copy link

@knoxcard knoxcard May 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ummm....how about........ !isNaN(data) ?

@mhart
Copy link
Owner

mhart commented May 17, 2018

Closing in favor of #9

@mhart mhart closed this May 17, 2018
@travispaul
Copy link

For anyone else tracking this down 2 years later, the relevant CVE is: CVE-2018-21270

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants