-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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.
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
electron: use timingSafeEqual to check token #7308
Conversation
@phosphore Would this make the token comparison safer? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There's no immediate risk here, but it's a desirable security improvement! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks fine to me, it is an improvement over the previous implementation.
I could also not find any regressions when running the electron
application.
871365c
to
efc12b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I quickly confirmed that this change does not impede on the token check - the resulting Electron app still forbids a random browser request to its endpoints 👍
This should prevent an attacker from guessing the Electron token via a timing attack to access an Electron application's local backend services. timingSafeEqual: https://nodejs.org/docs/latest-v10.x/api/crypto.html#crypto_crypto_timingsafeequal_a_b Timing Attack: https://en.wikipedia.org/wiki/Timing_attack Signed-off-by: Paul Maréchal <[email protected]>
efc12b1
to
d9d15b7
Compare
This should prevent an attacker from guessing the Electron token to
access an Electron application's local backend services.
Signed-off-by: Paul Maréchal [email protected]
What it does
Better safe than sorry, this PR addresses a comment made on the token implementation: #7205 (comment)
How to test
Hard to say, it would require trying to execute a purposeful timing attack, which I don't know how to do.
Review checklist
Reminder for reviewers