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

EC keys: i2d_PublicKey and d2i_PublicKey not working as expected #16989

Closed
baentsch opened this issue Nov 8, 2021 · 12 comments
Closed

EC keys: i2d_PublicKey and d2i_PublicKey not working as expected #16989

baentsch opened this issue Nov 8, 2021 · 12 comments
Labels
branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug

Comments

@baentsch
Copy link
Contributor

baentsch commented Nov 8, 2021

It seems to be intentional that it is not possible to use i2d_PublicKey and d2i_PublicKey to encode and decode an EC EVP_PKEY in immediate succession. Is that understanding correct? Understandable as the EC parameters don't go into the encoded data, right?

Reading #14258 confirmed this; thus I followed the advice given by @t8m using the code below, but to no avail: d2i_PublicKey keeps failing.

Is that a bug or should the code below read differently to work?

This happens with OpenSSL3 built on master ("OpenSSL 3.1.0-dev (Library: OpenSSL 3.1.0-dev )") and "gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04) "

#include "openssl/evp.h"
#include "openssl/ec.h"
#include "openssl/core_names.h"
#include <string.h>

int main(int argc, char* argv[]) {
   unsigned char* pubkey_enc = OPENSSL_malloc(1000); // surely enough
   const unsigned char* pk_enc = pubkey_enc;         // for use with d2i_PublicKey
   EVP_PKEY* eck = EVP_EC_gen("P-256");              // generate key
   int pklen = i2d_PublicKey(eck, &pubkey_enc);      // encode key

   EVP_PKEY* neck = EVP_PKEY_new();                  // shall hold EC key copy
   OSSL_PARAM params[2];                             // as per https://github.com/openssl/openssl/issues/14258#issuecomment-783351031

   params[0] = OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, "P-256", 0);
   params[1] = OSSL_PARAM_construct_end();
   EVP_PKEY_CTX * pctx = EVP_PKEY_CTX_new_from_name(NULL, "EC", NULL);
   if (pctx && EVP_PKEY_fromdata_init(pctx) &&
       EVP_PKEY_fromdata(pctx, &neck, OSSL_KEYMGMT_SELECT_DOMAIN_PARAMETERS, params))
      neck = d2i_PublicKey(EVP_PKEY_EC, &neck, &pk_enc, pklen);
   printf("neck = %p\n", neck);
   if (EVP_PKEY_eq(eck, neck)) printf("Success!\n");
   else printf("Failure\n");
}

@baentsch baentsch added the issue: question The issue was opened to ask a question label Nov 8, 2021
@levitte
Copy link
Member

levitte commented Nov 8, 2021

Is this possibly the same things as #16977? If it is, please try to the fix for it and see if that makes things better.

@baentsch
Copy link
Contributor Author

baentsch commented Nov 8, 2021

Is this possibly the same things as #16977? If it is, please try to the fix for it and see if that makes things better.

Nope. Applying the patch in #16983 doesn't change this behaviour. When debugging into the failure, the problem is here, called via

