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.

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

Improve testing of elliptic curve validation #2544

Closed
wants to merge 2 commits into from

Conversation

levitte
Copy link
Member

@levitte levitte commented Feb 2, 2017

This involves:

  • A directory of valid and invalid PEM-encoded curves.
    This is non-exhaustive and can be added to.
  • A test recipe (test/recipes/15-test_ecparams.t) which uses 'openssl
    ecparam' to check the test vectors.
  • A minor patch to 'openssl ecparam' to make it exit non-zero
    when curve validation fails.
Checklist
  • tests are added or updated
Description of change

Fixes #2375

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 2, 2017
@levitte
Copy link
Member Author

levitte commented Feb 2, 2017

FYI, I've emailed Joseph for a CLA.

ctz and others added 2 commits February 3, 2017 00:45
This involves:

- A directory of valid and invalid PEM-encoded curves.
  This is non-exhaustive and can be added to.
- A minor patch to 'openssl ecparam' to make it exit non-zero
  when curve validation fails.

- A test recipe is added in a separate commit.
Add a test recipe (test/recipes/15-test_ecparams.t) which uses 'openssl
ecparam' to check the test vectors.
@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Feb 2, 2017
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

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

once the CLA issue is sorted out ...

@levitte levitte added the branch: master Merge to master branch label Feb 3, 2017
@dot-asm
Copy link
Contributor

dot-asm commented Feb 8, 2017

Side note. I'm not sure about placement of this test in the test sequence. Or rather I don't like that its position depends on alphabetic order of script names when we have more explicit way to arrange tests in place. I mean if it's supposed to be performed after basic EC tests, then it should be numbered accordingly.

@levitte
Copy link
Member Author

levitte commented Feb 8, 2017

Ping @ctz

@levitte
Copy link
Member Author

levitte commented Feb 8, 2017

@dot-asm, I hear you. That should apply to the test_ecdh and test_ecdsa tests as well then, I assume? On my machine, they come before test_ec as well...

@dot-asm
Copy link
Contributor

dot-asm commented Feb 8, 2017

That should apply to the test_ecdh and test_ecdsa tests as well then

Yeah. In sense that [as far as I recall] it wasn't the original intention to rely on alphabetic sorting on each testing "level" [denoted by number in the beginning of recipe name].

On my machine, they [ecdh and ecdsa] come before test_ec as well...

??? I don't follow. I was under impression that at each level they are sorted alphabetically anyway. And I can see that test/run_tests.pl does sort, so that ecdh and ecdsa should run after ec, and the do on my machine. Once again, this comment is not about "bad" order [at least not on my computer], but [unintentional] reliance on alphabetic sorting on each level... And it was a side note, i.e. there is no expectation that it will be acted upon...

@levitte
Copy link
Member Author

levitte commented Feb 8, 2017

Nothing stops anyone from doing a bit of creative numbering, such as 15-test_ecdh.t -> 16-test_ecdh.t. There is space between the currently allocated numbers for a reason, I fully intended to leave space for sorting purposes when I set up the number scheme.

@richsalz richsalz removed the hold: cla required The contributor needs to submit a license agreement label Mar 18, 2017
@richsalz
Copy link
Contributor

CLA recorded; @levitte you want to merge?

@levitte
Copy link
Member Author

levitte commented Mar 18, 2017

I'm off desktop this weekend, so feel free

levitte pushed a commit that referenced this pull request Mar 20, 2017
This involves:

- A directory of valid and invalid PEM-encoded curves.
  This is non-exhaustive and can be added to.
- A minor patch to 'openssl ecparam' to make it exit non-zero
  when curve validation fails.

- A test recipe is added in a separate commit.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #2544)
levitte added a commit that referenced this pull request Mar 20, 2017
Add a test recipe (test/recipes/15-test_ecparams.t) which uses 'openssl
ecparam' to check the test vectors.

Reviewed-by: Rich Salz <rsalz@openssl.org>
(Merged from #2544)
@levitte
Copy link
Member Author

levitte commented Mar 20, 2017

Merged. 6d0b5ee and 691e302

@levitte levitte closed this Mar 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[rt.openssl.org #3939] [PATCH] Tests for CVE-2015-1788
5 participants