[WIP] PKCS#7 and CMS with provider side keys by levitte · Pull Request #11422 · openssl/openssl

levitte · 2020-03-27T10:10:39Z

Provider side keys are a bit of a challenge for code that relies on being able to use EVP_PKEY_METHOD and EVP_PKEY_ASN1_METHOD functions directly.

However, it turns out that for the PKCS#7 and CMS, the controls and functions that existed specifically for them were all only used to create relevant X509_ALGOR when and where needed. This was mainly enabled by passing libcrypto structures directly to the method code and letting them deal with those as they wish (potentially opening up for all sorts of more or less dirty hackery).

With providers, it's not possible to share libcrypto internal structures with the provider implementations, so we need another mechanism to achieve the objective, i.e. getting relevant X509_ALGOR for the EVP_PKEY or operation (via contexts) that's used.

This mechanism was already introduced in 6f4b766, but wasn't explained in general terms (or at all), nor was it yet made for general use. The mechanism involves getting a DER encoded AlgorithmIdentifier from the provider implementation as an OSSL_PARAM with the name "algorithm-id" (macro OSSL_PKEY_PARAM_ALGORITHM_ID) and decoding it with d2i_X509_ALGOR() in libcrypto. In this PR, we introduce general internal functionality to get the X509_ALGOR for any operation.

Related to this is the question asked in #11413, where it turns out that CMS has its own set of standards for diverse cases when an X509_ALGOR is needed, which may or may not match what you'd normally get for X.509, so we need a different parameter name specifically for CMS; for the moment, I've chosen "algorithm-id:CMS" (macro OSSL_CMS_PARAM_ALGORITHM_ID). The CMS code is made to try that first, and uses the "normal" parameter name as a fallback (because some CMS standards have signature algorithms that match the X.509 standards, so might as well).

Checklist

documentation is added or updated
tests are added or updated

levitte · 2020-03-27T10:13:05Z

This is currently in WIP because I haven't yet modified the relevant provider implementations to respond to a requested algorithm identifier... let alone with key generation.

levitte · 2020-03-27T10:21:57Z

Regardless of WIP or Travis failures, I'm interested in early comments on the ideas introduced in this PR

crypto/asn1/a_sign.c

mattcaswell · 2020-03-27T10:21:50Z

crypto/cms/cms_env.c

So why #if 0 if its just going to be removed?

This is my way to open that very question, i.e. comments, acclamations, etc welcome

Probing seems reasonable. Again I'd like to hear from @beldmit.

The approach proposed in the #else branch seems fine to me, we should probably just go ahead and drop this #if 0.

This is my way to open that very question, i.e. comments, acclamations, etc welcome

@levitte Hip hip hooray !!!

mattcaswell · 2020-03-27T10:28:57Z

crypto/cms/cms_env.c

This was added recently for GOST CMS support 71434ae, so maybe @beldmit can answer this question.

Sure.

New specification of Russian GOST allows more than one mechanism of the shared key transmission: KeyTransport, KeyAgreement, KeyEncryption. Not all of them are mandatory for implementation so this is a way to request whether the implementation used supports this mechanism.

What I'm thinking is that each of these mechanisms use different methods, so probing with diverse inits should give just as good an answer, and releaves the need to keep details in sync for the provider author.

It seems reasonable. @beldmit does this work for you?

Will check.

@beldmit do you have an update on this?

crypto/cms/cms_sd.c

doc/man3/EVP_PKEY_size.pod

kaduk · 2020-03-29T20:14:22Z

https://tools.ietf.org/html/draft-housley-lamps-cms-update-alg-id-protect-00 (and list discussion thereof) may provide some additional clarity for the use of multiple algorithm IDs within CMS.

beldmit · 2020-03-30T08:43:13Z

Well. I have many failing tests for CMS.

beldmit · 2020-03-30T08:44:57Z

The diagnostics is

