Skip to content

ec key validation checks updated #8564

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
Closed
slontis wants to merge 1 commit into from
Closed

ec key validation checks updated #8564

slontis wants to merge 1 commit into from

Conversation

@slontis
Copy link
Member

@slontis slontis commented Mar 24, 2019 •
edited
Loading

Checklist
  • documentation is added or updated
  • tests are added or updated

@romen romen mentioned this pull request Mar 25, 2019
Closed
2 tasks
kroeckx
kroeckx reviewed Mar 25, 2019
View reviewed changes
kroeckx
kroeckx reviewed Mar 25, 2019
View reviewed changes
davidben
davidben reviewed Mar 25, 2019
View reviewed changes
romen
romen reviewed Mar 27, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

Nice work! I have a few minor comments for this PR.

@romen romen self-assigned this Mar 27, 2019
@romen romen added the branch: master Applies to master branch label Mar 27, 2019
romen
romen suggested changes Apr 3, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

Two small comments!

@slontis
Copy link
Member Author

slontis commented Apr 5, 2019

ping

romen
romen approved these changes Apr 5, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again @slontis

@romen romen added the approval: review pending This pull request needs review by a committer label Apr 5, 2019
@bbbrumley
Copy link
Contributor

bbbrumley commented Apr 5, 2019

ping

Yea nice and modular here @slontis ! Looks good, although my vote doesn't count 😉

mattcaswell
mattcaswell requested changes Apr 5, 2019
View reviewed changes
@slontis
Copy link
Member Author

slontis commented Apr 8, 2019

Can you please review again @mattcaswell

@slontis
Copy link
Member Author

slontis commented Apr 11, 2019

rebased to fix conflict in ectest.c

@mattcaswell mattcaswell added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 11, 2019
romen
romen approved these changes Apr 11, 2019
View reviewed changes
Copy link
Member

@romen romen left a comment

Choose a reason for hiding this comment

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

Reconfirm after force-push.

@romen
Copy link
Member

romen commented Apr 11, 2019

Merged in master as 5173cdd!

Thanks again everyone!

@romen romen closed this Apr 11, 2019
levitte pushed a commit that referenced this pull request Apr 11, 2019
@slontis
ec key validation checks updated
5173cdd 
Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #8564)
@dlegaultbbry
Copy link

dlegaultbbry commented Jun 11, 2019

Can this be promoted to 1.1.1? There's a bunch of improvements that slontis added which I think would also be useful to be merged in 1.1.1 (#8557, #8555)

@slontis
Copy link
Member Author

slontis commented Jun 11, 2019

ping @paulidale or @mattcaswell : Should this be merged to 1.1.1?

@paulidale
Copy link
Contributor

paulidale commented Jun 12, 2019

I don't see a pressing need.
They don't seem to be bug fixes.

@mattcaswell
Copy link
Member

mattcaswell commented Jun 12, 2019

I agree with @paulidale.

@dlegaultbbry
Copy link

dlegaultbbry commented Jun 12, 2019

Well they are kind of security related in that they intercept invalid keys. I'm actually surprised that there isn't much checking being done against the invalid keys found in these vectors. DSA also has no check functions implemented.

https://github.com/google/wycheproof/tree/master/java/com/google/security/wycheproof/testcases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kroeckx kroeckx kroeckx left review comments

@mattcaswell mattcaswell mattcaswell approved these changes

+3 more reviewers

@davidben davidben davidben left review comments

@bbbrumley bbbrumley bbbrumley left review comments

@romen romen romen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@romen romen

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@slontis @bbbrumley @romen @dlegaultbbry @paulidale @mattcaswell @davidben @kroeckx