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

Fix PEM_write_bio_PrivateKey_traditional() to not output PKCS#8 #12728

Conversation

levitte
Copy link
Member

@levitte levitte commented Aug 27, 2020

PEM_write_bio_PrivateKey_traditional() uses i2d_PrivateKey() to do the
actual encoding to DER. However, i2d_PrivateKey() is a generic
function that will do what it can to produce output according to what
the associated EVP_PKEY_ASN1_METHOD offers. If that method offers a
function 'old_priv_encode', which is expected to produce the
"traditional" encoded form, then i2d_PrivateKey() uses that. If not,
i2d_PrivateKey() will go on and used more modern methods, which are
all expected to produce PKCS#8.

To ensure that PEM_write_bio_PrivateKey_traditional() never produces
more modern encoded forms, an extra check that 'old_priv_encode' is
non-NULL is added. If it is NULL, an error is returned.

PEM_write_bio_PrivateKey_traditional() uses i2d_PrivateKey() to do the
actual encoding to DER.  However, i2d_PrivateKey() is a generic
function that will do what it can to produce output according to what
the associated EVP_PKEY_ASN1_METHOD offers.  If that method offers a
function 'old_priv_encode', which is expected to produce the
"traditional" encoded form, then i2d_PrivateKey() uses that.  If not,
i2d_PrivateKey() will go on and used more modern methods, which are
all expected to produce PKCS#8.

To ensure that PEM_write_bio_PrivateKey_traditional() never produces
more modern encoded forms, an extra check that 'old_priv_encode' is
non-NULL is added.  If it is NULL, an error is returned.
@levitte levitte added branch: master Merge to master branch approval: review pending This pull request needs review by a committer branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Aug 27, 2020
@levitte
Copy link
Member Author

levitte commented Aug 27, 2020

A backport to 1.1.1 will come in a separate PR

@levitte
Copy link
Member Author

levitte commented Aug 27, 2020

Backport to 1.1.1 in #12729

@levitte
Copy link
Member Author

levitte commented Aug 27, 2020

... and with this, we discover that not all of our EVP_PKEY_ASN1_METHOD instances support "traditional" PEM or DER output... One example: DH

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.

Approved if CI passes.

@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 branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Aug 27, 2020
@levitte levitte added this to In progress in 3.0 New Core + FIPS via automation Aug 28, 2020
@levitte levitte moved this from In progress to Reviewer approved in 3.0 New Core + FIPS Aug 28, 2020
@levitte levitte added this to the 3.0.0 milestone Aug 28, 2020
@levitte levitte removed this from Reviewer approved in 3.0 New Core + FIPS Aug 28, 2020
@levitte levitte removed this from the 3.0.0 milestone Aug 28, 2020
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Aug 28, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Aug 28, 2020
@levitte
Copy link
Member Author

levitte commented Aug 28, 2020

Merged

87d91d2 Fix PEM_write_bio_PrivateKey_traditional() to not output PKCS#8
bddfea0 TEST: Adapt some tests for a stricter PEM_write_bio_PrivateKey_traditional()

@levitte levitte closed this Aug 28, 2020
openssl-machine pushed a commit that referenced this pull request Aug 28, 2020
PEM_write_bio_PrivateKey_traditional() uses i2d_PrivateKey() to do the
actual encoding to DER.  However, i2d_PrivateKey() is a generic
function that will do what it can to produce output according to what
the associated EVP_PKEY_ASN1_METHOD offers.  If that method offers a
function 'old_priv_encode', which is expected to produce the
"traditional" encoded form, then i2d_PrivateKey() uses that.  If not,
i2d_PrivateKey() will go on and used more modern methods, which are
all expected to produce PKCS#8.

To ensure that PEM_write_bio_PrivateKey_traditional() never produces
more modern encoded forms, an extra check that 'old_priv_encode' is
non-NULL is added.  If it is NULL, an error is returned.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12728)
openssl-machine pushed a commit that referenced this pull request Aug 28, 2020
…ional()

- test/endecode_test.c

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #12728)
@levitte levitte deleted the fix-PEM_write_bio_PrivateKey_traditional-1 branch August 28, 2020 18:49
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
PEM_write_bio_PrivateKey_traditional() uses i2d_PrivateKey() to do the
actual encoding to DER.  However, i2d_PrivateKey() is a generic
function that will do what it can to produce output according to what
the associated EVP_PKEY_ASN1_METHOD offers.  If that method offers a
function 'old_priv_encode', which is expected to produce the
"traditional" encoded form, then i2d_PrivateKey() uses that.  If not,
i2d_PrivateKey() will go on and used more modern methods, which are
all expected to produce PKCS#8.

To ensure that PEM_write_bio_PrivateKey_traditional() never produces
more modern encoded forms, an extra check that 'old_priv_encode' is
non-NULL is added.  If it is NULL, an error is returned.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12728)
swenkeratmicrosoft pushed a commit to swenkeratmicrosoft/openssl that referenced this pull request Sep 1, 2020
…ional()

- test/endecode_test.c

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from openssl#12728)
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 Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants