Skip to content

Comments

[EC][ASN1] Detect missing OID when serializing EC parameters and keys#12313

Closed
romen wants to merge 4 commits intoopenssl:masterfrom
romen:issues/3.0.0/ec_asn1_handle_no_OID
Closed

[EC][ASN1] Detect missing OID when serializing EC parameters and keys#12313
romen wants to merge 4 commits intoopenssl:masterfrom
romen:issues/3.0.0/ec_asn1_handle_no_OID

Conversation

@romen
Copy link
Member

@romen romen commented Jun 29, 2020

The following built-in curves do not have an assigned OID:

  • Oakley-EC2N-3
  • Oakley-EC2N-4

In general we shouldn't assume that an OID is always available.

This commit detects such cases, raises an error and returns appropriate
return values so that the condition can be detected and correctly
handled by the callers, when serializing EC parameters or EC keys with
the default ec_param_enc:named_curve.

Fixes #12306

This is the analogous of #12312 for master: they differ for the make update commit.

romen added 2 commits June 29, 2020 01:02
The following built-in curves do not have an assigned OID:

- Oakley-EC2N-3
- Oakley-EC2N-4

In general we shouldn't assume that an OID is always available.

This commit detects such cases, raises an error and returns appropriate
return values so that the condition can be detected and correctly
handled by the callers, when serializing EC parameters or EC keys with
the default `ec_param_enc:named_curve`.

Fixes openssl#12306
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

LGTM

}

if ((dsize = i2d(x, NULL)) < 0) {
if ((dsize = i2d(x, NULL)) <= 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice the discussion at #12312 (comment)

…eters and keys

Detect missing OID in serializer implementation
@romen
Copy link
Member Author

romen commented Jun 29, 2020

#12307 revealed that the new provider implementation also did not account for missing OIDs: f13ba54 addresses that.

…C parameters and keys

EC_R_MISSING_OID -> PROV_R_MISSING_OID
@romen
Copy link
Member Author

romen commented Jun 29, 2020

Inside the provider we can't use EC_R_MISSING_OID (there is a clash with another provider specific error), so we add PROV_R_MISSING_OID.

See fa55d57

@romen romen requested a review from t8m July 2, 2020 14:44
@romen
Copy link
Member Author

romen commented Jul 2, 2020

@t8m can you reapprove?

@levitte you had concerns (discussed in the 1.1.1 version of this PR), I could also use your feedback on the extra bits I added in 3.0.0 that are related to serializer code.

@t8m t8m 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 Jul 2, 2020
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jul 3, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

romen added a commit to romen/openssl that referenced this pull request Jul 4, 2020
The following built-in curves do not have an assigned OID:

- Oakley-EC2N-3
- Oakley-EC2N-4

In general we shouldn't assume that an OID is always available.

This commit detects such cases, raises an error and returns appropriate
return values so that the condition can be detected and correctly
handled by the callers, when serializing EC parameters or EC keys with
the default `ec_param_enc:named_curve`.

Fixes openssl#12306

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#12313)
openssl-machine pushed a commit that referenced this pull request Jul 7, 2020
The following built-in curves do not have an assigned OID:

- Oakley-EC2N-3
- Oakley-EC2N-4

In general we shouldn't assume that an OID is always available.

This commit detects such cases, raises an error and returns appropriate
return values so that the condition can be detected and correctly
handled by the callers, when serializing EC parameters or EC keys with
the default `ec_param_enc:named_curve`.

Fixes #12306

Reviewed-by: Tomas Mraz <[email protected]>
(Merged from #12313)
@romen
Copy link
Member Author

romen commented Jul 7, 2020

Merged to master as:

  • e0137ca [EC][ASN1] Detect missing OID when serializing EC parameters and keys

Thanks!

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

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

genpkey -genparam fails for some EC curves

3 participants