Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308348 > unrolled thread

Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type David Woodhouse <dwmw2@infradead.org> - 2016-01-13 13:30 +0100
    Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type David Howells <dhowells@redhat.com> - 2016-01-13 14:40 +0100
      Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type David Woodhouse <dwmw2@infradead.org> - 2016-01-13 14:50 +0100
    Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type Tadeusz Struk <tadeusz.struk@intel.com> - 2016-01-13 15:00 +0100

#1308348 — Re: [PATCH v2] crypto: AF_ALG - add support for keys/asymmetric-type

[Multipart message — attachments visible in raw view] — view raw

On Sat, 2015-12-26 at 07:50 -0800, Tadeusz Struk wrote:
> +static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
> +                        int (*setkey)(void *private, const u8 *key,
> +                                      unsigned int keylen))
> +{
> +       struct key *keyring;
> +       struct public_key *pkey;
> +       char key_name[12];
> +       u32 keyid = *((u32 *)key);
> +       int err;
> +
> +       sprintf(key_name, "id:%08x", keyid);
> +       keyring = request_key(&key_type_asymmetric, key_name, NULL);
> +
> +       err = -ENOKEY;
> +       if (IS_ERR(keyring))
> +               goto out;
> +
> +       pkey = keyring->payload.data[asym_crypto];
> +       if (!pkey) {
> +               key_put(keyring);
> +               goto out;
> +       }
> +
> +       err = setkey(private, pkey->key, pkey->keylen);
> +       key_put(keyring);
> +
> +out:
> +       return err;
> +}

This seems entirely wrong to me. You cannot just assume that the
private key data are available in software form to the kernel and can
be extracted.

Please ensure you test this with a key in a TPM, or in a SGX software
enclave or something like that.

David, is there a way to do that test purely in software without
needing hardware support? We know that the data might not actually be
present in all cases... is there an easy test for that case?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[toc] | [next] | [standalone]

#1308416

David Woodhouse <dwmw2@infradead.org> wrote:

> David, is there a way to do that test purely in software without
> needing hardware support? We know that the data might not actually be
> present in all cases... is there an easy test for that case?

I have written a user TPM driver that talks to a userspace TPM implementation
out of the backend.  It's been pushed to the TPM driver guy but I'm not sure
what became of it.  I'll chase it up.

David

[toc] | [prev] | [next] | [standalone]

#1308422

[Multipart message — attachments visible in raw view] — view raw

On Wed, 2016-01-13 at 13:36 +0000, David Howells wrote:
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > David, is there a way to do that test purely in software without
> > needing hardware support? We know that the data might not actually be
> > present in all cases... is there an easy test for that case?
> 
> I have written a user TPM driver that talks to a userspace TPM implementation
> out of the backend.  It's been pushed to the TPM driver guy but I'm not sure
> what became of it.  I'll chase it up.

I was thinking of something a lot simpler — like a test hack with a key
type that just puts a *pointer* to the key data in the 'payload', to
ensure that nobody is violating the rules about directly touching the
payload (which should be private to the implementation).

-- 
dwmw2

[toc] | [prev] | [next] | [standalone]

#1308428

Hello David,
On 01/13/2016 04:27 AM, David Woodhouse wrote:
>> +static int alg_setkey_id(void *private, const u8 *key, unsigned int keylen,
>> > +                        int (*setkey)(void *private, const u8 *key,
>> > +                                      unsigned int keylen))
>> > +{
>> > +       struct key *keyring;
>> > +       struct public_key *pkey;
>> > +       char key_name[12];
>> > +       u32 keyid = *((u32 *)key);
>> > +       int err;
>> > +
>> > +       sprintf(key_name, "id:%08x", keyid);
>> > +       keyring = request_key(&key_type_asymmetric, key_name, NULL);
>> > +
>> > +       err = -ENOKEY;
>> > +       if (IS_ERR(keyring))
>> > +               goto out;
>> > +
>> > +       pkey = keyring->payload.data[asym_crypto];
>> > +       if (!pkey) {
>> > +               key_put(keyring);
>> > +               goto out;
>> > +       }
>> > +
>> > +       err = setkey(private, pkey->key, pkey->keylen);
>> > +       key_put(keyring);
>> > +
>> > +out:
>> > +       return err;
>> > +}
> This seems entirely wrong to me. You cannot just assume that the
> private key data are available in software form to the kernel and can
> be extracted.

Thanks for your feedback.
Yes, I'm aware that there might be something missing, and it might not
work in all the cases, but as far as the user interface is concerned
do you think having the ALG_SET_KEY, ALG_SET_PUBKEY along with
ALG_SET_PUBKEY_ID and ALG_SET_KEY_ID should be enough for all cases.
That's the main point of this patch - to get the user interface right.
The worst what can happen now if the keys need to be explicitly
extracted from a TPM HW is that the user will get ENOKEY error.
From the other hand shouldn't that happen under the hood in request_key()
It doesn't look right to me that the pf_alg module has to directly
interact with the TPM.

> 
> Please ensure you test this with a key in a TPM, or in a SGX software
> enclave or something like that.

I don't have any HW for this. I only tested it with the keys instantiated
using the keyctl tool.

Thanks,
-- 
TS

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web