Limited usefulness of RSA set key function

Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

I have been working with the AF_ALG patches for akcipher lately and I find the RSA set key function way too limited. Especially the fact that it uses a format that I can not find a single reference / standard for worries me.

RsaKey ::= SEQUENCE {
        n INTEGER ({ rsa_get_n }),
        e INTEGER ({ rsa_get_e }),
        d INTEGER ({ rsa_get_d })
}

So where is this format coming from? I can find the RSA Public Key format which is a sequence of n and e. If you have a DER encoded RSA public key, then you can use it to encrypt and verify. So that is okay.

However if you have a standard Public Key that OpenSSL would create by default, then things do not work since that is actually a more complicated DER encoded format. However in the end, I would expect that we could also load such a key here. The RSA set key function should auto detect it and extract the right information if it is marked as rsaEncryption type.

My biggest concern however is that this does not reassemble the RSA Private Key at all. That key format is a sequence of 9 integers starting with a version. So logically I would expect that I can just set a RSA Private Key and then utilize the encrypt and decrypt features of the RSA cipher.

When it comes to exposing RSA via AF_ALG and akcipher, I really want standard format for the set key operation. Asking userspace to construct this Linux kernel only key format is not helpful. You want to be able to just load the RSA Private Key in DER format and be done with it.

Any ideas on how we can fix this to allow a sensible userspace API?

Regards

Marcel

Re: Limited usefulness of RSA set key function

[Stephan Mueller]

Am Sonntag, 2. August 2015, 21:16:47 schrieb Marcel Holtmann:

Hi Marcel,

>Hi Tadeusz,
>
>I have been working with the AF_ALG patches for akcipher lately and I find
>the RSA set key function way too limited. Especially the fact that it uses a
>format that I can not find a single reference / standard for worries me.
>
>RsaKey ::= SEQUENCE {
>        n INTEGER ({ rsa_get_n }),
>        e INTEGER ({ rsa_get_e }),
>        d INTEGER ({ rsa_get_d })
>}

Note, the kernel uses BER encoding.
>
>So where is this format coming from? I can find the RSA Public Key format
>which is a sequence of n and e. If you have a DER encoded RSA public key,
>then you can use it to encrypt and verify. So that is okay.
>
>However if you have a standard Public Key that OpenSSL would create by
>default, then things do not work since that is actually a more complicated
>DER encoded format. However in the end, I would expect that we could also
>load such a key here. The RSA set key function should auto detect it and
>extract the right information if it is marked as rsaEncryption type.

Do you propose to add a DER parser to the kernel? I feel that the BER parser 
is complex enough. If somebody has a DER key, user space should have the code 
to convert it to BER.
>
>My biggest concern however is that this does not reassemble the RSA Private
>Key at all. That key format is a sequence of 9 integers starting with a
>version. So logically I would expect that I can just set a RSA Private Key
>and then utilize the encrypt and decrypt features of the RSA cipher.

Why do you think this is not possible? testmgr.h seems to exactly do that.
>
>When it comes to exposing RSA via AF_ALG and akcipher, I really want standard
>format for the set key operation. Asking userspace to construct this Linux
>kernel only key format is not helpful. You want to be able to just load the
>RSA Private Key in DER format and be done with it.

I am all for it. BER is a standard, is it not? Yes, I have not yet seen a 
converter for DER to BER (and I have not searched for one either). But I feel 
DER should be left to user space.
>
>Any ideas on how we can fix this to allow a sensible userspace API?

I actually planned to add a BER encoder to my libkcapi. I feel that should be 
right place to provide that code, including a potential DER to BER converter.

Ciao
Stephan

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Stephan,

>> I have been working with the AF_ALG patches for akcipher lately and I find
>> the RSA set key function way too limited. Especially the fact that it uses a
>> format that I can not find a single reference / standard for worries me.
>> 
>> RsaKey ::= SEQUENCE {
>>       n INTEGER ({ rsa_get_n }),
>>       e INTEGER ({ rsa_get_e }),
>>       d INTEGER ({ rsa_get_d })
>> }
> 
> Note, the kernel uses BER encoding.

