[WIP] X509: Refactor X509_PUBKEY processing to include provider side keys by levitte · Pull Request #13994 · openssl/openssl

levitte · 2021-01-28T08:29:08Z

When a SubjectPublicKeyInfo (SPKI) is decoded into an X509_PUBKEY
structure, the corresponding EVP_PKEY is automatically added as well.
This used to only support our built-in keytypes, and only in legacy
form.

This is now refactored to try to decode the whole SPKI into an
EVP_PKEY using OSSL_DECODER, and cache that in the X509_PUBKEY very
early. Only when that fails, the existing decoder callback will try
to create a legacy EVP_PKEY.

For purposes where it's not desirable to do the provider side SPKI
decoding, there is now a function d2i_PUBKEY_legacy(). This is used
by all the type specific d2i__PUBKEY(), as well as in out
provider DER->key decoder, to avoid running into recursion problems.

Fixes #13893

This includes refactoring the DER->key decoder to be a bit more flexible with how it uses keytype specific d2i functions and adds the possibility to use d2i_TYPE_PUBKEY.

This also includes the addition of SM2 encoders and decoders...

ecossl_dh_keyexch_functions -> ossl_ecdh_keyexch_functions ecossl_dsa_signature_functions -> ossl_ecdsa_signature_functions sm2_asym_cipher_functions -> ossl_sm2_asym_cipher_functions sm2_keymgmt_functions -> ossl_sm2_keymgmt_functions sm2_signature_functions -> ossl_sm2_signature_functions

The EC KEYMGMT implementation handled SM2 as well, except what's needed to support decoding: loading functions for both EC and SM2 that checks for the presence or absence of the SM2 curve the same way as the EC / SM2 import functions.

The decoding of DER into keys with keytype specific routines depended entirely on the absence of the generic algo specific C type from EVP_PKEYs. That is not necessary, and may even prove to be a bit of a disadvantage, depending on what libcrypto has to offer in terms of type specific d2i functionality for different kinds of input structures. To remedy, we try with all available type specific functions first, and only turn to the general d2i functions (those that return an EVP_PKEY) as a last resort.

… decoders This makes it possible to use d2i_<TYPE>_PUBKEY instead of the generic d2i_PUBKEY()

When a SubjectPublicKeyInfo (SPKI) is decoded into an X509_PUBKEY structure, the corresponding EVP_PKEY is automatically added as well. This used to only support our built-in keytypes, and only in legacy form. This is now refactored to try to decode the whole SPKI into an EVP_PKEY using OSSL_DECODER, and cache that in the X509_PUBKEY very early. Only when that fails, the existing decoder callback will try to create a legacy EVP_PKEY. For purposes where it's not desirable to do the provider side SPKI decoding, there is now a function d2i_PUBKEY_legacy(). This is used by all the type specific d2i_<TYPE>_PUBKEY(), as well as in out provider DER->key decoder, to avoid running into recursion problems. Fixes openssl#13893

levitte · 2021-01-28T08:50:50Z

Something to be noted is the inherent problem with the early caching of an EVP_PKEY in the X509_PUBKEY, when someone else wants an easy way to decode the DER, as part of a decoder implementation for a keytype we know nothing about. It becomes a recursion problem.

The solution I could come up with was to have a d2i_PUBKEY_legacy(), which is currently internal. However, that doesn't help anyone making an external provider for a keytype that's unknown to us, and that merely wants to decode the SubjectPublicKeyInfo into its parts, but no further processing (i.e. no automatic caching of an EVP_PKEY).

levitte · 2021-01-28T08:58:19Z

A slightly different solution could have been to let x509_public_decode() handle the decoding via OSSL_DECODER. The only issue there is that keytypes like RSA-PSS would be a bit problematic, as its parameters aren't included in the pubkey BIT STRING itself, but rather in the AlgorithmID (X509_ALGOR) that sits beside it.

