Skip to content

fix: eval in portableTimingSafeEqual #227

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

Merged
merged 8 commits into from
Oct 15, 2019
Merged

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Oct 14, 2019

eval was added to help ensure that portableTimingSafeEqual
is not optimized into a non-constant time function.
However, the Content Security Policy 'unsafe-eval' will flag this.

It is better to remove this and risk such an optimization,
then to force customers to weaken the Content Security Policy.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

`eval` was added to help ensure that `portableTimingSafeEqual`
is not optimized into a non-constant time function.
However, the Content Security Policy `'unsafe-eval'` will flag this.

It is better to remove this and risk such an optimization,
then to force customers to weaken the Content Security Policy.
@seebees seebees requested a review from a team October 14, 2019 16:35
* it would fail on this eval.
* The value in attempting to ensure this function is not optimized,
* is not worth the cost of making customers to alow `'unsafe-eval'`.
* If you copy this function for your own use, make sure to educate yourself.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a reference for such a person to read up on? Just saying "educate yourself" comes across as dismissive, and I know that was not your intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. https://codahale.com/a-lesson-in-timing-attacks/ is linked above. Perhaps adding please? I was hoping that next sentence might mitigate some of this.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "please review the above link before use"?

@seebees seebees merged commit edd41f2 into aws:master Oct 15, 2019
@seebees seebees deleted the remove_eval branch October 15, 2019 16:33
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.

2 participants