if (!o2i_ECPublicKey(&ret->pkey.ec, pp, length)) {

@t8m t8m added branch: 3.0 Merge to openssl-3.0 branch branch: master Merge to master branch triaged: bug The issue/pr is/fixes a bug and removed issue: question The issue was opened to ask a question labels Nov 8, 2021
@baentsch
Copy link
Contributor Author

baentsch commented Nov 8, 2021

FWIW below a somewhat "hardcore" way around the problem: With this helper function:

EVP_PKEY* setECParams(EVP_PKEY *eck, int nid) {
    const char p256params[] = { 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07 };
    const char p384params[] = { 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x22 };
    const char p521params[] = { 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x23 };

    const unsigned char* params;
    switch(nid) {
        case NID_X9_62_prime256v1:
            params = p256params;
            return d2i_KeyParams(EVP_PKEY_EC, &eck, &params, sizeof(p256params));
        case NID_secp384r1:
            params = p384params;
            return d2i_KeyParams(EVP_PKEY_EC, &eck, &params, sizeof(p384params));
        case NID_secp521r1:
            params = p521params;
            return d2i_KeyParams(EVP_PKEY_EC, &eck, &params, sizeof(p521params));
        default:
            return NULL;
    }
}

one can replace the PARAM code and get the "bug reproducer code" to not fail in d2i_PublicKey(EVP_PKEY_EC, &neck, &pk_enc, pklen) and return a positive EVP_PKEY_eq(eck, neck):

int main(int argc, char* argv[]) {
   unsigned char* pubkey_enc = OPENSSL_malloc(1000); // surely enough
   const unsigned char* pk_enc = pubkey_enc;
   EVP_PKEY* eck = EVP_EC_gen(argv[1]);
   int pklen = i2d_PublicKey(eck, &pubkey_enc);

   int nid = NID_undef;

   if (!strcmp(argv[1], "P-256")) {
      nid = NID_X9_62_prime256v1;
   }
   else if (!strcmp(argv[1], "P-384")) {
      nid = NID_secp384r1;
   }
   else if (!strcmp(argv[1], "P-521")) {
      nid = NID_secp521r1;
   }

   EVP_PKEY* neck = EVP_PKEY_new();
   neck = setECParams(neck, nid);
   neck = d2i_PublicKey(EVP_PKEY_EC, &neck, &pk_enc, pklen);
   printf("neck = %p\n", neck);
   printf("eq: %d\n", EVP_PKEY_eq(eck, neck));
}

Output:

$ ./ecgen P-384
neck = 0x55ef6925ecc0
eq: 1

@mattcaswell
Copy link
Member

mattcaswell commented Nov 11, 2021

In my mind d2i_PublicKey with EC keys is just unusable at the moment. The current o2i implementation just makes no sense.

The documentation describing what d2i_PublicKey is supposed to do is .... brief :-)

In reference to d2i_PrivateKey_ex(), the d2i_PublicKey documentation says:

d2i_PublicKey() does the same for public keys.

And, d2i_PrivateKey_ex() says:

d2i_PrivateKey_ex() decodes a private key using algorithm I<type>. It attempts
to use any key-specific format or PKCS#8 unencrypted PrivateKeyInfo format.

But this description cannot simply be applied to d2i_PublicKey by replacing "private key" with "public key" since PKCS#8 uses PrivateKeyInfo format. In fact d2i_PublicKey only handles key-specific formats. It will never try a "generic" format such as SubjectPublicKeyInfo.

But there is no type specific format for EC public keys.

This strange behaviour of d2i_PublicKey() was also present in 1.1.1. At least there you could sort-of use the current functionality by pre-populating the EC_KEY with the parameters. But that seems a somewhat dubious workaround, and doesn't fit well in 3.0 where all the EC_KEY functions are deprecated.

Further the conversion of d2i_PublicKey() to be provider aware seems to have been missed, as noted in #16896.

So, now its unclear what should be done (and in what branches). Possibilities:

  1. Just document this weird behaviour
  2. Remove support for EC keys from d2i_PublicKey since there is no key specific format to be used (but this is a potential breaking change if people are somehow using this and getting it to work for EC keys)
  3. Use SubjectPublicKeyInfo format for EC keys only (again a possible breaking change)
  4. Fall back to SubjectPublicKeyInfo format in all cases (probably not breaking in most cases...but it could be...and feels more like a feature, so probably not suitable for 3.0)