This could be resolved by decoding the X509_ALGOR parameters field and the X509_PUBKEY pubkey fields separately, which is possible with the help of d2i_TYPEPublicKey() and d2i_TYPEparams(), assign what we get to EVP_PKEYs, and use EVP_PKEY_copy_parameters() to copy the parameters extracted from the X509_ALGOR onto the EVP_PKEY that contains the key itself... the only issue there being we don't have d2i_TYPEPublicKey() and d2i_TYPEparams() for all our legacy keytypes.

mattcaswell · 2021-01-28T10:46:47Z

Hmm, this solution doesn't seem to work for me. If I run this code:

        x509 = PEM_read_bio_X509(bio, NULL, NULL, NULL);
        assert(x509 != NULL);
        pkey = X509_get_pubkey(x509);

        assert(pkey != NULL);
        assert(pkey->keymgmt != NULL);

I get:

readx509-2: readx509-2.c:24: main: Assertion `pkey->keymgmt != NULL' failed.
Aborted (core dumped)

levitte · 2021-01-28T11:52:02Z

Yeah, something is off. See the CI failures

levitte · 2021-01-28T12:29:33Z

What key type was there in that certificate? I just found a problem where RSA and RSA-PSS gets mixed up...

mattcaswell · 2021-01-28T13:47:19Z

RSA

The OSSL_FUNC_KEYMGMT_LOAD function for both plain RSA and RSA-PSS keys now also check that the key to be loaded is the correct type, and refuse to load it if it's not.

Furthermore, there are cases where the decoder might not get the key type it expected. This may happen when certain key types that share the same OpenSSL structure may be mixed up somehow. The known cases are EC vs SM2 and RSA vs RSA-PSS. To remedy, we add the possibility to specify a checking function that can check if the key that was decoded meets decoder expectations.

levitte · 2021-01-29T04:02:33Z

A number of fixups added. @mattcaswell, would you mind trying again?

crypto/x509/x_pubkey.c

mattcaswell · 2021-01-29T08:47:22Z

A number of fixups added. @mattcaswell, would you mind trying again?

Same problem.

… keys

levitte · 2021-01-29T10:39:05Z

But hold on, @mattcaswell... is there point that the EVP_PKEY must be provider backed? I thought the primary problem was that it now may be provider backed. In other words, provider backed EVP_PKEYs were completely inaccessible when decoding an X509_PUBKEY before this change, i.e. other algos than our built in ones were impossible.

t8m · 2021-01-29T11:37:03Z

But hold on, @mattcaswell... is there point that the EVP_PKEY must be provider backed? I thought the primary problem was that it now may be provider backed. In other words, provider backed EVP_PKEYs were completely inaccessible when decoding an X509_PUBKEY before this change, i.e. other algos than our built in ones were impossible.

If it is not provider backed, will it be automatically imported to provider if you do for example signature verification of a certificate with the X509_PUBKEY? IMO if that's the case, I think we can keep that.

mattcaswell · 2021-01-29T11:39:50Z

It remains unclear to me why we would do things differently for key types that happen to have a legacy implementation vs for those that don't. If we've got the codepaths to handle provider based keys - why wouldn't we always do that if there is a provider available to do it?

levitte · 2021-01-29T14:12:25Z

If it is not provider backed, will it be automatically imported to provider if you do for example signature verification of a certificate with the X509_PUBKEY? IMO if that's the case, I think we can keep that.

Yes, this was implemented early. The keys are exported to the provider that is to do the signature work if that provider doesn't match the key's provider (or that the key is legacy). This is among the earliest things we implemented.

levitte · 2021-01-29T14:16:26Z

It remains unclear to me why we would do things differently for key types that happen to have a legacy implementation vs for those that don't. If we've got the codepaths to handle provider based keys - why wouldn't we always do that if there is a provider available to do it?

Generally speaking, priorities (we've all enough on our plates as it is). Do operations work with legacy EVP_PKEYs as well as with provider native EVP_PKEYs? In that case, it shouldn't matter whether X509_get_pubkey() one or the other.
The main problem here was that if the algorithm given by the AlgorithmID isn't known by OpenSSL, then X509_get_pubkey() would obviously return NULL... was it not?

mattcaswell · 2021-01-29T14:21:35Z

Generally speaking, priorities (we've all enough on our plates as it is). Do operations work with legacy EVP_PKEYs as well as with provider native EVP_PKEYs? In that case, it shouldn't matter whether X509_get_pubkey() one or the other.
The main problem here was that if the algorithm given by the AlgorithmID isn't known by OpenSSL, then X509_get_pubkey() would obviously return NULL... was it not?

That remains the primary issue - but I don't understand why it is any harder to not just do the same in all cases. Surely this is easier? Why does it behave differently in these cases?

levitte · 2021-01-29T14:25:47Z

That remains the primary issue - but I don't understand why it is any harder to not just do the same in all cases. Surely this is easier? Why does it behave differently in these cases?

I'm frankly not sure what you mean with "different"

levitte · 2021-01-29T14:49:50Z

I've tested the code that @mattcaswell used, and it turns out that the solution here isn't right. I'll have to revisit this

levitte · 2021-01-30T18:25:37Z

I'll have to revisit this

I knew there had to be a way:

openssl/include/openssl/asn1t.h.in

Lines 63 to 66 in a2a5506

    
            * The EXTERN type uses a new style d2i/i2d. 
        
            * The new style should be used where possible 
        
            * because it avoids things like the d2i IMPLICIT 
        
            * hack.

openssl/include/openssl/asn1t.h.in

Lines 609 to 610 in a2a5506

    
           const void *funcs;          /* further data and type-specific functions */ 
        
           /* funcs can be ASN1_PRIMITIVE_FUNCS*, ASN1_EXTERN_FUNCS*, or ASN1_AUX* */

openssl/include/openssl/asn1t.h.in

Lines 652 to 660 in a2a5506

    
           typedef struct ASN1_EXTERN_FUNCS_st { 
        
               void *app_data; 
        
               ASN1_ex_new_func *asn1_ex_new; 
        
               ASN1_ex_free_func *asn1_ex_free; 
        
               ASN1_ex_free_func *asn1_ex_clear; 
        
               ASN1_ex_d2i *asn1_ex_d2i; 
        
               ASN1_ex_i2d *asn1_ex_i2d; 
        
               ASN1_ex_print_func *asn1_ex_print; 
        
           } ASN1_EXTERN_FUNCS;

Using that form allows a programmatic "take-over" of the processing of an item. X509_NAME is processed this way, and it seems like the solution for our problem here is to do the same for X509_PUBKEY...

slontis · 2021-02-01T23:29:43Z

providers/implementations/encode_decode/decode_der2key.c

-            && (selection & OSSL_KEYMGMT_SELECT_PUBLIC_KEY) != 0) {
+        goto next;
+
+    /* We try the typs specific functions first, if available */


slontis · 2021-02-01T23:34:50Z

crypto/x509/x_pubkey.c

+    if ((dctx = OSSL_DECODER_CTX_new_by_EVP_PKEY(&pkey,
+                                                 "DER", "SubjectPublicKeyInfo",
+                                                 NULL, EVP_PKEY_PUBLIC_KEY,
+                                                 NULL, NULL)) != NULL


Why is there no libctx, propq here?

slontis · 2021-02-01T23:42:57Z

providers/implementations/encode_decode/decode_der2key.c


+static int ec_check(void *key, struct der2key_ctx_st *ctx)
+{
+    /* We're trying to be clever by comparing two truths */


This comment seems to be a riddle..

Not much, read the code...

levitte · 2021-02-04T18:34:44Z

I've made this WIP because of the needed rework

levitte · 2021-02-22T11:10:55Z

I'm closing in favor of a new PR where I've started over

levitte · 2021-02-22T22:35:07Z

See #13281

slontis · 2021-02-22T23:10:44Z

See #13281

You mean #14281

levitte · 2021-02-22T23:46:44Z

Ah, oops, yes

levitte added 5 commits January 28, 2021 08:07

PROV: Add type specific SubjectPublicKeyInfo decoding to the DER->key…

49195be

… decoders This makes it possible to use d2i_<TYPE>_PUBKEY instead of the generic d2i_PUBKEY()

levitte added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels Jan 28, 2021

levitte added this to the 3.0.0 beta1 milestone Jan 28, 2021

levitte mentioned this pull request Jan 28, 2021

Public keys decoded from an X509 certificate are legacy keys #13893

Closed

levitte mentioned this pull request Jan 28, 2021

removal of internal OSSL dependencies open-quantum-safe/openssl#267

Merged

levitte added 2 commits January 29, 2021 04:58

PROV: Add RSA-PSS specific OSSL_FUNC_KEYMGMT_LOAD function

52da5b7

The OSSL_FUNC_KEYMGMT_LOAD function for both plain RSA and RSA-PSS keys now also check that the key to be loaded is the correct type, and refuse to load it if it's not.

fixup! PROV: Refactor DER->key decoder

fb22aa1

slontis reviewed Jan 29, 2021

View reviewed changes

crypto/x509/x_pubkey.c Outdated Show resolved Hide resolved

fixup! X509: Refactor X509_PUBKEY processing to include provider side…

9c52467

… keys

slontis reviewed Feb 1, 2021

View reviewed changes

paulnelsontx assigned levitte Feb 4, 2021

levitte changed the title ~~X509: Refactor X509_PUBKEY processing to include provider side keys~~ [WIP] X509: Refactor X509_PUBKEY processing to include provider side keys Feb 4, 2021

levitte removed the approval: review pending This pull request needs review by a committer label Feb 4, 2021

paulnelsontx self-assigned this Feb 4, 2021

paulnelsontx modified the milestones: 3.0.0 beta1, Sprint-Hydrogen, Sprint Hydrogen Feb 8, 2021

levitte closed this Feb 22, 2021

This was referenced Feb 24, 2021

EVP_PKCS82PKEY_ex() always creates legacy keys #14302

Closed

Provider side encoders and decoders need to stop using EVP_PKEY #14306

Closed

Stop using EVP_PKEY in encoders and decoders #14314

Closed

Uh oh!

Conversation

levitte commented Jan 28, 2021

Uh oh!

levitte commented Jan 28, 2021

Uh oh!

levitte commented Jan 28, 2021

Uh oh!

mattcaswell commented Jan 28, 2021

Uh oh!

levitte commented Jan 28, 2021

Uh oh!

levitte commented Jan 28, 2021

Uh oh!

mattcaswell commented Jan 28, 2021

Uh oh!

levitte commented Jan 29, 2021

Uh oh!

Uh oh!

mattcaswell commented Jan 29, 2021

Uh oh!

levitte commented Jan 29, 2021

Uh oh!

t8m commented Jan 29, 2021

Uh oh!

mattcaswell commented Jan 29, 2021

Uh oh!

levitte commented Jan 29, 2021

Uh oh!

levitte commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Jan 29, 2021

Uh oh!

levitte commented Jan 29, 2021

Uh oh!

levitte commented Jan 29, 2021

Uh oh!

levitte commented Jan 30, 2021

Uh oh!

slontis Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

slontis Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

slontis Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

levitte Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

levitte commented Feb 4, 2021

Uh oh!

levitte commented Feb 22, 2021

Uh oh!

levitte commented Feb 22, 2021

Uh oh!

slontis commented Feb 22, 2021

Uh oh!

levitte commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

levitte commented Jan 29, 2021 •

edited

Loading