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

CMS working with external provider? #17717

Closed
baentsch opened this issue Feb 17, 2022 · 19 comments
Closed

CMS working with external provider? #17717

baentsch opened this issue Feb 17, 2022 · 19 comments
Labels
branch: master Merge to master branch help wanted triaged: feature The issue/pr requests/adds a feature

Comments

@baentsch
Copy link
Contributor

In trying to run openssl cms (most current master branch on Linux) with an external provider (oqs-provider) several issues let me ask the question: Is CMS meant/tested to work with an external provider (running non-default/classic crypto)?

Issue 1: When not passing a MessageDigest to the cms call, the function EVP_PKEY_get_default_digest_nid gets called and as it returns 0 processing at that point ends. This is IMO not quite OK, as OQS algorithms permissibly work without (default) digests.

Issue 2: When passing an MD explicitly to the CMS invocation (e.g., openssl cms -in inputfile -sign -signer dilithium2.crt -inkey dilithium2.key -nodetach -outform pem -binary -out signedfile.cms -md sha512 -provider oqsprovider -provider default) , the provider gets correctly called, the signature internally gets created by the provider, but the resulting CMS file signedfile.cms looks as follows:

-----BEGIN CMS-----
-----END CMS-----

-> No error is output by the above call (even the return code is surprisingly OK), but when debugging into the issue it seems PEM_write_bio_CMS_stream fails to write out the complete CMS structure (possibly containing elements arguably not handled by the default provider). Unexpected is that none of the functions of the OQS-provider are called. It seems ASN1_item_ex_i2d (eventually called within PEM_write_bio_CMS_stream) silently fails with a -1 resulting in a failure to write any data -> This lets me ask: Is this logic geared to work with an external provider and is our provider making a mistake somehow causing it to be not called?

The OQS-provider (incl. encode/decode) functions have been tested to work OK by creating (and verifying) OQS-certificates against our OQS-OpenSSL1.1.1 fork: The PEM structures are created (read/write) OK; certificates work fine in, e.g., nginx, installations.

The thing confusing me most is that none of the encode/decode functions seems to be called during the core CMS sign/write operations. When loading the cert for signature they are called, letting me be certain that the provider logic is invoked and works OK during the "certificate & key reading part" of CMS operations. No provider calls occur when the CMS structure is written out, though.

@baentsch baentsch added the issue: question The issue was opened to ask a question label Feb 17, 2022
@t8m
Copy link
Member

t8m commented Feb 17, 2022

Full support of additional algorithms with CMS is definitely not there and would be a kind-of major feature. It would be something that we would surely like to have but is currently not a priority for the next release.

@t8m t8m added branch: master Merge to master branch help wanted triaged: feature The issue/pr requests/adds a feature and removed issue: question The issue was opened to ask a question labels Feb 17, 2022
@t8m t8m added this to the Post 3.1.0 milestone Feb 17, 2022
@baentsch
Copy link
Contributor Author

Full support of additional algorithms with CMS is definitely not there and would be a kind-of major feature. It would be something that we would surely like to have but is currently not a priority for the next release.

Thanks for the feedback and expectation management. Are the required changes something someone "external" like me could take on (with some github-based pointers/help from the core OpenSSL team) or is this unlikely to be successful (or would a corresponding PR anyway not be reviewed due to other priorities)?

@t8m
Copy link
Member

t8m commented Feb 17, 2022

It would not be an easy task for an external contributor but IMO it should be doable. There might be some hurdles but we would help with those.

Unfortunately with the current priorities in mind I cannot promise you how much time the core OpenSSL team will be able to spend on this.

@baentsch
Copy link
Contributor Author

Understood. Thanks again for the openness. So let's give it a try:

After some digging, some progress: Here's some PKEY-type-dependent code that seems overly permissive (silently accepting unknown PKEYs and thus not playing nice with provider-implemented PKEYs):

openssl/crypto/cms/cms_sd.c

Lines 241 to 242 in 7850cc8

if (pkey->ameth == NULL || pkey->ameth->pkey_ctrl == NULL)
return 1;

So replacing the rather blunt return 1 with a more generic PKEY-type->OID mapping function (return ossl_cms_generic_sign(si, cmd);), seems like a "lightweight" fix:

static int ossl_cms_generic_sign(CMS_SignerInfo *si, int verify)
{
    if (!verify) {
        int hnid;
        X509_ALGOR *alg1, *alg2;
        EVP_PKEY *pkey = si->pkey;

        CMS_SignerInfo_get0_algs(si, NULL, NULL, &alg1, &alg2);
        /* Digest presence check really necessary? */
        if (alg1 == NULL || alg1->algorithm == NULL)
            return -1;
        hnid = OBJ_obj2nid(alg1->algorithm);
        if (hnid == NID_undef)
            return -1;

        /* Generic PKEY->name->NID->OID mapping check */
        const char* typename = EVP_PKEY_get0_type_name(pkey);
        if (!typename)
            return -1;
        return X509_ALGOR_set0(alg2, OBJ_nid2obj(OBJ_sn2nid(typename)), V_ASN1_UNDEF, NULL);
    }
    return 1;
}

After adding this, all openssl tests still pass and issue 2 above goes away.

--> Only question right now: Do you think I'm on the right track here? If so, I'll go about working through more CMS functions.

@t8m
Copy link
Member

t8m commented Feb 18, 2022

Given we have the provider upcalls for adding objects this looks like a way to go. So yeah.

@baentsch
Copy link
Contributor Author

