Use EVP_PKEY for RSA key generation #49336

bartonjs · 2021-03-08T22:20:29Z

Switch from using RSA_generate_key_ex to EVP_PKEY_keygen for creating RSA keys.
This is the preferred model in OpenSSL 3, as it allows for an easier substitute provider
model than the low level primitive.

Further changes in this area will get the managed code / shim interaction to a point
where everything is in terms of EVP_PKEY* values instead of RSA* values.
Among other things, it will allow us to eliminate some of our code from the pipeline,
because the EVP_PKEY interfaces have better support for OAEP and PSS than the primitive.

Contributes to #46526.

ghost · 2021-03-08T22:20:34Z

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Switch from using RSA_generate_key_ex to EVP_PKEY_keygen for creating RSA keys.
This is the preferred model in OpenSSL 3, as it allows for an easier substitute provider
model than the low level primitive.

Further changes in this area will get the managed code / shim interaction to a point
where everything is in terms of EVP_PKEY* values instead of RSA* values.
Among other things, it will allow us to eliminate some of our code from the pipeline,
because the EVP_PKEY interfaces have better support for OAEP and PSS than the primitive.

Contributes to #46526.

Author:	bartonjs
Assignees:	bartonjs
Labels:	`area-System.Security`, `enhancement`
Milestone:	6.0.0

vcsjones · 2021-03-10T02:36:51Z

@bartonjs not sure based on your comment in #48256

Hm, still passing on my machine and failing in the lab.

I can repro the segfault in a Xenial, let me know if I can give you any more info or help.

bartonjs · 2021-03-16T19:10:21Z

let me know if I can give you any more info or help.

I appear to have at least a short-term problem in getting this to the top of my attention heap. If you want to point out where the segfault is that'd be helpful. If you want to take this over and drive it to success, that'd be super-duper helpful 😄.

vcsjones · 2021-03-16T20:15:22Z

I'll take a stab at fixing it (can PR in to your fork) but just in case your psychic debugging powers awaken:

  * frame #0: 0x00007fc587641ab0 libcrypto.so.1.0.0`EVP_PKEY_base_id
    frame #1: 0x00007fc587bd5208 libSystem.Security.Cryptography.Native.OpenSsl.so`local_RSA_pkey_ctx_ctrl(ctx=0x00007fc4c00017b0, optype=4, cmd=4099, p1=2048, p2=0x0000000000000000) at apibridge.c:798:13
    frame #2: 0x00007fc587bdae6d libSystem.Security.Cryptography.Native.OpenSsl.so`CryptoNative_RsaGenerateKey(keySize=2048) at pal_evp_pkey_rsa.c:18:32

Dumping ctx shows that the underlying PKEY in the context is NULL, so pkey is NULL here:

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

Lines 796 to 798 in 74e3e4b

    
           EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx); 
        
           if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption)

If I put an assert(pkey != NULL); there it gets hit.

vcsjones · 2021-03-16T20:45:34Z

@bartonjs

It looks like that if check is done only to double-check that we are indeed operating on an RSA key. But this is called before we actually have a PKEY, we've only done init at this point, so there is no private key. The actual ctrl gets set via the EVP_PKEY_METHOD.

If I just delete this whole if statement, then I get 100% tests passing (In S.S.C.Algorithms):

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

Lines 794 to 802 in 74e3e4b

    
           if (ctx != NULL) 
        
           { 
        
               EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx); 
        
               if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption) 
        
               { 
        
                   return -1; 
        
               } 
        
           }

The correct way to check the key type with EVP_PKEY_CTX_ctrl is to pass it in as the second parameter instead of as -1 to check the pkey_id on the EVP_PKEY_METHOD and that gets checked in OpenSSL here:

https://github.com/openssl/openssl/blob/e818b74be2170fbe957a07b0da4401c2b694b3b8/crypto/evp/pmeth_lib.c#L367

vcsjones

Suggested fix for OpenSSL 1.0.2.

vcsjones · 2021-03-16T20:47:17Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/apibridge.c