so what? DER is still valid BER.

>> So where is this format coming from? I can find the RSA Public Key format
>> which is a sequence of n and e. If you have a DER encoded RSA public key,
>> then you can use it to encrypt and verify. So that is okay.
>> 
>> However if you have a standard Public Key that OpenSSL would create by
>> default, then things do not work since that is actually a more complicated
>> DER encoded format. However in the end, I would expect that we could also
>> load such a key here. The RSA set key function should auto detect it and
>> extract the right information if it is marked as rsaEncryption type.
> 
> Do you propose to add a DER parser to the kernel? I feel that the BER parser 
> is complex enough. If somebody has a DER key, user space should have the code 
> to convert it to BER.

Why would you do that? DER is still valid BER.

>> My biggest concern however is that this does not reassemble the RSA Private
>> Key at all. That key format is a sequence of 9 integers starting with a
>> version. So logically I would expect that I can just set a RSA Private Key
>> and then utilize the encrypt and decrypt features of the RSA cipher.
> 
> Why do you think this is not possible? testmgr.h seems to exactly do that.

It does not. The RSA Private Key has a different format.

      RSAPrivateKey ::= SEQUENCE {
          version           Version,
          modulus           INTEGER,  -- n
          publicExponent    INTEGER,  -- e
          privateExponent   INTEGER,  -- d
          prime1            INTEGER,  -- p
          prime2            INTEGER,  -- q
          exponent1         INTEGER,  -- d mod (p-1)
          exponent2         INTEGER,  -- d mod (q-1)
          coefficient       INTEGER,  -- (inverse of q) mod p
      }

And honestly that the RSA Public Key magically matches seems more luck then clear intention.

      RSAPublicKey ::= SEQUENCE {
          modulus           INTEGER,  -- n
          publicExponent    INTEGER   -- e
      }

So what you have in testmgr.h is neither RSA Private Key nor RSA Public Key. It is a brand new format that is sadly Linux specific.

>> When it comes to exposing RSA via AF_ALG and akcipher, I really want standard
>> format for the set key operation. Asking userspace to construct this Linux
>> kernel only key format is not helpful. You want to be able to just load the
>> RSA Private Key in DER format and be done with it.
> 
> I am all for it. BER is a standard, is it not? Yes, I have not yet seen a 
> converter for DER to BER (and I have not searched for one either). But I feel 
> DER should be left to user space.

DER is valid BER. However just saying the key is BER has no meaning whatsoever. It does not define an actual key format.

What I would like to see is support for RSA Private Key (DER encoded) and RSA Public Key (DER encoded) when using the RSA cipher. That makes sense to me. Everything else is just insanity since we force the user to convert well known formats into new ones for no apparent reason.

>> Any ideas on how we can fix this to allow a sensible userspace API?
> 
> I actually planned to add a BER encoder to my libkcapi. I feel that should be 
> right place to provide that code, including a potential DER to BER converter.

No idea what DER to BER would give you since DER is always valid BER. I see no need for BER or DER encoders if the keys and also certificates are already available in DER format your system (or can be easily converted from PEM to DER with standard tools).

Seriously, I want to take the RSA Private Key that comes out of OpenSSL and use it. I want to be able to extract the RSA Public Key out of a certificate and use it. Plain and simple. No other gimmicks, conversions or tricks to play. Using existing key format standards is really the key here.

Regards

Marcel

Re: Limited usefulness of RSA set key function

[Stephan Mueller]

Am Montag, 3. August 2015, 00:14:28 schrieb Marcel Holtmann:

Hi Marcel,