Full support of additional algorithms with CMS is definitely not there and would be a kind-of major feature. It would be something that we would surely like to have but is currently not a priority for the next release.

@t8m, can I ask whether this is now a feature considered to be "officially supported"? If so, as of which OpenSSL version? (CMS-)oqsprovider testing with OpenSSL3.0 fails-- but should it be expected to pass with 3.1? With 3.2? It does pass with master. Given the relatively small size of #17733 would it be an option to back-port that PR to 3.0 to also enable CMS operations there? Thanks in advance for your advice (which OpenSSL version to target in our testing for this feature).

@levitte
Copy link
Member

levitte commented Oct 16, 2022

I see the problem in an opposite direction.

  1. It should be possible to fetch algorithms directly by name or OID (in canonical text form). This is supported at a fetching level, but CMS isn't well adapted for this yet.
  2. We need replacements for some of the old CMS specific ameth controls. This really boils down to passing AlgorithmIdentifier parameters to the provider, something we currently only support for Ciphers (which might be sufficient, I'm not sure).

@baentsch
Copy link
Contributor Author

I see the problem in an opposite direction.

What does this mean? That CMS isn't (supposed/supported to be) working for provider-based algorithms in any 3.x branch?

CMS isn't well adapted for this yet. [...] We need replacements for some of the old CMS specific ameth controls

Is there something specific you could point to and that I may be able to help with?

It'd really be useful to be able to create quantum-safe signed documents using OpenSSL now that at least one algorithm is standardized. Further, as some people believe seriously useful quantum computers will be around in our lifetime (depending on one's age, I presume :) having easy & well-known tooling to create such signatures may be getting more and more interesting.

@levitte
Copy link
Member

levitte commented Oct 17, 2022

[sigh] sorry, I spoke before seeing that most of this issue is old and that things have updated since. Never mind.

I don't see reason not to include #17733 at least in upcoming 3.1.

@baentsch
Copy link
Contributor Author

I don't see reason not to include #17733 at least in upcoming 3.1.

Thanks -- that'd be great. Any chance for a 3.0 backport? Would be glad to do a PR for that branch.

@t8m
Copy link
Member

t8m commented Oct 17, 2022

Any chance for a 3.0 backport?

IMO this does NOT belong to a stable release. This is definitely not a bug fix.

@levitte
Copy link
Member

levitte commented Oct 17, 2022

Any chance for a 3.0 backport?

IMO this does NOT belong to a stable release. This is definitely not a bug fix.

What's the current alternative for having external providers like OQS work with openssl cms?

Note: I'm not trying to push for 3.0... I'm actually ambivalent in that regard. I just want to know the reasoning for your statement

@baentsch
Copy link
Contributor Author

What's the current alternative for having external providers like OQS work with openssl cms?

We currently tell users of oqsprovider that want to create OQS CMS structures to not use their OpenSSL3.0 installation but build OpenSSL from source (branch master).

This is definitely not a bug fix.

I fully understand this statement from an "insiders perspective" knowing about feature sets (and their priorities).

Allow me to add an "outside perspective", too:

That these two features do not work together in 3.0 is not documented anywhere for all I know. Please correct me if I'm wrong with this. Without such documentation, this could be considered a bug therefore. The small size of the logic change in #17733 also speaks for "bug fix" rather than "feature change".

Again, do not get me wrong: I absolutely accept your decision as to what's feature and what's bug and I'm grateful for any and all feedback allowing me to add functionality. I'm primarily asking for clarity so we can tell users what to do. Personally I also have the (admittedly sometimes silly) goal to make life as easy as possible for users of our software (i.e., avoid having to build OpenSSL from master if not absolutely necessary). Apologies for the long response.

@t8m
Copy link
Member

t8m commented Oct 18, 2022

The ability to provide arbitrary algorithm in CMS through the provider support is a feature not a bug. At least to me this is clear. So an exception would have to be given.

@mingw-io
Copy link

Ciao.
We are new to the oqs-provider project and started experimenting with it using OpenSSL master branch.

image

End users should never be forced to build projects (and all their dependencies) from source. That is unacceptable! End users are not developers (but perhaps in academia they are!). We should provide binaries for different platforms or OSes.
We will prepare a simple installer which users can download and use straight away.

We are more than happy to do more testing against other branches (3.x).

@t8m
Copy link
Member

t8m commented Oct 24, 2022

End users should never be forced to build projects (and all their dependencies) from source. That is unacceptable! End users are not developers (but perhaps in academia they are!). We should provide binaries for different platforms or OSes.
We will prepare a simple installer which users can download and use straight away.

Please do not use such strong words as unacceptable, etc. The consumers of the openssl library sources are the application developers and not end users. The openssl library by itself is not supposed to be consumed by end users of software directly. It is either part of an operating system (such as the various Linux distros) or part of an application. Even the openssl command line tool is not supposed to be used by end users except for those that are experts in the field. The command line tool was never written in a way to be friendly to end users without fairly deep knowledge in applied crypto and security topics.

@t8m t8m removed this from the Post 3.1.0 milestone Mar 30, 2023
@baentsch
Copy link
Contributor Author

CMS now ("master") works with providers. Thus closing the issue.

@excitoon
Copy link

@t8m As 3.1 and 3.0 are still supported, can we add the fix d15d561 there?

@t8m
Copy link
Member

t8m commented Apr 4, 2024

@t8m As 3.1 and 3.0 are still supported, can we add the fix d15d561 there?

That change is a feature change so unfortunately it is not eligible for backport to older releases.

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 help wanted triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

No branches or pull requests

5 participants