Error decrypting CMS using private key
00:C1:24:36:68:7F:00:00:error:CMS routines:CMS_decrypt_set1_pkey_and_peer:no matching recipient:crypto/cms/cms_smime.c:677:

beldmit · 2020-03-30T09:56:48Z

I see two sets of problems for GOST.

GOST engine I use supports both key derivation and key encryption. The code https://github.com/openssl/openssl/pull/11422/files#diff-47b8520df0a2f8fa0433ff37611596f3R1092-R1099 enforces selection according to supported operation implicitly.

Reordering the options (checking first for encryption, then for derivation) does the trick for GOST.
As support of KeyTransRecipientInfo is mandatory according to specification (see below quote in Russian, unfortunately, no text in English yet)

Совместимые реализации, оперирующие сообщениями с типом данных «конверт данных», должны поддерживать следующий функционал при зашифровании и расшифровании:
• Обработка шифрованных сообщений с использованием эфемерного протокола Диффи-Хеллмана;
• Закодирование и раскодирование информации о получателях шифрованного сообщения в виде структур типа KeyTransRecipientInfo;

I'd suggest to leave it this way.

The 2nd problem seems unrelated to this one. It prevents decryption for key agreement and I'm in process of investigations.

beldmit · 2020-03-30T10:20:06Z

The 2nd problem is related to cms_pkey_is_ri_type_supported function. Now it returns the result of comparison with one exact value instead of checking for more than one in ctrl function.

@levitte, feel free to ask for more information. I currently have a reasonable test installation for its work.

crypto/cms/cms_env.c

levitte · 2020-04-07T09:28:27Z

#11450 is in, I've rebased, now I just gotta finish up

romen · 2020-04-07T11:51:25Z

The travis failure seems relevant

$ if test -n "$CHECKDOCS" && ! $make doc-nits; then echo -e '\052\052 FAILED -- MAKE DOC-NITS'; travis_terminate 1; fi
/usr/bin/perl ./util/find-doc-nits -n -l -e
doc/man3/EVP_PKEY_size.pod:1: EVP_PKEY_gettable_params(3) also in NAME section of doc/man3/EVP_PKEY_gettable_params.pod
doc/man3/EVP_PKEY_size.pod:1: The following exist as other .pod files: EVP_PKEY_gettable_params
*** WARNING: No items in =over (at line 22) / =back list at line 27 in file doc/man3/X509_get0_distinguishing_id.pod
*** WARNING: No items in =over (at line 12) / =back list at line 52 in file doc/man7/EVP_MD-common.pod
*** WARNING: No items in =over (at line 19) / =back list at line 25 in file doc/man7/OSSL_PROVIDER-FIPS.pod
*** WARNING: No items in =over (at line 18) / =back list at line 22 in file doc/man7/OSSL_PROVIDER-default.pod
*** WARNING: No items in =over (at line 21) / =back list at line 25 in file doc/man7/OSSL_PROVIDER-legacy.pod
Makefile:1812: recipe for target 'doc-nits' failed
make: *** [doc-nits] Error 1
** FAILED -- MAKE DOC-NITS

romen

I really like this refactoring!

I have a few comments, and a ping for @beldmit for comments to see what we can do to move this forward.

romen · 2020-04-07T13:33:49Z

crypto/cms/cms_env.c

Should we raise some special error for this case?

Because? Mind you, there are error codes to say that this and that other library had an error and serves as a trace... unfortunately, I recently discovered that those codes are messed up...

romen · 2020-04-07T13:40:06Z

crypto/cms/cms_env.c

The approach proposed in the #else branch seems fine to me, we should probably just go ahead and drop this #if 0.

This is my way to open that very question, i.e. comments, acclamations, etc welcome

@levitte Hip hip hooray !!!

crypto/cms/cms_env.c

romen · 2020-04-07T13:41:56Z

crypto/cms/cms_env.c

@beldmit do you have an update on this?

romen · 2020-04-07T13:42:53Z

crypto/cms/cms_sd.c

Should we raise a specific error for this case?

