Skip to content

buffer: don't compare same buffers#742

Closed
vkurchatkin wants to merge 1 commit into
nodejs:v1.xfrom
vkurchatkin:buffer-compare
Closed

buffer: don't compare same buffers#742
vkurchatkin wants to merge 1 commit into
nodejs:v1.xfrom
vkurchatkin:buffer-compare

Conversation

@vkurchatkin
Copy link
Copy Markdown
Contributor

@aredridel
Copy link
Copy Markdown
Contributor

This has potential information leak implications via timing attack.

@vkurchatkin
Copy link
Copy Markdown
Contributor Author

Don't think it does: the underlying implementation uses memcmp which I think is vulnerable to timing attack as it is, so nothing new.

Also I don't see way for attacker to trigger this branch, as it requires to send the same buffer object somehow.

@bnoordhuis
Copy link
Copy Markdown
Member

@vkurchatkin LGTM but can you add one or two tests?

@vkurchatkin
Copy link
Copy Markdown
Contributor Author

@bnoordhuis just test comparing same objects? I couldn't imagine a failing test without reaching internals

@bnoordhuis
Copy link
Copy Markdown
Member

@vkurchatkin Just some regression tests in case someone horribly botches a merge and accidentally changes the return 0 to return 42 (to make up a really far-fetched example.)

@vkurchatkin
Copy link
Copy Markdown
Contributor Author

added some tests

@bnoordhuis
Copy link
Copy Markdown
Member

LGTM

1 similar comment
@trevnorris
Copy link
Copy Markdown
Contributor

LGTM

vkurchatkin added a commit that referenced this pull request Feb 6, 2015
PR-URL: #742

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@micnic
Copy link
Copy Markdown
Contributor

micnic commented Feb 6, 2015

Landed in 1cd1d7a

@micnic micnic closed this Feb 6, 2015
feross added a commit to feross/buffer that referenced this pull request Feb 10, 2015
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.

5 participants