> 
> It does not. The RSA Private Key has a different format.
> 
>       RSAPrivateKey ::= SEQUENCE {
>           version           Version,
>           modulus           INTEGER,  -- n
>           publicExponent    INTEGER,  -- e
>           privateExponent   INTEGER,  -- d
>           prime1            INTEGER,  -- p
>           prime2            INTEGER,  -- q
>           exponent1         INTEGER,  -- d mod (p-1)
>           exponent2         INTEGER,  -- d mod (q-1)
>           coefficient       INTEGER,  -- (inverse of q) mod p
>       }
> 
> And honestly that the RSA Public Key magically matches seems more luck then
> clear intention.
> 
>       RSAPublicKey ::= SEQUENCE {
>           modulus           INTEGER,  -- n
>           publicExponent    INTEGER   -- e
>       }

I think here we may have the issue: the ASN.1 structure the kernel uses should 
be changed to implement that commonly used ASN.1 structure. If this change 
would allow a DER to be used, I think we have the solution.


-- 
Ciao
Stephan

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Stephan,

>> It does not. The RSA Private Key has a different format.
>> 
>>      RSAPrivateKey ::= SEQUENCE {
>>          version           Version,
>>          modulus           INTEGER,  -- n
>>          publicExponent    INTEGER,  -- e
>>          privateExponent   INTEGER,  -- d
>>          prime1            INTEGER,  -- p
>>          prime2            INTEGER,  -- q
>>          exponent1         INTEGER,  -- d mod (p-1)
>>          exponent2         INTEGER,  -- d mod (q-1)
>>          coefficient       INTEGER,  -- (inverse of q) mod p
>>      }
>> 
>> And honestly that the RSA Public Key magically matches seems more luck then
>> clear intention.
>> 
>>      RSAPublicKey ::= SEQUENCE {
>>          modulus           INTEGER,  -- n
>>          publicExponent    INTEGER   -- e
>>      }
> 
> I think here we may have the issue: the ASN.1 structure the kernel uses should 
> be changed to implement that commonly used ASN.1 structure. If this change 
> would allow a DER to be used, I think we have the solution.

as you can clearly see. There are two formats defined here. There is no single ASN.1 structure that can decode both of these.

It is what it is, RSA Public Key and RSA Private Key formats are two different key formats. And OpenSSL also treats it like this. You can extract the public key from a private key (same way you can extract it from a certificate), but you can not create a private key structure that only contains the public key.

For RSA we need to support the two formats as listed above. To make this really easy from an API point of view, I would have setkey and setpubkey function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket options for AF_ALG.

Regards

Marcel

Re: Limited usefulness of RSA set key function

[Tadeusz Struk]

Hi Marcel,
On 08/03/2015 12:30 AM, Marcel Holtmann wrote:
> as you can clearly see. There are two formats defined here. There is no single ASN.1 structure that can decode both of these.
> 
> It is what it is, RSA Public Key and RSA Private Key formats are two different key formats. And OpenSSL also treats it like this. You can extract the public key from a private key (same way you can extract it from a certificate), but you can not create a private key structure that only contains the public key.
> 
> For RSA we need to support the two formats as listed above. To make this really easy from an API point of view, I would have setkey and setpubkey function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket options for AF_ALG.

I'll have a look what will be the easiest way to get the openSSL generated & unmodified private key working.
Thanks,
T

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

>> as you can clearly see. There are two formats defined here. There is no single ASN.1 structure that can decode both of these.
>> 
>> It is what it is, RSA Public Key and RSA Private Key formats are two different key formats. And OpenSSL also treats it like this. You can extract the public key from a private key (same way you can extract it from a certificate), but you can not create a private key structure that only contains the public key.
>> 
>> For RSA we need to support the two formats as listed above. To make this really easy from an API point of view, I would have setkey and setpubkey function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket options for AF_ALG.
> 
> I'll have a look what will be the easiest way to get the openSSL generated & unmodified private key working.

I already have patches for that actually. The question is just which approach to take?

My current proposal is to separate the current crypto_akcipher_setkey into two functions. Use the crypto_akcipher_setkey for loading combined private and public key formats and crypto_akcipher_setpubkey for just loading the public key only format.