+    if (ctx != NULL)
+    {
+        EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx);
+
+        if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption)
+        {
+            return -1;
+        }
+    }
+
+    return EVP_PKEY_CTX_ctrl(ctx, -1, optype, cmd, p1, p2);


Suggested change

if (ctx != NULL)

{

EVP_PKEY* pkey = EVP_PKEY_CTX_get0_pkey(ctx);

if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption)

{

return -1;

}

}

return EVP_PKEY_CTX_ctrl(ctx, -1, optype, cmd, p1, p2);

return EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_RSA, optype, cmd, p1, p2);

bartonjs · 2021-03-16T20:47:34Z

I was trying to write https://github.com/openssl/openssl/blob/081a7061f3da07318c4b0f5de67b82285630bf6b/crypto/rsa/rsa_lib.c#L485-L493.

Looks like I missed that ctx->pkey can be NULL.

vcsjones · 2021-03-16T20:50:45Z

@bartonjs ah. Yeah pkey will always be NULL until you do the actual generate. That was looking at the pmeth but I don't see a good way to get it out of the opaque structures.

bartonjs · 2021-03-16T20:51:28Z

I assume if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption) => if (pkey != NULL && EVP_PKEY_base_id(pkey) != NID_rsaEncryption)

vcsjones · 2021-03-16T20:53:48Z

I assume if (EVP_PKEY_base_id(pkey) != NID_rsaEncryption) => if (pkey != NULL && EVP_PKEY_base_id(pkey) != NID_rsaEncryption)

I'm not sure that really does anything then. It will always be NULL the way it is being used. I think my proposed change should work since when we create the CTX we always use EVP_PKEY_RSA.

runtime/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c

Line 8 in 74e3e4b

EVP_PKEY_CTX* ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, NULL);

bartonjs · 2021-03-16T21:09:33Z

It will always be NULL the way it is being used.

...

Hmm, not sure if this works for PSS / OAEP.

Yeah, it's really these two together that are why I made the local_ as complex as the real thing.

The local_ only checks for EVP_PKEY_RSA because it uses base_id, which turns EVP_PKEY_RSA_PSS into EVP_PKEY_RSA. I think the purpose of the sidecar routine here was "ugh, we have two different IDs for "RSA" and we keep getting the "or" wrong, so let's put them in one place". At least... that's the only reason I can figure out that they added it.

Yeah, in the generate flow the pkey is always NULL (which makes me wonder why they routed EVP_PKEY_CTX_set_rsa_keygen_bits through it at all).

1.0.2: https://github.com/openssl/openssl/blob/12ad22dd16ffe47f8cde3cddb84a160e8cdb3e30/crypto/rsa/rsa.h#L256-L258

# define EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, bits) \
        EVP_PKEY_CTX_ctrl(ctx, EVP_PKEY_RSA, EVP_PKEY_OP_KEYGEN, \
                                EVP_PKEY_CTRL_RSA_KEYGEN_BITS, bits, NULL)

1.1.1: https://github.com/openssl/openssl/blob/081a7061f3da07318c4b0f5de67b82285630bf6b/include/openssl/rsa.h#L121-L123

# define EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, bits) \
        RSA_pkey_ctx_ctrl(ctx, EVP_PKEY_OP_KEYGEN, \
                          EVP_PKEY_CTRL_RSA_KEYGEN_BITS, bits, NULL)

Looks like in 3.0 it's a function in its own right, but I believe the EVP_PKEY_CTX_ctrl route (with or without going through RSA_pkey_ctx_ctrl) will still work.

vcsjones · 2021-03-16T22:54:01Z

Yeah I guess the question then is what is the right thing for the shim to be doing.

turns EVP_PKEY_RSA_PSS into EVP_PKEY_RSA

There is no EVP_PKEY_RSA_PSS in OpenSSL 1.0, so I still think just using EVP_PKEY_RSA for the EVP_PKEY_CTX_ctrl is workable. Even OpenSSL's 1.0 macros for setting PSS parameters use EVP_PKEY_RSA.

https://github.com/openssl/openssl/blob/e818b74be2170fbe957a07b0da4401c2b694b3b8/crypto/rsa/rsa.h#L250-L251