This is a situation where we usually count on the called function to use that already...

doc/man3/EVP_PKEY_size.pod

include/openssl/core_names.h

beldmit · 2020-04-07T14:13:53Z

@levitte, @romen - could you please remind me how to fetch the current state of this PR to make tests? There were hints at the top of the page when a review was requested, but they've disappeared :(
Many thanks!

beldmit · 2020-04-07T14:16:07Z

Found the commands in history.

git fetch origin refs/pull/11422/head
git checkout -b levitte/cms-with-providers FETCH_HEAD

Among others, this benefits PKCS#7 and CMS

This affects not only our SIGNATURE implementations, but also our KEYMGMT implementations.

mattcaswell · 2020-04-08T14:01:38Z

Is this still WIP?

levitte · 2020-04-08T14:14:34Z

Is this still WIP?

Yes. I'm currently testing it merged with #11328, I need to ensure that works first.

levitte · 2020-04-08T14:14:58Z

However, please feel welcome to comment what you see!

I had to do the same of thing with cms_RecipientInfo_kari_encrypt()

levitte · 2020-04-08T18:58:22Z

Oh... wow! The code that's in right now works flawlessly when merged with #11328

levitte · 2020-04-08T19:08:39Z

Oh... wow! The code that's in right now works flawlessly when merged with #11328

... heh, that says nothing. Since it's merged with EC key generation, and we only tell EC with key derivation, for which I've added an EVP_PKEY downgrade hack, it reduces the whole test suite to legacy key only. I wonder what happens if I merge in the DSA key generation PR, which is around, er, somewhere... yes, in #11303.

There was one spot where this function would look at ctx->pmeth directly to determine if it's for RSASSA-PSS, which fails when presented with an EVP_PKEY_CTX holding a provider side key. Switching to use EVP_PKEY_is_a() should make things better.

slontis

Richard I am not going to block this PR,, but have some concerns about whether EVP_PKEY_get_params() should be public or even exist at all..
The one case where you are using it currently it uses a single parameter?

There were multiple reason(s) why I did not add it when I added the individual accessors.
(which I did send around in a mail when I was doing the work).
i.e:

It has some issues related to BIGNUM's (to me this is a fairly significant issue).
figuring out which params have been silently ignored may be important. The individual API's all set the return size internally. I see you have done this trick internally where you have used it. (A end user may not be so fortunate to understand this intricacy)
When it fails it may be harder to figure out which entry failed.

levitte · 2020-04-09T05:11:45Z

I think we need to discuss the issues with OSSL_PARAM / BIGNUM elsewhere. Let's no hijack this PR (or any other PR) for that discussion.

paulidale · 2020-04-09T05:23:46Z

Elsewhere we have EVP_XX_CTX_[gs]et_params. Here the names are ass-about. Can we work on a better name for a start? Just because the underlying naming is rotten does not mean we need to perpetuate the rot.

The BIGNUM concerns do need a separate discussion. Agreed that it will not happen here. BIGNUMs are difficult with the existing OSSL_PARAM framework. The framework mandates extra buffers of the right size before knowing what the actual values are. My opinion is that OSSL_PARAM is broken with respects to this :( I understand why we're byte reordering but it's pain for minimal benefit.

Figuring out which OSSL_PARAMs have been used is a two directional affair, we will need to address this at some point, again not here.

levitte · 2020-04-09T06:28:37Z

BIGNUMs are difficult

I think that sums it up. It comes down to lack of language support.

levitte · 2020-04-09T06:29:26Z

... damnit. NOT hijack this PR, @levitte...

paulidale · 2020-04-09T06:42:48Z

... damnit. NOT hijack this PR, @levitte...

We understood :)

Time is too pressing to hold things up.

h-vetinari · 2021-03-23T17:20:24Z

Is this a topic for beta1...?

t8m · 2021-07-23T14:32:52Z

Is this still relevant? IMO not.

levitte force-pushed the cms-with-providers branch from 34bfb75 to 641193f Compare March 27, 2020 10:11

