Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

buffer: don't compare same buffers #742

Closed
wants to merge 1 commit into from
Closed

buffer: don't compare same buffers #742

wants to merge 1 commit into from

Conversation

vkurchatkin
Copy link
Contributor

@aredridel
Copy link
Contributor

This has potential information leak implications via timing attack.

@vkurchatkin
Copy link
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
Member

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

@vkurchatkin
Copy link
Contributor Author

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

@bnoordhuis
Copy link
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
Contributor Author

added some tests

@bnoordhuis
Copy link
Member

LGTM

1 similar comment
@trevnorris
Copy link
Contributor

LGTM

vkurchatkin added a commit that referenced this pull request Feb 6, 2015
@vkurchatkin @micnic
buffer: don't compare same buffers
1cd1d7a 
PR-URL: #742

Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@micnic
Copy link
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
@feross
don't compare same buffers
ffccb1a 
Copied from iojs: nodejs/node#742
itu2n1i8w added a commit to itu2n1i8w/buffer that referenced this pull request Aug 5, 2024
@itu2n1i8w @feross
don't compare same buffers
a704fcf 
Copied from iojs: nodejs/node#742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vkurchatkin @aredridel @bnoordhuis @trevnorris @micnic