One other option is actually for crypto_akcipher_setkey to use struct public_key from include/crypto/public_key.h. I mean we have this all defined. Why do we operate on binary blobs for the keys in the first place. For example if the key comes from a certificate in the keyring, lets just use that data. Most likely the asymmetric key type already decoded it nicely for us. No need to do this again.

I mean there is more than RSA out there and this would allow us to express struct public_key_algorithm in addition to the allowed applications of said key as well. This would properly also allow us to combine the crypto/asymmetric_keys/rsa.c and crypto/rsa.c so that we only have a single RSA implementation. Not to mention that we also have lib/digsig.c in the kernel.

Regards

Marcel

Re: Limited usefulness of RSA set key function

[Tadeusz Struk]

On 08/03/2015 10:39 AM, Marcel Holtmann wrote:
> I already have patches for that actually. The question is just which approach to take?
> 
> My current proposal is to separate the current crypto_akcipher_setkey into two functions. Use the crypto_akcipher_setkey for loading combined private and public key formats and crypto_akcipher_setpubkey for just loading the public key only format.

I would prefer to have one setkey function if possible.

> 
> One other option is actually for crypto_akcipher_setkey to use struct public_key from include/crypto/public_key.h. I mean we have this all defined. Why do we operate on binary blobs for the keys in the first place. For example if the key comes from a certificate in the keyring, lets just use that data. Most likely the asymmetric key type already decoded it nicely for us. No need to do this again.

That was the first proposal to use this struct, but it was rejected. Looks like MPIs are not acceptable on the API.
I think it make sense now as it will only be useful for the SW implementation. For hardware the easiest way is to take
it in this form.

> 
> I mean there is more than RSA out there and this would allow us to express struct public_key_algorithm in addition to the allowed applications of said key as well. This would properly also allow us to combine the crypto/asymmetric_keys/rsa.c and crypto/rsa.c so that we only have a single RSA implementation. Not to mention that we also have lib/digsig.c in the kernel.

The crypto/asymmetric_keys/ will be converted to the new api and crypto/asymmetric_keys/rsa.c removed soon.

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

>> I already have patches for that actually. The question is just which approach to take?
>> 
>> My current proposal is to separate the current crypto_akcipher_setkey into two functions. Use the crypto_akcipher_setkey for loading combined private and public key formats and crypto_akcipher_setpubkey for just loading the public key only format.
> 
> I would prefer to have one setkey function if possible.
> 
>> 
>> One other option is actually for crypto_akcipher_setkey to use struct public_key from include/crypto/public_key.h. I mean we have this all defined. Why do we operate on binary blobs for the keys in the first place. For example if the key comes from a certificate in the keyring, lets just use that data. Most likely the asymmetric key type already decoded it nicely for us. No need to do this again.
> 
> That was the first proposal to use this struct, but it was rejected. Looks like MPIs are not acceptable on the API.
> I think it make sense now as it will only be useful for the SW implementation. For hardware the easiest way is to take
> it in this form.

actually I think this reasoning needs to be revisited. When I look at this, this makes no sense whatsoever. The end result is that we have keys in multiple formats in the kernel and have to convert between them or parse them again.

If you do not want to use struct public_key, then lets go for struct key as I proposed in my other response. That should in the end be able to represent hardware keys as well. There is really no good reason to move things around and parse them again or create new formats out of it.

My take is that we want to store a key once in a single location and single location only. Storing the same key in different formats in different locations is just a plain bad idea. Keep it secure and locked in one place.

>> I mean there is more than RSA out there and this would allow us to express struct public_key_algorithm in addition to the allowed applications of said key as well. This would properly also allow us to combine the crypto/asymmetric_keys/rsa.c and crypto/rsa.c so that we only have a single RSA implementation. Not to mention that we also have lib/digsig.c in the kernel.
> 
> The crypto/asymmetric_keys/ will be converted to the new api and crypto/asymmetric_keys/rsa.c removed soon.