levitte requested review from DDvO, beldmit and mattcaswell and removed request for mattcaswell March 27, 2020 10:11

levitte mentioned this pull request Mar 27, 2020

EC param / key generation #11328

Closed

mattcaswell requested changes Mar 27, 2020

View reviewed changes

levitte force-pushed the cms-with-providers branch from e860355 to 2ca147b Compare March 27, 2020 12:47

slontis reviewed Mar 27, 2020

View reviewed changes

doc/man3/EVP_PKEY_size.pod Outdated Show resolved Hide resolved

beldmit requested changes Mar 30, 2020

View reviewed changes

crypto/cms/cms_env.c Outdated Show resolved Hide resolved

levitte force-pushed the cms-with-providers branch from 4146d7a to 5558f2b Compare March 31, 2020 14:35

levitte mentioned this pull request Mar 31, 2020

PROV: DER writer #11450

Closed

levitte changed the title ~~[WIP] PKCS#7 and CMS with provider side keys~~ [WIP, pending on #11450] PKCS#7 and CMS with provider side keys Mar 31, 2020

levitte force-pushed the cms-with-providers branch 3 times, most recently from 6ce64d3 to 1e3f62b Compare April 7, 2020 09:26

levitte changed the title ~~[WIP, pending on #11450] PKCS#7 and CMS with provider side keys~~ [WIP] PKCS#7 and CMS with provider side keys Apr 7, 2020

romen suggested changes Apr 7, 2020

View reviewed changes

levitte added 5 commits April 8, 2020 15:59

fixup! PKCS7: Adapt for provider side keys

767753c

fixup! CMS: Adapt for provider side keys

4301242

fixup! CMS: Adapt for provider side keys

e3f13de

PROV: Add diverse digest AlgorithmIdentifiers

84953f9

Among others, this benefits PKCS#7 and CMS

PROV: Add AlgorithmIdentifier responses for CMS

05175ac

This affects not only our SIGNATURE implementations, but also our KEYMGMT implementations.

levitte force-pushed the cms-with-providers branch from ebc5472 to 05175ac Compare April 8, 2020 13:59

levitte added 3 commits April 8, 2020 20:46

fixup! PROV: Add AlgorithmIdentifier responses for CMS

40e3108

fixup! PROV: Add DERlib support for DSA

ab8d8dc

squash! squash! CMS: Adapt for provider side keys

4e43f3b

I had to do the same of thing with cms_RecipientInfo_kari_encrypt()

This was referenced Apr 8, 2020

Add dsa keygen to providers #11303

Closed

EXPERIMENTAL: remove all the legacy bits from a EVP_PKEY #10797

Closed

slontis reviewed Apr 9, 2020

View reviewed changes

h-vetinari mentioned this pull request Mar 23, 2021

Update 3.0 wiki with current status #14663

Closed

t8m added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels Jul 23, 2021

DDvO removed their request for review July 23, 2021 15:22

t8m closed this Jul 3, 2024

Uh oh!

Comments

Conversation

levitte commented Mar 27, 2020

Checklist

Uh oh!

levitte commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Mar 27, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaduk commented Mar 29, 2020

Uh oh!

beldmit commented Mar 30, 2020

Uh oh!

beldmit commented Mar 30, 2020

Uh oh!

beldmit commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beldmit commented Mar 30, 2020

Uh oh!

Uh oh!

levitte commented Apr 7, 2020

Uh oh!

romen commented Apr 7, 2020

Uh oh!

romen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

beldmit commented Apr 7, 2020

Uh oh!

beldmit commented Apr 7, 2020

Uh oh!

mattcaswell commented Apr 8, 2020

Uh oh!

levitte commented Apr 8, 2020

Uh oh!

levitte commented Apr 8, 2020

Uh oh!

levitte commented Apr 8, 2020

levitte commented Mar 27, 2020 •

edited

Loading

beldmit commented Mar 30, 2020 •

edited

Loading