bartonjs · 2021-03-17T15:07:47Z

There is no EVP_PKEY_RSA_PSS in OpenSSL 1.0,

Ah. OK, so if we're running in the local_ then we don't need the early failure.

Giving that a run, thanks for the help thus far 😄.

vcsjones · 2021-03-17T21:35:37Z

Looks promising.

GrabYourPitchforks · 2021-03-18T21:58:20Z

src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_evp_pkey_rsa.c

+    }
+
+    EVP_PKEY* pkey = NULL;
+    int success = 1;


This code pattern feels weird to me. Seems like it's too tempting to refactor this to success &= ... and break the short-circuiting logic.
Isn't if (call1() == 1 && call2() == 1 && call3() == 1) a more established pattern?

Yeah, I think I was sort of playing with the syntax a bit looking for "legible and compact", but didn't do it anywhere else in the EVP_PKEY experimentation.

I'll flip it around to look more uniform.

vcsjones · 2021-03-18T23:26:04Z

I think you can get rid of the REQUIRED_FUNCTIONs that aren't used anymore.

But maybe you'll end up bringing them back in a later PR.

bartonjs · 2021-03-18T23:42:55Z

But maybe you'll end up bringing them back in a later PR.

Yeah, probably.

Once all the work for converting to EVP_PKEY is done, and whatever else is needed to be happily working with OpenSSL 3, it's probably time to see how many function binds are no longer necessary.

bartonjs added 2 commits March 5, 2021 17:05

Use EVP_PKEY for RSA key generation (managed side)

4ec694f

Use EVP_PKEY for RSA key generation (native side)

f6edcb9

bartonjs added enhancement Product code improvement that does NOT require public API changes/additions area-System.Security labels Mar 8, 2021

bartonjs added this to the 6.0.0 milestone Mar 8, 2021

bartonjs self-assigned this Mar 8, 2021

Add missing RSA_pkey_ctx_ctrl prototype for OpenSSL 1.0 compilation

74e3e4b

vcsjones reviewed Mar 16, 2021

View reviewed changes

Simplify local_RSA_pkey_ctx_ctrl

b600114

GrabYourPitchforks reviewed Mar 18, 2021

View reviewed changes

GrabYourPitchforks approved these changes Mar 18, 2021

View reviewed changes

Use a more common short-circuit pattern in generating keys.

0f998d9

runfoapp bot mentioned this pull request Mar 19, 2021

Failing test disableaiaoptionworks on OSX #47492

Closed

bartonjs merged commit 6841143 into dotnet:main Mar 19, 2021

bartonjs deleted the rsa_evp_pkey_generation branch March 19, 2021 22:05

ghost locked as resolved and limited conversation to collaborators Apr 18, 2021

bartonjs removed their assignment Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use EVP_PKEY for RSA key generation #49336

Use EVP_PKEY for RSA key generation #49336

bartonjs commented Mar 8, 2021

ghost commented Mar 8, 2021

vcsjones commented Mar 10, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

vcsjones commented Mar 16, 2021

vcsjones left a comment

vcsjones Mar 16, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 17, 2021

vcsjones commented Mar 17, 2021

GrabYourPitchforks Mar 18, 2021

bartonjs Mar 18, 2021

vcsjones commented Mar 18, 2021

bartonjs commented Mar 18, 2021

Use EVP_PKEY for RSA key generation #49336

Use EVP_PKEY for RSA key generation #49336

Conversation

bartonjs commented Mar 8, 2021

ghost commented Mar 8, 2021

vcsjones commented Mar 10, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

vcsjones commented Mar 16, 2021

vcsjones left a comment

Choose a reason for hiding this comment

vcsjones Mar 16, 2021

Choose a reason for hiding this comment

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 16, 2021

vcsjones commented Mar 16, 2021

bartonjs commented Mar 17, 2021

vcsjones commented Mar 17, 2021

GrabYourPitchforks Mar 18, 2021

Choose a reason for hiding this comment

bartonjs Mar 18, 2021

Choose a reason for hiding this comment

vcsjones commented Mar 18, 2021

bartonjs commented Mar 18, 2021