I hope someone is looking at lib/digsig.c as well and gets this all cleaned up into a single implementation. However for this to happen, we really need to get this key stuff figured out. Right now this looks to me like the left hand does not know what the right hand is doing.

Regards

Marcel

Re: Limited usefulness of RSA set key function

[Tadeusz Struk]

On 08/03/2015 11:20 AM, Marcel Holtmann wrote:
> actually I think this reasoning needs to be revisited. When I look at this, this makes no sense whatsoever. The end result is that we have keys in multiple formats in the kernel and have to convert between them or parse them again.
> 
> If you do not want to use struct public_key, then lets go for struct key as I proposed in my other response. That should in the end be able to represent hardware keys as well. There is really no good reason to move things around and parse them again or create new formats out of it.
> 
> My take is that we want to store a key once in a single location and single location only. Storing the same key in different formats in different locations is just a plain bad idea. Keep it secure and locked in one place.
> 

The approach was not to use any of the existing public key stuff.
To be consistent with all the other crypto API tfm needs to "own" the key.
The existing struct key is not controlled by the tfm and the key can be gone without tfm knowing.
We can change this approach, but this is not my call, but Herbert's.
Herbert, any comments?

> I hope someone is looking at lib/digsig.c as well and gets this all cleaned up into a single implementation. However for this to happen, we really need to get this key stuff figured out. Right now this looks to me like the left hand does not know what the right hand is doing.

Yes, your are right, but crypto/public_key/rsa.c and lib/digsig.c have been added long before akcipher.
Now we are trying to get all this fixed, but we need to have stable API before any of it will be converted.
Thanks,
T
 

Re: Limited usefulness of RSA set key function

[Marcel Holtmann]

Hi Tadeusz,

>>> as you can clearly see. There are two formats defined here. There is no single ASN.1 structure that can decode both of these.
>>> 
>>> It is what it is, RSA Public Key and RSA Private Key formats are two different key formats. And OpenSSL also treats it like this. You can extract the public key from a private key (same way you can extract it from a certificate), but you can not create a private key structure that only contains the public key.
>>> 
>>> For RSA we need to support the two formats as listed above. To make this really easy from an API point of view, I would have setkey and setpubkey function. And also expose them as ALG_SET_KEY and ALG_SET_PUBKEY socket options for AF_ALG.
>> 
>> I'll have a look what will be the easiest way to get the openSSL generated & unmodified private key working.
> 
> I already have patches for that actually. The question is just which approach to take?
> 
> My current proposal is to separate the current crypto_akcipher_setkey into two functions. Use the crypto_akcipher_setkey for loading combined private and public key formats and crypto_akcipher_setpubkey for just loading the public key only format.
> 
> One other option is actually for crypto_akcipher_setkey to use struct public_key from include/crypto/public_key.h. I mean we have this all defined. Why do we operate on binary blobs for the keys in the first place. For example if the key comes from a certificate in the keyring, lets just use that data. Most likely the asymmetric key type already decoded it nicely for us. No need to do this again.

actually taking this one step further, why don't we integrate directly with the keys.

int crypto_akcipher_setkey(struct crypto_akcipher *tfm, struct key *key)

Or maybe key_ref_t or key_serial_t depending on what is the preferred method of referencing keys. So for every key loaded via the asymmetric key type, we can a have nice clean way to get the key from various different parts of the kernel and it is also reference counted.

It is also by definition cipher agnostic. So this would work for DSA, ECDH or whatever comes next. The only check needed is that the provided key is of type asymmetric and of subtype public key.

This means there is no need to parse it twice or store an extra copy of it or invent some new reference counting around struct public_key.

And then it would be dead simple to introduce ALG_SET_KEY_ID that takes a key_serial_t for usage with AF_ALG. I have implemented ALG_SET_KEY_ID for skcipher and it works great. However for akcipher this would show even more benefits. Especially when certificates and public keys come from the system keyring or via TPM / UEFI.

Regards

Marcel