In addition to all of the above we need to decide whether provider support should be added to master and whether it should be backported to 3.0 (as tracked in issue #16896)

@mattcaswell mattcaswell added the hold: need otc decision The OTC needs to make a decision label Nov 11, 2021
@baentsch
Copy link
Contributor Author

Thanks for the assessment and options, @mattcaswell! FWIW, options 2-4 indeed would break our hybrid (classic/EC+PQ/QSC) implementation(s): FYI see what we do here for the OpenSSL1.1.1 fork and here and here what's in the OpenSSL3 provider.
Then again, breaking may be no big issue as no-one is supposed to rely on this code base for anything -- however, it might be folks have created hybrid EC/QSC certificates that would need to be changed if option 3 or 4 gets implemented. Option 2 is the least desirable option as I presently wouldn't understand then how to re-create in the provider the functionality we had in the OQS-OpenSSL1.1.1 fork in an interoperable manner; using OpenSSL1.1.1 APIs in an OpenSSL3 provider sure doesn't seem right.

@dlegaultbbry
Copy link

From what I've seen and could read of other libraries and online docs is that EC Public Keys are just a dump of a curve point without the curve id (ANSI X9.62/X9.63 from SEC1 format). While you can convert an X.509 SPKI into an EC public key, you can't do the reverse because you've lost some information. You need to be able to tell which curve it came from. My understanding of this thread is that is what i2o_PublicKey does, but the o2i will not work. I assume it's possible with openssl to manually rebuild an EC public key internally if you supply the point + the curve information like other libraries provide. After which you could dump it into a know format like X.509 SPKI.

It would also be great if the ''type-specific" formats would state which format we are talking about in the docs somewhere. I checked and I couldn't find it. In the code it's really hard to track all of this in the macro over macro generic code generation that happens. Maybe there's a trick to figure it out quickly.

@mattcaswell
Copy link
Member

what I've seen and could read of other libraries and online docs is that EC Public Keys are just a dump of a curve point without the curve id (ANSI X9.62/X9.63 from SEC1 format)

If I read that correctly - are you saying that other libraries do the same as us for a type specific EC Public Key, i.e. just dump the raw public key (without the curve data) and wrap it up in a PEM file? AFAIK this isn't standardised anywhere and I was just assuming it was an OpenSSL aberration.

@baentsch
Copy link
Contributor Author

I was just assuming it was an OpenSSL aberration

In turn, I personally wouldn't rule out the possibility that other(library developer)'s aimed for compatibility with OpenSSL -- especially in the absence of an authoritative standard.

@dlegaultbbry
Copy link

what I've seen and could read of other libraries and online docs is that EC Public Keys are just a dump of a curve point without the curve id (ANSI X9.62/X9.63 from SEC1 format)

If I read that correctly - are you saying that other libraries do the same as us for a type specific EC Public Key, i.e. just dump the raw public key (without the curve data) and wrap it up in a PEM file? AFAIK this isn't standardised anywhere and I was just assuming it was an OpenSSL aberration.

I was working on a piece of H/W which stores public keys in DER this way (all keys are from curve p-256) and it uses wolfssl's wc_ecc_import_x963_ex to import the key. This function requires that you specify the curve from which the parameters come from. I was looking for comparable code in openssl to do something similar but I couldn't find it until I stumbled on this discussion.

@mattcaswell
Copy link
Member

I was working on a piece of H/W which stores public keys in DER this way

Ah, ok. Is it actually DER or is it just binary data? What OpenSSL does it output the raw binary representation of the EC Point and then wraps that up in a PEM file. The binary representation of an EC Point is actually standardised (in X9.62). That same standard says that the DER representation of an EC Point has the binary EC point wrapped up in an ASN.1 OCTET STRING - which OpenSSL is not doing in this case. OpenSSL is using the PEM headers "EC PUBLIC KEY" which I also don't think are standardised anywhere.

So, it sounds to me like OpenSSL really is doing its own thing here. I doubt this PEM format is widely supported. AFAIK you can't persuade the OpenSSL command line tools to emit this format, and they won't consume it either.

@paulidale
Copy link
Contributor

OTC: We should either make the round trip back work or document that it doesn't work (perhaps adding an error in this case). Further investigation is required.

@t8m
Copy link
Member

t8m commented Nov 16, 2021

I am looking at this and IMO the testcase in the opening post could be made to work if we downgraded the key in d2i_PublicKey via evp_pkey_copy_downgraded. The problem is how to deal with the old provider based key in the parameter. Should it be simply left out dangling but then the testcase would have to explicitly free it. Or should d2i_PublicKey free it by itself? But that semantics is not documented anywhere.

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 branch: 3.0 Merge to openssl-3.0 branch triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants