Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-20 Thread Jarkko Sakkinen
On Fri, Jan 18, 2019 at 12:59:06PM -0800, James Bottomley wrote:
> On Fri, 2019-01-18 at 16:33 +0200, Jarkko Sakkinen wrote:
> > On Fri, Jan 11, 2019 at 07:28:58AM -0800, James Bottomley wrote:
> > > On Fri, 2019-01-11 at 16:02 +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Jan 08, 2019 at 05:43:53PM -0800, Andy Lutomirski wrote:
> > > > > (Also, do we have a sensible story of how the TPM interacts
> > > > > with hibernation at all?  Presumably we should at least try to
> > > > > replay the PCR operations that have occurred so that we can
> > > > > massage the PCRs into the same state post-hibernation.  Also,
> > > > > do we have any way for the kernel to sign something with the
> > > > > TPM along with an attestation that the signature was requested
> > > > > *by the kernel*?  Something like a sub-hierarchy of keys that
> > > > > the kernel explicitly prevents userspace from accessing?)
> > > > 
> > > > Kernel can keep it is own key hierarchy in memory as TPM2 chips
> > > > allow to offload data in encrypted form and load it to TPM when
> > > > it needs to use it.
> > > > 
> > > > The in-kernel resource manager that I initiated couple years ago
> > > > provides this type of functionality.
> > > 
> > > Actually, the resource manager only keeps volatile objects
> > > separated when in use not when offloaded.  The problem here is that
> > > the object needs to be persisted across reboots, so either it gets
> > > written to the NV area, bypassing the resource manager and making
> > > it globally visible or it has to get stored in TPM form in the
> > > hibernation image, meaning anyone with access to the TPM who can
> > > read the image can extract and load it. Further: anyone with access
> > > to the TPM can create a bogus sealed key and encrypt a malicious
> > > hibernation image with it.  So there are two additional problems
> > > 
> > >1. Given that the attacker may have access to the binary form of
> > > the
> > >   key, can we make sure only the kernel can get it released?
> > >2. How do we prevent an attacker with access to the TPM from
> > > creating a
> > >   bogus sealed key?
> > > 
> > > This is why I was thinking localities.
> > 
> > Why you would want to go for localities and not seal to PCRs?
> 
> Because the requested functionality was a key that would be accessible
> to the kernel and not to user space and also guaranteed created by the
> kernel.  The only discriminator we have to enforce that is the locality
> (assuming we reserve a locality as accessible to the kernel but
> inaccessible to userspace).  PCRs alone can't restrict where the key is
> accessed or created from.

OK, locality would probably make sense, assuming that the key is stored
in nvram.

/Jarkko

/Jarkko


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-18 Thread James Bottomley
On Fri, 2019-01-18 at 16:33 +0200, Jarkko Sakkinen wrote:
> On Fri, Jan 11, 2019 at 07:28:58AM -0800, James Bottomley wrote:
> > On Fri, 2019-01-11 at 16:02 +0200, Jarkko Sakkinen wrote:
> > > On Tue, Jan 08, 2019 at 05:43:53PM -0800, Andy Lutomirski wrote:
> > > > (Also, do we have a sensible story of how the TPM interacts
> > > > with hibernation at all?  Presumably we should at least try to
> > > > replay the PCR operations that have occurred so that we can
> > > > massage the PCRs into the same state post-hibernation.  Also,
> > > > do we have any way for the kernel to sign something with the
> > > > TPM along with an attestation that the signature was requested
> > > > *by the kernel*?  Something like a sub-hierarchy of keys that
> > > > the kernel explicitly prevents userspace from accessing?)
> > > 
> > > Kernel can keep it is own key hierarchy in memory as TPM2 chips
> > > allow to offload data in encrypted form and load it to TPM when
> > > it needs to use it.
> > > 
> > > The in-kernel resource manager that I initiated couple years ago
> > > provides this type of functionality.
> > 
> > Actually, the resource manager only keeps volatile objects
> > separated when in use not when offloaded.  The problem here is that
> > the object needs to be persisted across reboots, so either it gets
> > written to the NV area, bypassing the resource manager and making
> > it globally visible or it has to get stored in TPM form in the
> > hibernation image, meaning anyone with access to the TPM who can
> > read the image can extract and load it. Further: anyone with access
> > to the TPM can create a bogus sealed key and encrypt a malicious
> > hibernation image with it.  So there are two additional problems
> > 
> >1. Given that the attacker may have access to the binary form of
> > the
> >   key, can we make sure only the kernel can get it released?
> >2. How do we prevent an attacker with access to the TPM from
> > creating a
> >   bogus sealed key?
> > 
> > This is why I was thinking localities.
> 
> Why you would want to go for localities and not seal to PCRs?

Because the requested functionality was a key that would be accessible
to the kernel and not to user space and also guaranteed created by the
kernel.  The only discriminator we have to enforce that is the locality
(assuming we reserve a locality as accessible to the kernel but
inaccessible to userspace).  PCRs alone can't restrict where the key is
accessed or created from.

James



Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-18 Thread Jarkko Sakkinen
On Fri, Jan 11, 2019 at 07:28:58AM -0800, James Bottomley wrote:
> On Fri, 2019-01-11 at 16:02 +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 08, 2019 at 05:43:53PM -0800, Andy Lutomirski wrote:
> > > (Also, do we have a sensible story of how the TPM interacts with
> > > hibernation at all?  Presumably we should at least try to replay
> > > the PCR operations that have occurred so that we can massage the
> > > PCRs into the same state post-hibernation.  Also, do we have any
> > > way for the kernel to sign something with the TPM along with an
> > > attestation that the signature was requested *by the
> > > kernel*?  Something like a sub-hierarchy of keys that the kernel
> > > explicitly prevents userspace from accessing?)
> > 
> > Kernel can keep it is own key hierarchy in memory as TPM2 chips allow
> > to offload data in encrypted form and load it to TPM when it needs to
> > use it.
> > 
> > The in-kernel resource manager that I initiated couple years ago
> > provides this type of functionality.
> 
> Actually, the resource manager only keeps volatile objects separated
> when in use not when offloaded.  The problem here is that the object
> needs to be persisted across reboots, so either it gets written to the
> NV area, bypassing the resource manager and making it globally visible
> or it has to get stored in TPM form in the hibernation image, meaning
> anyone with access to the TPM who can read the image can extract and
> load it. Further: anyone with access to the TPM can create a bogus
> sealed key and encrypt a malicious hibernation image with it.  So there
> are two additional problems
> 
>1. Given that the attacker may have access to the binary form of the
>   key, can we make sure only the kernel can get it released?
>2. How do we prevent an attacker with access to the TPM from creating a
>   bogus sealed key?
> 
> This is why I was thinking localities.

Why you would want to go for localities and not seal to PCRs?

/Jarkko


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-11 Thread Jarkko Sakkinen
On Wed, Jan 09, 2019 at 10:34:42AM -0800, Andy Lutomirski wrote:
> I suppose I should go read the 2.0 spec. I’ve read the 1.2 spec, but I
> always assumed that 2.0 was essentially a superset of 1.2
> functionality.

They are essentially different protocols. No real compatibility.


> Can the kernel filter TPM 2.0 operations?  If so, then a signature
> that the kernel would have prevented user code from generating is de
> facto an attestation that the kernel generated it (or that the kernel
> was compromised, which is sort of equivalent).

You shoud look into TPM resource manager that I implemented with great
work from James on session swapping and see how far it scales what you
have in mind. It is currently exposed only to the user space but could
be easily made an in-kernel API.

Side-topic: right now the TPM driver can be compiled as a module when
its APIs are not used by the kernel (namely IMA and trusted keys) with
some Kconfig magic. Since it looks like that there will be even more
customers, I think it would make sense to make the TPM driver core as
part of the core kernel (device drivers for different types of chips
could still be modules). I've proposed this before maybe two times, but
it has always been rejected.

/Jarkko


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-11 Thread Jarkko Sakkinen
On Thu, Jan 10, 2019 at 02:11:55AM +0800, joeyli wrote:
> > Well, I think here, if we were actually trying to solve the problem of
> > proving the hibernated image were the same one we would need to prove
> > some log of the kernel operation came to a particular value *after* the
> > hibernated image were restored ... it's not really possible to
> > condition key release which must occur before the restore on that
> > outcome, so it strikes me we need more than a simple release bound to
> > PCR values.
> >
> 
> hm... I am studying your information. But I have a question...
> 
> If PCR is not capped and the root be compromised, is it possible that a
> sealed bundle also be compromised? 
> 
> Is it possible that kernel can produce a sealed key with PCR by TPM when
> booting? Then kernel caps a PCR by a constant value before the root is
> available for userland. Then the sealed key can be exposed to userland
> or be attached on hibernate image. Even the root be compromised, the TPM
> trusted key is still secure.

I think this even might be reasonable. Especially when we land James'
encrypted sessions patches at some point.

/Jarkko


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-11 Thread James Bottomley
On Fri, 2019-01-11 at 16:02 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 08, 2019 at 05:43:53PM -0800, Andy Lutomirski wrote:
> > (Also, do we have a sensible story of how the TPM interacts with
> > hibernation at all?  Presumably we should at least try to replay
> > the PCR operations that have occurred so that we can massage the
> > PCRs into the same state post-hibernation.  Also, do we have any
> > way for the kernel to sign something with the TPM along with an
> > attestation that the signature was requested *by the
> > kernel*?  Something like a sub-hierarchy of keys that the kernel
> > explicitly prevents userspace from accessing?)
> 
> Kernel can keep it is own key hierarchy in memory as TPM2 chips allow
> to offload data in encrypted form and load it to TPM when it needs to
> use it.
> 
> The in-kernel resource manager that I initiated couple years ago
> provides this type of functionality.

Actually, the resource manager only keeps volatile objects separated
when in use not when offloaded.  The problem here is that the object
needs to be persisted across reboots, so either it gets written to the
NV area, bypassing the resource manager and making it globally visible
or it has to get stored in TPM form in the hibernation image, meaning
anyone with access to the TPM who can read the image can extract and
load it. Further: anyone with access to the TPM can create a bogus
sealed key and encrypt a malicious hibernation image with it.  So there
are two additional problems

   1. Given that the attacker may have access to the binary form of the
  key, can we make sure only the kernel can get it released?
   2. How do we prevent an attacker with access to the TPM from creating a
  bogus sealed key?

This is why I was thinking localities.

James



Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-11 Thread Jarkko Sakkinen
On Tue, Jan 08, 2019 at 05:43:53PM -0800, Andy Lutomirski wrote:
> (Also, do we have a sensible story of how the TPM interacts with
> hibernation at all?  Presumably we should at least try to replay the
> PCR operations that have occurred so that we can massage the PCRs into
> the same state post-hibernation.  Also, do we have any way for the
> kernel to sign something with the TPM along with an attestation that
> the signature was requested *by the kernel*?  Something like a
> sub-hierarchy of keys that the kernel explicitly prevents userspace
> from accessing?)

Kernel can keep it is own key hierarchy in memory as TPM2 chips allow
to offload data in encrypted form and load it to TPM when it needs to
use it.

The in-kernel resource manager that I initiated couple years ago
provides this type of functionality.

/Jarkko


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Pavel Machek
Hi!

> > > Note if someone has your laptop and the ability to boot their own
> > > kernels, they could always corrupt the kernel into decrypting the
> > > image or giving you the unsealed key, but there's no real way of
> > > preventing that even with PCR sealing or lockdown, so the basis for
> > > the threat model is very much my laptop in my possession running my
> > > kernel.
> > 
> > I'm not entirely sure I agree.  With a TPM-aware bootloader, it
> > really ought to be possible to seal to PCRs such that a corrupted
> > kernel can't restore the image.  Obviously a *compromised* but
> > otherwise valid kernel will be able to restore the image.
> 
> It is possible to seal the key so that only the same booted kernel can
> restore the image, yes.  One of the measurements that goes into the
> boot log is the hash of the kernel and you can seal to this value ...
> obviously if you upgrade your kernel RPM (or shim or grub) this value
> changes and you'd lose the ability to restore the hibernated image, but
> since the image is very kernel specific, that's probably OK.

Non-ancient kernels actually support hibernation by one kernel and
restore by another one.

But yes, normally it is same kernel binary doing hibernation and
restore.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread James Bottomley
On Wed, 2019-01-09 at 12:12 -0800, Andy Lutomirski wrote:
> On Wed, Jan 9, 2019 at 11:46 AM James Bottomley
>  wrote:

[...]
> > > I’m not sure I follow. Here are the two properties I’d like to
> > > see:
> > > 
> > > 1. If you have an encrypted hibernation image, the only thing you
> > > should be able to do with it is to restore it. So only an actual
> > > Linux kernel in hibernation restore mode ought to be able to
> > > restore it.  We get this if the image can only be read with
> > > appropriate PCRs and then only by the kernel.  This way, you
> > > can’t just read out secrets from the image if you steal a laptop
> > > — you have to actually boot the thing.
> > 
> > Right, this we can do and if you use a TPM sealed encryption key,
> > you can guarantee the image will only restore on the same physical
> > system. You don't need PCRs for this, just the TPM and the locality
> > enforcement.
> > 
> > Note if someone has your laptop and the ability to boot their own
> > kernels, they could always corrupt the kernel into decrypting the
> > image or giving you the unsealed key, but there's no real way of
> > preventing that even with PCR sealing or lockdown, so the basis for
> > the threat model is very much my laptop in my possession running my
> > kernel.
> 
> I'm not entirely sure I agree.  With a TPM-aware bootloader, it
> really ought to be possible to seal to PCRs such that a corrupted
> kernel can't restore the image.  Obviously a *compromised* but
> otherwise valid kernel will be able to restore the image.

It is possible to seal the key so that only the same booted kernel can
restore the image, yes.  One of the measurements that goes into the
boot log is the hash of the kernel and you can seal to this value ...
obviously if you upgrade your kernel RPM (or shim or grub) this value
changes and you'd lose the ability to restore the hibernated image, but
since the image is very kernel specific, that's probably OK.

> But this is all barking up the wrong tree.  If you want your laptop
> to resist physical theft such that whoever stole it can boot it but
> can't directly extract any data, you want to use dm-crypt (or, haha,
> OPAL, but I just read a paper about some people who evaluated a bunch
> of drives and had a very hard time finding one that actually
> implemented OPAL in a usefully secure manner).  A LUKS replacement or
> wrapper that does something intelligent with the TPM would be
> great.  This kind of thing IMO does not belong in hibernation.

Right, so the simplest way of doing this is to save the image on an
encrypted partition or filesystem which must be unlocked by a password
on boot.

> IOW, I think we do get about as much as we would want if we just seal
> with a locality that only allows kernel use and ignore PCRs entirely.
> This makes it so that you need the ability to run ring 0 code to
> decrypt the image, which means that we get all the nice lockdown
> properties.

Yes, it's a useful balance of security and ease of implementation, I
think.

> > > 2. You shouldn’t be able to create an intentionally corrupt image
> > > that pwns you when you restore it unless you have already pwned
> > > the kernel.
> > 
> > So here there's a problem: the policy stated above governs key
> > *use* not key creation, so anyone can create a key that has a
> > locality restriction.  The way to guarantee that the key was
> > created by something that has access to the locality is to have a
> > parent key with a locality use policy (key creation requires parent
> > key use authorization).  Which means every system would have to
> > create a persistent parent key for the kernel to use (the kernel
> > could do this and it could be made NV resident for persistence, so
> > it's not impossible, just complicated).
> 
> Why does anything here need to be persistent?  The kernel could
> create a locality-restricted key on the fly, use it to sign and/or
> seal the hibernation image, and write the wrapped key blob into the
> hibernation image.

Yes, but so could I as a malicious user with a desire to create a bad
hibernation image.  I could do it entirely in userspace.  I need access
to the TPM to seal the key, but I'd get that:  All you see in-kernel is
a single policy sha256 sum and you can't reverse that back to the
actual policy, so the kernel has no idea what policy is being sealed
and so can't police the policy.

In order to get the security that only the kernel created the
hibernation key, you need to have a key parent with a policy that only
allows in-kernel locality use so you know the child key was created by
the kernel and nothing else.

>  > I suppose that a good summary of my opinion is that there is no
> point
> > > to kernel support for encrypted hibernation images until lockdown
> > > is upstream.
> > 
> > I really don't think lockdown helps.  If we implement locality
> > isolation for the kernels use of keys, a properly functioning
> > kernel isn't going to be tricked into releasing one of its keys to

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Andy Lutomirski
On Wed, Jan 9, 2019 at 11:46 AM James Bottomley
 wrote:
>
> On Wed, 2019-01-09 at 10:34 -0800, Andy Lutomirski wrote:
> > > > On Jan 8, 2019, at 10:49 PM, James Bottomley  > > > senpartnership.com> wrote:
> > > >

> >   If so, then a signature that the kernel would have prevented user
> > code from generating is de facto an attestation that the kernel
> > generated it (or that the kernel was compromised, which is sort of
> > equivalent).
>
> The TPM's idea of this is it polices by authorization.  Now one of the
> things we can do here is add what's called locality based
> authorization.  we have three non-uefi localities to play with and we
> could enforce walling one off for the kernel only to use, so a kernel
> key could come with a policy requiring use of the kernel locality for
> use of the key.  That would give you an effective guarantee that only
> the kernel could use this key.  Note the enforcement of locality would
> require a key policy, which is easy for TPM 1.2, but requires the use
> of a policy session for TPM 2.0 which means we'd have to improve our
> policy session handling.

Hmm.  On an *extremely* brief reading of what "locality" means, this
seems entirely sensible.

> >
> > I’m not sure I follow. Here are the two properties I’d like to see:
> >
> > 1. If you have an encrypted hibernation image, the only thing you
> > should be able to do with it is to restore it. So only an actual
> > Linux kernel in hibernation restore mode ought to be able to restore
> > it.  We get this if the image can only be read with appropriate PCRs
> > and then only by the kernel.  This way, you can’t just read out
> > secrets from the image if you steal a laptop — you have to actually
> > boot the thing.
>
> Right, this we can do and if you use a TPM sealed encryption key, you
> can guarantee the image will only restore on the same physical system.
> You don't need PCRs for this, just the TPM and the locality
> enforcement.
>
> Note if someone has your laptop and the ability to boot their own
> kernels, they could always corrupt the kernel into decrypting the image
> or giving you the unsealed key, but there's no real way of preventing
> that even with PCR sealing or lockdown, so the basis for the threat
> model is very much my laptop in my possession running my kernel.

I'm not entirely sure I agree.  With a TPM-aware bootloader, it really
ought to be possible to seal to PCRs such that a corrupted kernel
can't restore the image.  Obviously a *compromised* but otherwise
valid kernel will be able to restore the image.

But this is all barking up the wrong tree.  If you want your laptop to
resist physical theft such that whoever stole it can boot it but can't
directly extract any data, you want to use dm-crypt (or, haha, OPAL,
but I just read a paper about some people who evaluated a bunch of
drives and had a very hard time finding one that actually implemented
OPAL in a usefully secure manner).  A LUKS replacement or wrapper that
does something intelligent with the TPM would be great.  This kind of
thing IMO does not belong in hibernation.

IOW, I think we do get about as much as we would want if we just seal
with a locality that only allows kernel use and ignore PCRs entirely.
This makes it so that you need the ability to run ring 0 code to
decrypt the image, which means that we get all the nice lockdown
properties.

>
> > 2. You shouldn’t be able to create an intentionally corrupt image
> > that pwns you when you restore it unless you have already pwned the
> > kernel.
>
> So here there's a problem: the policy stated above governs key *use*
> not key creation, so anyone can create a key that has a locality
> restriction.  The way to guarantee that the key was created by
> something that has access to the locality is to have a parent key with
> a locality use policy (key creation requires parent key use
> authorization).  Which means every system would have to create a
> persistent parent key for the kernel to use (the kernel could do this
> and it could be made NV resident for persistence, so it's not
> impossible, just complicated).

Why does anything here need to be persistent?  The kernel could create
a locality-restricted key on the fly, use it to sign and/or seal the
hibernation image, and write the wrapped key blob into the hibernation
image.

 > I suppose that a good summary of my opinion is that there is no point
> > to kernel support for encrypted hibernation images until lockdown is
> > upstream.
>
> I really don't think lockdown helps.  If we implement locality
> isolation for the kernels use of keys, a properly functioning kernel
> isn't going to be tricked into releasing one of its keys to userspace.
> A buggy kernel might be exploited to cause it to give one up but that
> would happen irrespective of lockdown and, of course, all bets are off
> if the attacker can boot their own kernel.
>

I'm not saying that lockdown helps.  I'm saying that encrypting the
hibernation image in the kernel may be 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread James Bottomley
On Wed, 2019-01-09 at 10:34 -0800, Andy Lutomirski wrote:
> > > On Jan 8, 2019, at 10:49 PM, James Bottomley  > > senpartnership.com> wrote:
> > > 
> > > On Tue, 2019-01-08 at 17:43 -0800, Andy Lutomirski wrote:
> > > [Adding Jarkko because this stuff relates to the TPM.]
> > > Anyway, if we're talking about the TPM, it seems like the entire
> > > "trusted key" mechanism in the kernel is missing the point.  If I
> > > want to encrypt something like a hibernation image on a machine
> > > with a TPM, it makes essentially no sense to me that we would get
> > > a key with a known raw value that is merely TPM-backed (i.e. the
> > > "trusted key") and use that to decrypt the image.  The right way
> > > to do it is to the use the TPM as it was intended to be used:
> > > generate a single-use key that protects the hibernation image and
> > > seal *that* directly on the TPM, such that it can only be
> > > unsealed with appropriate PCR values.  Heck, we could even use
> > > one of the fancy NV counters such that we *can't* decrypt the
> > > image later on.  And using HMAC or any AE construction the normal
> > > way is also wrong -- we should *hash* the image and sign the hash
> > > directly on the TPM so that the restore code can validate the PCR
> > > values that were in place when the hibernation image was
> > > created.  [0]
> > 
> > Well, theoretically, trusted keys can be used for PCR sealed
> > bundles, at least in 1.2 ... I'm not sure the 2.0 one actually
> > works because you have to construct the policy session outside the
> > kernel.
> 
> I suppose I should go read the 2.0 spec. I’ve read the 1.2 spec, but
> I always assumed that 2.0 was essentially a superset of 1.2
> functionality.

It was sold as an incremental upgrade, but in practice, adding crypto
agility and flexible policy (the main 2.0 enhancements) meant that the
API is redically different.

> > >  Presumably we should at least try to replay the PCR operations
> > > that have occurred so that we can massage the PCRs into the same
> > > state post-hibernation.  Also, do we have any way for the kernel
> > > to sign something with the TPM along with an attestation that the
> > > signature was requested *by the kernel*?  Something like a sub-
> > > hierarchy of keys that the kernel explicitly prevents userspace
> > > from accessing?)
> > 
> > We're just growing that now with the TPM asymmetric operations.
> > Attesting that the kernel requested the signature is harder.  The
> > TPM can attest to log entries (as it does for the UEFI log and IMA)
> > and it can certify keys, but that only proves they're TPM resident
> > not who the requestor was.  Effectively the latter is an assertion
> > about who knows the key authority, which is hard to prove.
> 
> Can the kernel filter TPM 2.0 operations?

There is a proposal for this, but the proposal is on the operations by
type, not the content.  Meaning we can't really police loading and
using a kernel key because we can't distinguish whether any key is a
kernel key.  We can't forbid all key load operations to non-kernel
because that removes most of the TPM usefulness as a keystore.

>   If so, then a signature that the kernel would have prevented user
> code from generating is de facto an attestation that the kernel
> generated it (or that the kernel was compromised, which is sort of
> equivalent).

The TPM's idea of this is it polices by authorization.  Now one of the
things we can do here is add what's called locality based
authorization.  we have three non-uefi localities to play with and we
could enforce walling one off for the kernel only to use, so a kernel
key could come with a policy requiring use of the kernel locality for
use of the key.  That would give you an effective guarantee that only
the kernel could use this key.  Note the enforcement of locality would
require a key policy, which is easy for TPM 1.2, but requires the use
of a policy session for TPM 2.0 which means we'd have to improve our
policy session handling.

> > > [0] If you take some data, run it through an authenticated
> > > encryption algorithm, and sign (key, nonce, tag), I think you're
> > > operating outside of the accepted security definitions if you
> > > expect this to guarantee that the data wasn't tampered with.  I'm
> > > reasonably confident that there are quite a few excellent AE
> > > algorithms that completely fail if used this like this.  In fact,
> > > pretty much all of the modern fast ones probably fail.  AE is for
> > > when the key is *secret*.
> > 
> > Well, I think here, if we were actually trying to solve the problem
> > of proving the hibernated image were the same one we would need to
> > prove some log of the kernel operation came to a particular value
> > *after* the hibernated image were restored ... it's not really
> > possible to condition key release which must occur before the
> > restore on that outcome, so it strikes me we need more than a
> > simple release bound to PCR values.
> 
> I’m not sure I 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Andy Lutomirski
>> On Jan 8, 2019, at 10:49 PM, James Bottomley 
>>  wrote:
>>
>> On Tue, 2019-01-08 at 17:43 -0800, Andy Lutomirski wrote:
>> [Adding Jarkko because this stuff relates to the TPM.]
>
>> Anyway, if we're talking about the TPM, it seems like the entire
>> "trusted key" mechanism in the kernel is missing the point.  If I
>> want to encrypt something like a hibernation image on a machine with
>> a TPM, it makes essentially no sense to me that we would get a key
>> with a known raw value that is merely TPM-backed (i.e. the "trusted
>> key") and use that to decrypt the image.  The right way to do it is
>> to the use the TPM as it was intended to be used: generate a single-
>> use key that protects the hibernation image and seal *that* directly
>> on the TPM, such that it can only be unsealed with appropriate PCR
>> values.  Heck, we could even use one of the fancy NV counters such
>> that we *can't* decrypt the image later on.  And using HMAC or any AE
>> construction the normal way is also wrong -- we should *hash* the
>> image and sign the hash directly on the TPM so that the restore code
>> can validate the PCR values that were in place when the hibernation
>> image was created.  [0]
>
> Well, theoretically, trusted keys can be used for PCR sealed bundles,
> at least in 1.2 ... I'm not sure the 2.0 one actually works because you
> have to construct the policy session outside the kernel.

I suppose I should go read the 2.0 spec. I’ve read the 1.2 spec, but I
always assumed that 2.0 was essentially a superset of 1.2
functionality.

>>  Presumably we should at least try to replay the PCR operations that
>> have occurred so that we can massage the PCRs into the same state
>> post-hibernation.  Also, do we have any way for the kernel to sign
>> something with the TPM along with an attestation that the signature
>> was requested *by the kernel*?  Something like a sub-hierarchy of
>> keys that the kernel explicitly prevents userspace from accessing?)
>
> We're just growing that now with the TPM asymmetric operations.
> Attesting that the kernel requested the signature is harder.  The TPM
> can attest to log entries (as it does for the UEFI log and IMA) and it
> can certify keys, but that only proves they're TPM resident not who the
> requestor was.  Effectively the latter is an assertion about who knows
> the key authority, which is hard to prove.

Can the kernel filter TPM 2.0 operations?  If so, then a signature
that the kernel would have prevented user code from generating is de
facto an attestation that the kernel generated it (or that the kernel
was compromised, which is sort of equivalent).

>
>> [0] If you take some data, run it through an authenticated encryption
>> algorithm, and sign (key, nonce, tag), I think you're operating
>> outside of the accepted security definitions if you expect this to
>> guarantee that the data wasn't tampered with.  I'm reasonably
>> confident that there are quite a few excellent AE algorithms that
>> completely fail if used this like this.  In fact, pretty much all of
>> the modern fast ones probably fail.  AE is for when the key is
>> *secret*.
>
> Well, I think here, if we were actually trying to solve the problem of
> proving the hibernated image were the same one we would need to prove
> some log of the kernel operation came to a particular value *after* the
> hibernated image were restored ... it's not really possible to
> condition key release which must occur before the restore on that
> outcome, so it strikes me we need more than a simple release bound to
> PCR values.

I’m not sure I follow. Here are the two properties I’d like to see:

1. If you have an encrypted hibernation image, the only thing you
should be able to do with it is to restore it. So only an actual Linux
kernel in hibernation restore mode ought to be able to restore it.  We
get this if the image can only be read with appropriate PCRs and then
only by the kernel.  This way, you can’t just read out secrets from
the image if you steal a laptop — you have to actually boot the thing.

2. You shouldn’t be able to create an intentionally corrupt image that
pwns you when you restore it unless you have already pwned the kernel.

Maybe the “kernel” bit in #1 can be relaxed to “root” without totally
defeating the purpose, but if some random non-root process that
happens to have access to /dev/tpm* can make a valid-looking TPM
image, then I think we fail.  Limiting it to the kernel is only
dubiously better than limiting it to root until we implement lockdown,
in which case it's important.

#2 only really matters with lockdown.

I suppose that a good summary of my opinion is that there is no point
to kernel support for encrypted hibernation images until lockdown is
upstream.

--Andy


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Stephan Mueller
Am Mittwoch, 9. Januar 2019, 18:34:55 CET schrieb Eric Biggers:

Hi Eric,

> That would not meet my performance requirements as I want to precompute
> HKDF-Extract, and then do HKDF-Expand many times.  Also the HKDF-Expand part
> should be thread-safe and not require allocating memory, especially not a
> whole crypto_shash tfm every time.
> 
> So presumably with crypto_rng, crypto_rng_reset() would need to take the
> input keyring material and salt and do HKDF-Extract (like my
> fscrypt_init_hkdf()), and crypto_rng_generate() would need to take the
> application-specific info string and do HKDF-Expand (like my
> fscrypt_hkdf_expand()).

Great, that was the idea I had in mind as well. Maybe the example was not 
right to convey that. Let me work on that.
> 
> It is ugly though.  Please also consider just having simple crypto_hkdf_*()
> helper functions which wrap a HMAC tfm along the lines of my patch, rather
> than shoehorning it into the crypto_rng API.
> 
> - Eric



Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread joeyli
Hi James

On Tue, Jan 08, 2019 at 10:49:39PM -0800, James Bottomley wrote:
> On Tue, 2019-01-08 at 17:43 -0800, Andy Lutomirski wrote:
> > [Adding Jarkko because this stuff relates to the TPM.]
> > 
> > On Tue, Jan 8, 2019 at 4:44 PM James Bottomley
> >  wrote:
> > > 
> > > On Tue, 2019-01-08 at 15:54 -0800, Andy Lutomirski wrote:
> > > > > On Jan 7, 2019, at 11:09 PM, Stephan Mueller  > > > > de>
> > > > > wrote:
> > > > > 
> > > > > Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:
> > > > > 
> > > > > Hi Herbert,
> > > > > 
> > > > > > Are we going to have multiple implementations for the same
> > > > > > KDF? If not then the crypto API is not a good fit.  To
> > > > > > consolidate multiple implementations of the same KDF, simply
> > > > > > provide helpers for them.
> > > > > 
> > > > > It is unlikely to have multiple implementations of a KDF.
> > > > > However, KDFs relate to hashes like block chaining modes to raw
> > > > > block ciphers. Thus a KDF can be applied with different hashes.
> > > > > 
> > > > > My idea was to add template support to RNGs (because KDFs are
> > > > > effectively a type of RNG since they produce an arbitrary
> > > > > output from a fixed input). The KDFs would be a template
> > > > > wrapping hashes.  For example, the CTR-KDF from SP800-108 could
> > > > > be instantiated like kdf-ctr(sha256).
> > > > > 
> > > > > 
> > > > 
> > > > I think that, if the crypto API is going to grow a KDF facility,
> > > > it should be done right. Have a key type or flag or whatever that
> > > > says “this key may *only* be used to derive keys using such-and-
> > > > such algorithm”, and have a helper to derive a key.  That helper
> > > > should take some useful parameters and mix them in:
> > > > 
> > > > - What type of key is being derived?  ECDSA signing key?  HMAC
> > > > key?  AES key?
> > > > 
> > > > - Can user code access the derived key?
> > > > 
> > > > - What is the key’s purpose?  “Encrypt and authenticate a
> > > > hibernation image” would be a purpose.
> > > > 
> > > > - Number of bytes.
> > > > 
> > > > All of these parameters should be mixed in to the key derivation.
> > > > 
> > > > Also, an AE key, even for AES+HMAC, should be just one derived
> > > > key.  If you need 512 bits, ask for a 512-bit key, not two 256-
> > > > bit keys.
> > > 
> > > Actually, it would be enormously helpful if we could reuse these
> > > pieces for the TPM as well.  It has two KDFs: KDFa, which is the
> > > CTR-KDF from SP800-108 and KDFe which is the SP800-56A KDF for
> > > elliptic curve one pass Diffie Hellman, so if we're going to do the
> > > former, I'd really like the latter as well.
> > > 
> > > The way the TPM parametrises input to both KDFs is
> > > 
> > > (hashAlg, key, label, contextU, contextV, bits)
> > > 
> > > Where
> > > 
> > > hashAlg  = the hash algorithm used as the PRF
> > > key  = the input parameter of variable bit size or
> > >the x co-ordinate of the shared point
> > > label= An ASCII string representing the use
> > > contextU = public input U
> > > contextV = public input V
> > > bits = number of output bits
> > > 
> > > Is that a good enough parametrisation (not the only way you
> > > distinguish uses is with the label, which is not
> > > recoverable)?  ContextU and ContextV are simply concatenated to
> > > form the full Context of SP800-108, but we tend to need two
> > > separate inputs (for KDFe they're the public x co-ordinates of the
> > > points of the two inputs to ECDH for instance; in KDFa they're
> > > usually the local and remote nonces).
> > > 
> > > The labels for TPM usage are things like "INTEGRITY" for HMAC keys
> > > or "CFB" when generating an aes128-cfb session key. For KDFe, the
> > > tpm seems to like the label "SECRET".  Although the TPM specifies
> > > fixed short strings for the label, nothing prevents them being
> > > longer like the purpose you state above (essentially we could mix
> > > purpose, use and key type into the label and the contexts).
> > > 
> > 
> > That really ought to cover anything the kernel needs.
> > 
> > But can you explain what's up with with KDFe?  The obvious searches
> > end up with just warnings that the US currently has no government :(
> 
> You mean you can't find SP100-56A because NIST is a government entity
> and it's discontinued its website because of the government shutdown? 
> No idea, I only live here, you'll have to ask a real American.
> 
> ACM does have a copy:
> 
> http://delivery.acm.org/10.1145/221/2206270/SP800-56A_Revision1_Mar08-2007.pdf?ip=50.35.68.20=2206270=OPEN=4D4702B0C3E38B35%2E4D4702B0C3E38B35%2E4D4702B0C3E38B35%2E6D218144511F3437&__acm__=1546993111_ed9c8bd24b2f838c829d428aac7f5d71
> 
> > Anyway, if we're talking about the TPM, it seems like the entire
> > "trusted key" mechanism in the kernel is missing the point.  If I
> > want to encrypt something like a hibernation image on a machine with
> > a TPM, it makes essentially no sense to me that we would get a 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Eric Biggers
On Wed, Jan 09, 2019 at 11:17:45AM +0100, Stephan Mueller wrote:
> Am Mittwoch, 9. Januar 2019, 09:21:04 CET schrieb Eric Biggers:
> 
> Hi Eric,
> > 
> > FWIW, it's been very slow going since I've been working on other projects
> > and I also need to be very sure to get the API changes right, but I still
> > plan to change the KDF in fscrypt (a.k.a. ext4/f2fs/ubifs encryption) to
> > HKDF-SHA512 as part of a larger set of improvements to how fscrypt
> > encryption keys are managed. I sent the last patchset a year ago
> > (https://marc.info/?l=linux-fsdevel=150879493206257) but I'm working to
> > revive it.  In the work-in-progress version in my git tree, this is the
> > commit that adds a HKDF implementation as fs/crypto/hkdf.c:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?id=e8a78767131c9717ee838f0c4e307948d65a4427
> > It basically just wraps a crypto_shash for "hmac(sha512)".
> > 
> > I'd be fine with using a common implementation instead, provided that it
> > gives the same functionality, including supporting user-specified salt and
> > application-specific info strings, and isn't slower or more complex to use.
> > 
> > (This comment is solely on the tangential discussion about KDF
> > implementations; I've not looked at the hibernation image encryption stuff
> > yet.)
> 
> Thanks for the clarification. I have started a generic HKDF implementation 
> for 
> the kernel crypto API which lead to the questions above. I would then also 
> try 
> to provide a HKDF proposal.
> 
> To use the (H)KDF, I currently envision 2 calls apart from alloc/free. The 
> following code would serve as an example.
> 
>  * Example without proper error handling:
>  *  char *keying_material = "\x00\x11\x22\x33\x44\x55\x66\x77";
>  *  char *label_context = "\xde\xad\xbe\xef\x00\xde\xad\xbe\xef";
>  *  kdf = crypto_alloc_rng(name, 0, 0);
>  *  crypto_rng_reset(kdf, keying_material, 8);
>  *  crypto_rng_generate(kdf, label_context, 9, outbuf, outbuflen);
> 
> That hopefully should be simple enough.
> 
> For HKDF, as mentioned, I would envision to use a struct instead of a char * 
> for the label_context to communicate IKM, Salt, and the label/info 
> information.
> 
> Ciao
> Stephan
> 

That would not meet my performance requirements as I want to precompute
HKDF-Extract, and then do HKDF-Expand many times.  Also the HKDF-Expand part
should be thread-safe and not require allocating memory, especially not a whole
crypto_shash tfm every time.

So presumably with crypto_rng, crypto_rng_reset() would need to take the input
keyring material and salt and do HKDF-Extract (like my fscrypt_init_hkdf()), and
crypto_rng_generate() would need to take the application-specific info string
and do HKDF-Expand (like my fscrypt_hkdf_expand()).

It is ugly though.  Please also consider just having simple crypto_hkdf_*()
helper functions which wrap a HMAC tfm along the lines of my patch, rather than
shoehorning it into the crypto_rng API.

- Eric


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread James Bottomley
On Wed, 2019-01-09 at 08:05 +0100, Stephan Mueller wrote:
> Am Mittwoch, 9. Januar 2019, 07:58:28 CET schrieb James Bottomley:
> 
> Hi James,
> 
> > On Wed, 2019-01-09 at 07:45 +0100, Stephan Mueller wrote:
> > > Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James
> > > Bottomley:
> > > 
> > > Hi James,
> > > 
> > > > Actually, it would be enormously helpful if we could reuse
> > > > these pieces for the TPM as well.
> > > 
> > > Could you please help me understand whether the KDFs in TPM are
> > > directly usable as a standalone cipher primitive or does it go
> > > together with additional  key generation operations?
> > 
> > They're used as generators ... which means they deterministically
> > produce keys from what the TPM calls seeds so we can get crypto
> > agility of TPM 2.0 ... well KDFa does.  KDFe is simply what NIST
> > recommends you do when using EC for a shared key agreement ... and
> > really we shouldn't be using ECDH in the kernel without it.
> > 
> 
> Thanks for clarifying. That would mean that indeed we would have
> hardware-provided KDF implementations that may be usable with the
> kernel crypto API.

Just on this point, the TPM doesn't actually provide any KDFa or e API,
so it can't be used for hardware acceleration (and even if it did, the
TPM is a pretty slow engine, so software would be faster anyway).  We
need these algorithms in software because the TPM uses key agreements
derived from shared secrets to produce session encryption keys to
ensure confidentiality and integrity (HMAC key), so we establish the
shared secret then have to derive our key in software and the TPM
derives the same key internally and we use the shared derived key to
symmetrically encrypt and/or HMAC secret communications.

James



Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Stephan Mueller
Am Mittwoch, 9. Januar 2019, 09:21:04 CET schrieb Eric Biggers:

Hi Eric,
> 
> FWIW, it's been very slow going since I've been working on other projects
> and I also need to be very sure to get the API changes right, but I still
> plan to change the KDF in fscrypt (a.k.a. ext4/f2fs/ubifs encryption) to
> HKDF-SHA512 as part of a larger set of improvements to how fscrypt
> encryption keys are managed. I sent the last patchset a year ago
> (https://marc.info/?l=linux-fsdevel=150879493206257) but I'm working to
> revive it.  In the work-in-progress version in my git tree, this is the
> commit that adds a HKDF implementation as fs/crypto/hkdf.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?i
> d=e8a78767131c9717ee838f0c4e307948d65a4427 It basically just wraps a
> crypto_shash for "hmac(sha512)".
> 
> I'd be fine with using a common implementation instead, provided that it
> gives the same functionality, including supporting user-specified salt and
> application-specific info strings, and isn't slower or more complex to use.
> 
> (This comment is solely on the tangential discussion about KDF
> implementations; I've not looked at the hibernation image encryption stuff
> yet.)

Thanks for the clarification. I have started a generic HKDF implementation for 
the kernel crypto API which lead to the questions above. I would then also try 
to provide a HKDF proposal.

To use the (H)KDF, I currently envision 2 calls apart from alloc/free. The 
following code would serve as an example.

 * Example without proper error handling:
 *  char *keying_material = "\x00\x11\x22\x33\x44\x55\x66\x77";
 *  char *label_context = "\xde\xad\xbe\xef\x00\xde\xad\xbe\xef";
 *  kdf = crypto_alloc_rng(name, 0, 0);
 *  crypto_rng_reset(kdf, keying_material, 8);
 *  crypto_rng_generate(kdf, label_context, 9, outbuf, outbuflen);

That hopefully should be simple enough.

For HKDF, as mentioned, I would envision to use a struct instead of a char * 
for the label_context to communicate IKM, Salt, and the label/info 
information.

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-09 Thread Eric Biggers
On Wed, Jan 09, 2019 at 08:05:21AM +0100, Stephan Mueller wrote:
> Am Mittwoch, 9. Januar 2019, 07:58:28 CET schrieb James Bottomley:
> 
> Hi James,
> 
> > On Wed, 2019-01-09 at 07:45 +0100, Stephan Mueller wrote:
> > > Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:
> > > 
> > > Hi James,
> > > 
> > > > Actually, it would be enormously helpful if we could reuse these
> > > > pieces for the TPM as well.
> > > 
> > > Could you please help me understand whether the KDFs in TPM are
> > > directly usable as a standalone cipher primitive or does it go
> > > together with additional  key generation operations?
> > 
> > They're used as generators ... which means they deterministically
> > produce keys from what the TPM calls seeds so we can get crypto agility
> > of TPM 2.0 ... well KDFa does.  KDFe is simply what NIST recommends you
> > do when using EC for a shared key agreement ... and really we shouldn't
> > be using ECDH in the kernel without it.
> > 
> 
> Thanks for clarifying. That would mean that indeed we would have hardware-
> provided KDF implementations that may be usable with the kernel crypto API.
> 
> [...]
> > 
> > > Would it be appropriate, to implement a type cast to a structure from
> > > the u8 pointer?
> > > 
> > > E.g. for the aforementioned label/context data, we could define
> > > something like
> > > 
> > > struct crypto_kdf_ctr {
> > > 
> > >   char *label;
> > >   size_t label_len;
> > >   u8 *contextU;
> > >   size_t contextU_len;
> > >   u8 *contextV;
> > >   size_t contextV_len;
> > > 
> > > };
> > > 
> > > And the implementation of the generate function for CTR KDF would
> > > 
> > > then have a  type cast along the following lines:
> > >   if (slen != sizeof(struct crypto_kdf_ctr))
> > >   
> > >   return -EINVAL;
> > >   
> > >   const struct crypto_kdf_ctr *kdf_ctr_input = (struct
> > > 
> > > crypto_kdf_ctr *)src;
> > > 
> > > 
> > > For different KDFs, different structs would be needed.
> > 
> > Actually, we probably just need the input key (or secret material), the
> > concatenation and the number of output bits.
> 
> Thanks for confirming. Though, when it comes to HKDF (not that I see it being 
> needed or required in the kernel), there is a need to split up the src 
> pointer 
> since the mentioned input is used in different ways.
> 
> In order to try to get the implementation and thus the interface right, I 
> would suggest to at least have a consensus on how to handle such situations.
> 
> Thus, would the proposal be acceptable for such KDFs that may need to have 
> different components communicated as input to the KDF?
> 

FWIW, it's been very slow going since I've been working on other projects and I
also need to be very sure to get the API changes right, but I still plan to
change the KDF in fscrypt (a.k.a. ext4/f2fs/ubifs encryption) to HKDF-SHA512 as
part of a larger set of improvements to how fscrypt encryption keys are managed.
I sent the last patchset a year ago
(https://marc.info/?l=linux-fsdevel=150879493206257) but I'm working to revive
it.  In the work-in-progress version in my git tree, this is the commit that
adds a HKDF implementation as fs/crypto/hkdf.c:
https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git/commit/?id=e8a78767131c9717ee838f0c4e307948d65a4427
It basically just wraps a crypto_shash for "hmac(sha512)".

I'd be fine with using a common implementation instead, provided that it gives
the same functionality, including supporting user-specified salt and
application-specific info strings, and isn't slower or more complex to use.

(This comment is solely on the tangential discussion about KDF implementations;
I've not looked at the hibernation image encryption stuff yet.)

- Eric


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread Stephan Mueller
Am Mittwoch, 9. Januar 2019, 07:58:28 CET schrieb James Bottomley:

Hi James,

> On Wed, 2019-01-09 at 07:45 +0100, Stephan Mueller wrote:
> > Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:
> > 
> > Hi James,
> > 
> > > Actually, it would be enormously helpful if we could reuse these
> > > pieces for the TPM as well.
> > 
> > Could you please help me understand whether the KDFs in TPM are
> > directly usable as a standalone cipher primitive or does it go
> > together with additional  key generation operations?
> 
> They're used as generators ... which means they deterministically
> produce keys from what the TPM calls seeds so we can get crypto agility
> of TPM 2.0 ... well KDFa does.  KDFe is simply what NIST recommends you
> do when using EC for a shared key agreement ... and really we shouldn't
> be using ECDH in the kernel without it.
> 

Thanks for clarifying. That would mean that indeed we would have hardware-
provided KDF implementations that may be usable with the kernel crypto API.

[...]
> 
> > Would it be appropriate, to implement a type cast to a structure from
> > the u8 pointer?
> > 
> > E.g. for the aforementioned label/context data, we could define
> > something like
> > 
> > struct crypto_kdf_ctr {
> > 
> > char *label;
> > size_t label_len;
> > u8 *contextU;
> > size_t contextU_len;
> > u8 *contextV;
> > size_t contextV_len;
> > 
> > };
> > 
> > And the implementation of the generate function for CTR KDF would
> > 
> > then have a  type cast along the following lines:
> > if (slen != sizeof(struct crypto_kdf_ctr))
> > 
> > return -EINVAL;
> > 
> > const struct crypto_kdf_ctr *kdf_ctr_input = (struct
> > 
> > crypto_kdf_ctr *)src;
> > 
> > 
> > For different KDFs, different structs would be needed.
> 
> Actually, we probably just need the input key (or secret material), the
> concatenation and the number of output bits.

Thanks for confirming. Though, when it comes to HKDF (not that I see it being 
needed or required in the kernel), there is a need to split up the src pointer 
since the mentioned input is used in different ways.

In order to try to get the implementation and thus the interface right, I 
would suggest to at least have a consensus on how to handle such situations.

Thus, would the proposal be acceptable for such KDFs that may need to have 
different components communicated as input to the KDF?

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread James Bottomley
On Wed, 2019-01-09 at 07:45 +0100, Stephan Mueller wrote:
> Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:
> 
> Hi James,
> 
> > Actually, it would be enormously helpful if we could reuse these
> > pieces for the TPM as well. 
> 
> Could you please help me understand whether the KDFs in TPM are
> directly usable as a standalone cipher primitive or does it go
> together with additional  key generation operations?

They're used as generators ... which means they deterministically
produce keys from what the TPM calls seeds so we can get crypto agility
of TPM 2.0 ... well KDFa does.  KDFe is simply what NIST recommends you
do when using EC for a shared key agreement ... and really we shouldn't
be using ECDH in the kernel without it.

> > It has two KDFs: KDFa, which is the CTR-KDF from
> > SP800-108 and KDFe which is the SP800-56A KDF for elliptic curve
> > one pass Diffie Hellman, so if we're going to do the former, I'd
> > really like the latter as well.
> > 
> > The way the TPM parametrises input to both KDFs is
> > 
> > (hashAlg, key, label, contextU, contextV, bits)
> > 
> > Where
> > 
> > hashAlg  = the hash algorithm used as the PRF
> > key  = the input parameter of variable bit size or
> >the x co-ordinate of the shared point
> > label= An ASCII string representing the use
> > contextU = public input U
> > contextV = public input V
> > bits = number of output bits
> 
> When implementing KDFs as an extension of the kernel crypto API's RNG
> facility we currently have to handle the limitiation of the existing
> API. The label/context data (and when considering RFC 5869 HKDF
> requring IKM, salt and  additional information as input) currently
> cannot directly be communicated  through the API.
> 
> The issue is that the RNG facility currently has the following
> prototype defined:
> 
> int (*generate)(struct crypto_rng *tfm,
> const u8 *src, unsigned int slen,
> u8 *dst, unsigned int dlen);
> 
> The src pointer would need to take the label/context data.

That's probably good enough.  For both KDFa and KDFe the label contextU
and ContextV are concatenated, so in both cases a single source is
probably good enough.  However, we also need to feed in the key somehow
since it's usually used separately in the derivation functions.

> Would it be appropriate, to implement a type cast to a structure from
> the u8 pointer?
> 
> E.g. for the aforementioned label/context data, we could define
> something like
> 
> struct crypto_kdf_ctr {
>   char *label;
>   size_t label_len;
>   u8 *contextU;
>   size_t contextU_len;
>   u8 *contextV;
>   size_t contextV_len;
> };
> 
> And the implementation of the generate function for CTR KDF would
> then have a  type cast along the following lines:
> 
>   if (slen != sizeof(struct crypto_kdf_ctr))
>   return -EINVAL;
>   const struct crypto_kdf_ctr *kdf_ctr_input = (struct
> crypto_kdf_ctr *)src;
> 
> 
> For different KDFs, different structs would be needed.

Actually, we probably just need the input key (or secret material), the
concatenation and the number of output bits.

James



Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread James Bottomley
On Tue, 2019-01-08 at 17:43 -0800, Andy Lutomirski wrote:
> [Adding Jarkko because this stuff relates to the TPM.]
> 
> On Tue, Jan 8, 2019 at 4:44 PM James Bottomley
>  wrote:
> > 
> > On Tue, 2019-01-08 at 15:54 -0800, Andy Lutomirski wrote:
> > > > On Jan 7, 2019, at 11:09 PM, Stephan Mueller  > > > de>
> > > > wrote:
> > > > 
> > > > Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:
> > > > 
> > > > Hi Herbert,
> > > > 
> > > > > Are we going to have multiple implementations for the same
> > > > > KDF? If not then the crypto API is not a good fit.  To
> > > > > consolidate multiple implementations of the same KDF, simply
> > > > > provide helpers for them.
> > > > 
> > > > It is unlikely to have multiple implementations of a KDF.
> > > > However, KDFs relate to hashes like block chaining modes to raw
> > > > block ciphers. Thus a KDF can be applied with different hashes.
> > > > 
> > > > My idea was to add template support to RNGs (because KDFs are
> > > > effectively a type of RNG since they produce an arbitrary
> > > > output from a fixed input). The KDFs would be a template
> > > > wrapping hashes.  For example, the CTR-KDF from SP800-108 could
> > > > be instantiated like kdf-ctr(sha256).
> > > > 
> > > > 
> > > 
> > > I think that, if the crypto API is going to grow a KDF facility,
> > > it should be done right. Have a key type or flag or whatever that
> > > says “this key may *only* be used to derive keys using such-and-
> > > such algorithm”, and have a helper to derive a key.  That helper
> > > should take some useful parameters and mix them in:
> > > 
> > > - What type of key is being derived?  ECDSA signing key?  HMAC
> > > key?  AES key?
> > > 
> > > - Can user code access the derived key?
> > > 
> > > - What is the key’s purpose?  “Encrypt and authenticate a
> > > hibernation image” would be a purpose.
> > > 
> > > - Number of bytes.
> > > 
> > > All of these parameters should be mixed in to the key derivation.
> > > 
> > > Also, an AE key, even for AES+HMAC, should be just one derived
> > > key.  If you need 512 bits, ask for a 512-bit key, not two 256-
> > > bit keys.
> > 
> > Actually, it would be enormously helpful if we could reuse these
> > pieces for the TPM as well.  It has two KDFs: KDFa, which is the
> > CTR-KDF from SP800-108 and KDFe which is the SP800-56A KDF for
> > elliptic curve one pass Diffie Hellman, so if we're going to do the
> > former, I'd really like the latter as well.
> > 
> > The way the TPM parametrises input to both KDFs is
> > 
> > (hashAlg, key, label, contextU, contextV, bits)
> > 
> > Where
> > 
> > hashAlg  = the hash algorithm used as the PRF
> > key  = the input parameter of variable bit size or
> >the x co-ordinate of the shared point
> > label= An ASCII string representing the use
> > contextU = public input U
> > contextV = public input V
> > bits = number of output bits
> > 
> > Is that a good enough parametrisation (not the only way you
> > distinguish uses is with the label, which is not
> > recoverable)?  ContextU and ContextV are simply concatenated to
> > form the full Context of SP800-108, but we tend to need two
> > separate inputs (for KDFe they're the public x co-ordinates of the
> > points of the two inputs to ECDH for instance; in KDFa they're
> > usually the local and remote nonces).
> > 
> > The labels for TPM usage are things like "INTEGRITY" for HMAC keys
> > or "CFB" when generating an aes128-cfb session key. For KDFe, the
> > tpm seems to like the label "SECRET".  Although the TPM specifies
> > fixed short strings for the label, nothing prevents them being
> > longer like the purpose you state above (essentially we could mix
> > purpose, use and key type into the label and the contexts).
> > 
> 
> That really ought to cover anything the kernel needs.
> 
> But can you explain what's up with with KDFe?  The obvious searches
> end up with just warnings that the US currently has no government :(

You mean you can't find SP100-56A because NIST is a government entity
and it's discontinued its website because of the government shutdown? 
No idea, I only live here, you'll have to ask a real American.

ACM does have a copy:

http://delivery.acm.org/10.1145/221/2206270/SP800-56A_Revision1_Mar08-2007.pdf?ip=50.35.68.20=2206270=OPEN=4D4702B0C3E38B35%2E4D4702B0C3E38B35%2E4D4702B0C3E38B35%2E6D218144511F3437&__acm__=1546993111_ed9c8bd24b2f838c829d428aac7f5d71

> Anyway, if we're talking about the TPM, it seems like the entire
> "trusted key" mechanism in the kernel is missing the point.  If I
> want to encrypt something like a hibernation image on a machine with
> a TPM, it makes essentially no sense to me that we would get a key
> with a known raw value that is merely TPM-backed (i.e. the "trusted
> key") and use that to decrypt the image.  The right way to do it is
> to the use the TPM as it was intended to be used: generate a single-
> use key that protects the hibernation image and seal *that* directly
> 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread Stephan Mueller
Am Mittwoch, 9. Januar 2019, 01:44:31 CET schrieb James Bottomley:

Hi James,

> Actually, it would be enormously helpful if we could reuse these pieces
> for the TPM as well. 

Could you please help me understand whether the KDFs in TPM are directly 
usable as a standalone cipher primitive or does it go together with additional 
key generation operations?

> It has two KDFs: KDFa, which is the CTR-KDF from
> SP800-108 and KDFe which is the SP800-56A KDF for elliptic curve one
> pass Diffie Hellman, so if we're going to do the former, I'd really
> like the latter as well.
> 
> The way the TPM parametrises input to both KDFs is
> 
> (hashAlg, key, label, contextU, contextV, bits)
> 
> Where
> 
> hashAlg  = the hash algorithm used as the PRF
> key  = the input parameter of variable bit size or
>the x co-ordinate of the shared point
> label= An ASCII string representing the use
> contextU = public input U
> contextV = public input V
> bits = number of output bits

When implementing KDFs as an extension of the kernel crypto API's RNG facility 
we currently have to handle the limitiation of the existing API. The label/
context data (and when considering RFC 5869 HKDF requring IKM, salt and 
additional information as input) currently cannot directly be communicated 
through the API.

The issue is that the RNG facility currently has the following prototype 
defined:

int (*generate)(struct crypto_rng *tfm,
const u8 *src, unsigned int slen,
u8 *dst, unsigned int dlen);

The src pointer would need to take the label/context data.

Would it be appropriate, to implement a type cast to a structure from the u8 
pointer?

E.g. for the aforementioned label/context data, we could define something like

struct crypto_kdf_ctr {
char *label;
size_t label_len;
u8 *contextU;
size_t contextU_len;
u8 *contextV;
size_t contextV_len;
};

And the implementation of the generate function for CTR KDF would then have a 
type cast along the following lines:

if (slen != sizeof(struct crypto_kdf_ctr))
return -EINVAL;
const struct crypto_kdf_ctr *kdf_ctr_input = (struct crypto_kdf_ctr 
*)src;


For different KDFs, different structs would be needed.

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread Stephan Mueller
Am Mittwoch, 9. Januar 2019, 00:54:22 CET schrieb Andy Lutomirski:

Hi Andy,
> 
> I think that, if the crypto API is going to grow a KDF facility, it should
> be done right. Have a key type or flag or whatever that says “this key may
> *only* be used to derive keys using such-and-such algorithm”, and have a
> helper to derive a key.  That helper should take some useful parameters and
> mix them in:
> 
> - What type of key is being derived?  ECDSA signing key?  HMAC key?  AES
> key?
> 
> - Can user code access the derived key?
> 
> - What is the key’s purpose?  “Encrypt and authenticate a hibernation image”
> would be a purpose.
> 
> - Number of bytes.
> 
> All of these parameters should be mixed in to the key derivation.
> 
> Also, an AE key, even for AES+HMAC, should be just one derived key.  If you
> need 512 bits, ask for a 512-bit key, not two 256-bit keys.

I concur with your requirements. However, is the kernel crypto API the right 
place to enforce such policies? To me, the kernel crypto API is a tinker-toy 
set of ciphers.

The real policy enforcer would or should be the keyring facility. Thus, may I 
propose to:

- implement the cryptographic primitive of the KDF in the kernel crypto API

- implement the policy system how to use the KDF in the keyring facility

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread Andy Lutomirski
[Adding Jarkko because this stuff relates to the TPM.]

On Tue, Jan 8, 2019 at 4:44 PM James Bottomley
 wrote:
>
> On Tue, 2019-01-08 at 15:54 -0800, Andy Lutomirski wrote:
> > > On Jan 7, 2019, at 11:09 PM, Stephan Mueller 
> > > wrote:
> > >
> > > Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:
> > >
> > > Hi Herbert,
> > >
> > > > Are we going to have multiple implementations for the same KDF?
> > > > If not then the crypto API is not a good fit.  To consolidate
> > > > multiple implementations of the same KDF, simply provide helpers
> > > > for them.
> > >
> > > It is unlikely to have multiple implementations of a KDF. However,
> > > KDFs relate to hashes like block chaining modes to raw block
> > > ciphers. Thus a KDF can be applied with different hashes.
> > >
> > > My idea was to add template support to RNGs (because KDFs are
> > > effectively a type of RNG since they produce an arbitrary output
> > > from a fixed input). The KDFs would be a template wrapping hashes.
> > > For example, the CTR-KDF from SP800-108 could be instantiated like
> > > kdf-ctr(sha256).
> > >
> > >
> >
> > I think that, if the crypto API is going to grow a KDF facility, it
> > should be done right. Have a key type or flag or whatever that says
> > “this key may *only* be used to derive keys using such-and-such
> > algorithm”, and have a helper to derive a key.  That helper should
> > take some useful parameters and mix them in:
> >
> > - What type of key is being derived?  ECDSA signing key?  HMAC
> > key?  AES key?
> >
> > - Can user code access the derived key?
> >
> > - What is the key’s purpose?  “Encrypt and authenticate a hibernation
> > image” would be a purpose.
> >
> > - Number of bytes.
> >
> > All of these parameters should be mixed in to the key derivation.
> >
> > Also, an AE key, even for AES+HMAC, should be just one derived
> > key.  If you need 512 bits, ask for a 512-bit key, not two 256-bit
> > keys.
>
> Actually, it would be enormously helpful if we could reuse these pieces
> for the TPM as well.  It has two KDFs: KDFa, which is the CTR-KDF from
> SP800-108 and KDFe which is the SP800-56A KDF for elliptic curve one
> pass Diffie Hellman, so if we're going to do the former, I'd really
> like the latter as well.
>
> The way the TPM parametrises input to both KDFs is
>
> (hashAlg, key, label, contextU, contextV, bits)
>
> Where
>
> hashAlg  = the hash algorithm used as the PRF
> key  = the input parameter of variable bit size or
>the x co-ordinate of the shared point
> label= An ASCII string representing the use
> contextU = public input U
> contextV = public input V
> bits = number of output bits
>
> Is that a good enough parametrisation (not the only way you distinguish
> uses is with the label, which is not recoverable)?  ContextU and
> ContextV are simply concatenated to form the full Context of SP800-108,
> but we tend to need two separate inputs (for KDFe they're the public x
> co-ordinates of the points of the two inputs to ECDH for instance; in
> KDFa they're usually the local and remote nonces).
>
> The labels for TPM usage are things like "INTEGRITY" for HMAC keys or
> "CFB" when generating an aes128-cfb session key. For KDFe, the tpm
> seems to like the label "SECRET".  Although the TPM specifies fixed
> short strings for the label, nothing prevents them being longer like
> the purpose you state above (essentially we could mix purpose, use and
> key type into the label and the contexts).
>

That really ought to cover anything the kernel needs.

But can you explain what's up with with KDFe?  The obvious searches
end up with just warnings that the US currently has no government :(

Anyway, if we're talking about the TPM, it seems like the entire
"trusted key" mechanism in the kernel is missing the point.  If I want
to encrypt something like a hibernation image on a machine with a TPM,
it makes essentially no sense to me that we would get a key with a
known raw value that is merely TPM-backed (i.e. the "trusted key") and
use that to decrypt the image.  The right way to do it is to the use
the TPM as it was intended to be used: generate a single-use key that
protects the hibernation image and seal *that* directly on the TPM,
such that it can only be unsealed with appropriate PCR values.  Heck,
we could even use one of the fancy NV counters such that we *can't*
decrypt the image later on.  And using HMAC or any AE construction the
normal way is also wrong -- we should *hash* the image and sign the
hash directly on the TPM so that the restore code can validate the PCR
values that were in place when the hibernation image was created.  [0]

In other words, I think that a kernel-based encrypted hibernation
mechanism should create an image like this:

- wrapped key
- instructions, if needed, for unwrapping
- hash of the entire image except the hash and signature fields
- signature of the hash

and the remainder is a regular hiberation image that is encrypted
against the 

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread James Bottomley
On Tue, 2019-01-08 at 15:54 -0800, Andy Lutomirski wrote:
> > On Jan 7, 2019, at 11:09 PM, Stephan Mueller 
> > wrote:
> > 
> > Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:
> > 
> > Hi Herbert,
> > 
> > > Are we going to have multiple implementations for the same KDF?
> > > If not then the crypto API is not a good fit.  To consolidate
> > > multiple implementations of the same KDF, simply provide helpers
> > > for them.
> > 
> > It is unlikely to have multiple implementations of a KDF. However,
> > KDFs relate to hashes like block chaining modes to raw block
> > ciphers. Thus a KDF can be applied with different hashes.
> > 
> > My idea was to add template support to RNGs (because KDFs are
> > effectively a type of RNG since they produce an arbitrary output
> > from a fixed input). The KDFs would be a template wrapping hashes.
> > For example, the CTR-KDF from SP800-108 could be instantiated like
> > kdf-ctr(sha256).
> > 
> > 
> 
> I think that, if the crypto API is going to grow a KDF facility, it
> should be done right. Have a key type or flag or whatever that says
> “this key may *only* be used to derive keys using such-and-such
> algorithm”, and have a helper to derive a key.  That helper should
> take some useful parameters and mix them in:
> 
> - What type of key is being derived?  ECDSA signing key?  HMAC
> key?  AES key?
> 
> - Can user code access the derived key?
> 
> - What is the key’s purpose?  “Encrypt and authenticate a hibernation
> image” would be a purpose.
> 
> - Number of bytes.
> 
> All of these parameters should be mixed in to the key derivation.
> 
> Also, an AE key, even for AES+HMAC, should be just one derived
> key.  If you need 512 bits, ask for a 512-bit key, not two 256-bit
> keys.

Actually, it would be enormously helpful if we could reuse these pieces
for the TPM as well.  It has two KDFs: KDFa, which is the CTR-KDF from
SP800-108 and KDFe which is the SP800-56A KDF for elliptic curve one
pass Diffie Hellman, so if we're going to do the former, I'd really
like the latter as well.

The way the TPM parametrises input to both KDFs is

(hashAlg, key, label, contextU, contextV, bits)

Where

hashAlg  = the hash algorithm used as the PRF
key  = the input parameter of variable bit size or
   the x co-ordinate of the shared point
label= An ASCII string representing the use
contextU = public input U
contextV = public input V
bits = number of output bits

Is that a good enough parametrisation (not the only way you distinguish
uses is with the label, which is not recoverable)?  ContextU and
ContextV are simply concatenated to form the full Context of SP800-108, 
but we tend to need two separate inputs (for KDFe they're the public x
co-ordinates of the points of the two inputs to ECDH for instance; in
KDFa they're usually the local and remote nonces).

The labels for TPM usage are things like "INTEGRITY" for HMAC keys or
"CFB" when generating an aes128-cfb session key. For KDFe, the tpm
seems to like the label "SECRET".  Although the TPM specifies fixed
short strings for the label, nothing prevents them being longer like
the purpose you state above (essentially we could mix purpose, use and
key type into the label and the contexts).

>From the point of view of accelerators, the only thing you really need
to know is the hash algorthim (PRF), because everything else above is
an input to the function, so I suppose it makes sense to name them as
kdf-X(PRF)  where X would be ctr or ecdh and PRF would be a hash.

James



Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-08 Thread Andy Lutomirski


> On Jan 7, 2019, at 11:09 PM, Stephan Mueller  wrote:
> 
> Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:
> 
> Hi Herbert,
> 
>> Are we going to have multiple implementations for the same KDF?
>> If not then the crypto API is not a good fit.  To consolidate
>> multiple implementations of the same KDF, simply provide helpers
>> for them.
> 
> It is unlikely to have multiple implementations of a KDF. However, KDFs 
> relate 
> to hashes like block chaining modes to raw block ciphers. Thus a KDF can be 
> applied with different hashes.
> 
> My idea was to add template support to RNGs (because KDFs are effectively a 
> type of RNG since they produce an arbitrary output from a fixed input). The 
> KDFs would be a template wrapping hashes. For example, the CTR-KDF from 
> SP800-108 could be instantiated like kdf-ctr(sha256).
> 
> 

I think that, if the crypto API is going to grow a KDF facility, it should be 
done right. Have a key type or flag or whatever that says “this key may *only* 
be used to derive keys using such-and-such algorithm”, and have a helper to 
derive a key.  That helper should take some useful parameters and mix them in:

- What type of key is being derived?  ECDSA signing key?  HMAC key?  AES key?

- Can user code access the derived key?

- What is the key’s purpose?  “Encrypt and authenticate a hibernation image” 
would be a purpose.

- Number of bytes.

All of these parameters should be mixed in to the key derivation.

Also, an AE key, even for AES+HMAC, should be just one derived key.  If you 
need 512 bits, ask for a 512-bit key, not two 256-bit keys.

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-07 Thread Stephan Mueller
Am Dienstag, 8. Januar 2019, 06:03:58 CET schrieb Herbert Xu:

Hi Herbert,

> Are we going to have multiple implementations for the same KDF?
> If not then the crypto API is not a good fit.  To consolidate
> multiple implementations of the same KDF, simply provide helpers
> for them.

It is unlikely to have multiple implementations of a KDF. However, KDFs relate 
to hashes like block chaining modes to raw block ciphers. Thus a KDF can be 
applied with different hashes.

My idea was to add template support to RNGs (because KDFs are effectively a 
type of RNG since they produce an arbitrary output from a fixed input). The 
KDFs would be a template wrapping hashes. For example, the CTR-KDF from 
SP800-108 could be instantiated like kdf-ctr(sha256).

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-07 Thread Herbert Xu
On Mon, Jan 07, 2019 at 04:52:00PM +0100, Stephan Mueller wrote:
>
> Would it make sense to polish these mentioned KDF patches and add them to the 
> kernel crypto API? The sprawl of key derivation logic here and there which 
> seemingly does not comply to any standard and thus possibly have issues 
> should 
> be prevented and cleaned up.

Are we going to have multiple implementations for the same KDF?
If not then the crypto API is not a good fit.  To consolidate
multiple implementations of the same KDF, simply provide helpers
for them.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-07 Thread Stephan Mueller
Am Montag, 7. Januar 2019, 16:33:27 CET schrieb joeyli:

Hi Herbert,

> 
> > use an official KDF type like SP800-108 or HKDF?
> > 
> > You find the counter-KDF according to SP800-108 in security/keys/dh.c
> > (search for functions *kdf*).
> > 
> > Or we may start pulling in KDF support into the kernel crypto API via the
> > patches along the line of [1].
> > 
> > [1] http://www.chronox.de/kdf.html
> 
> Thanks for your suggestion. I didn't touch any key derivation standard
> before. I will study it.
> 
> But I still want to use my original function currently. Because the same
> logic is also used in trusted key. I will happy to move to SP800-108 or
> HKDF when it's available in kernel.

Would it make sense to polish these mentioned KDF patches and add them to the 
kernel crypto API? The sprawl of key derivation logic here and there which 
seemingly does not comply to any standard and thus possibly have issues should 
be prevented and cleaned up.

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-07 Thread joeyli
Hi Stephan, 

First, thanks for your review!

On Sun, Jan 06, 2019 at 09:01:27AM +0100, Stephan Mueller wrote:
> Am Donnerstag, 3. Januar 2019, 15:32:23 CET schrieb Lee, Chun-Yi:
> 
> Hi Chun,
> 
> > This patch adds a snapshot keys handler for using the key retention
> > service api to create keys for snapshot image encryption and
> > authentication.
> > 
> > This handler uses TPM trusted key as the snapshot master key, and the
> > encryption key and authentication key are derived from the snapshot
> > key. The user defined key can also be used as the snapshot master key
> > , but user must be aware that the security of user key relies on user
> > space.
> > 
[...snip]
> > +static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen,
> > +bool may_sleep)
> > +{
> > +   struct shash_desc *desc;
> > +   int err;
> > +
> > +   desc = kzalloc(sizeof(struct shash_desc) +
> > +  crypto_shash_descsize(hash_tfm),
> > +  may_sleep ? GFP_KERNEL : GFP_ATOMIC);
> 
> Why not using SHASH_DESC_ON_STACK?
>

Because security concern and bad runtime performance. Please looking at
c2cd0b08e1e patch for hibernation. And reference:

https://lore.kernel.org/lkml/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com/T/#u
https://lwn.net/Articles/749064/
 
> > +   if (!desc)
> > +   return -ENOMEM;
> > +
> > +   desc->tfm = hash_tfm;
> > +   desc->flags = may_sleep ? CRYPTO_TFM_REQ_MAY_SLEEP : 0;
> > +   err = crypto_shash_digest(desc, buf, buflen, digest);
> > +   shash_desc_zero(desc);
> > +   kzfree(desc);
> > +
> > +   return err;
> > +}
> > +
> > +static int calc_key_hash(u8 *key, unsigned int key_len, const char *salt,
> > +u8 *hash, bool may_sleep)
> > +{
> > +   unsigned int salted_buf_len;
> > +   u8 *salted_buf;
> > +   int ret;
> > +
> > +   if (!key || !hash_tfm || !hash)
> > +   return -EINVAL;
> > +
> > +   salted_buf_len = strlen(salt) + 1 + SNAPSHOT_KEY_SIZE;
> 
> strlen on binary data? I guess that will not work. May I suggest to hand down 
> the length of salt to this function?
>

hm... The salt is actually a "salt string" that's gave from
snapshot_get_auth_key() or snapshot_get_enc_key(). So I use
strlen() here. I will change the name to salt_string to avoid
confusion. 
 
> > +   salted_buf = kzalloc(salted_buf_len,
> > +   may_sleep ? GFP_KERNEL : GFP_ATOMIC);
> > +   if (!salted_buf)
> > +   return -ENOMEM;
> > +
> > +   strcpy(salted_buf, salt);
> > +   memcpy(salted_buf + strlen(salted_buf) + 1, key, key_len);
> > +
> > +   ret = calc_hash(hash, salted_buf, salted_buf_len, may_sleep);
> > +   memzero_explicit(salted_buf, salted_buf_len);
> > +   kzfree(salted_buf);
> > +
> > +   return ret;
> > +}
> 
> This function looks very much like a key derivation. May I strongly propose 
> to 

Actually key derivation function is modified from the get_derived_key() from
the encrypted.c file in encrypted key.

> use an official KDF type like SP800-108 or HKDF?
> 
> You find the counter-KDF according to SP800-108 in security/keys/dh.c (search 
> for functions *kdf*).
> 
> Or we may start pulling in KDF support into the kernel crypto API via the 
> patches along the line of [1].
> 
> [1] http://www.chronox.de/kdf.html
>

Thanks for your suggestion. I didn't touch any key derivation standard
before. I will study it.

But I still want to use my original function currently. Because the same
logic is also used in trusted key. I will happy to move to SP800-108 or
HKDF when it's available in kernel.  

> > +
> > +/* Derive authentication/encryption key */
> > +static int get_derived_key(u8 *derived_key, const char *derived_type_str,
> > +  bool may_sleep)
[...snip]
> > +static int trusted_key_init(void)
> > +{
> > +   struct trusted_key_payload *tkp;
> > +   struct key *key;
> > +   int err = 0;
> > +
> > +   pr_debug("%s\n", __func__);
> > +
> > +   /* find out swsusp-key */
> > +   key = request_key(_type_trusted, skey.key_name, NULL);
> > +   if (IS_ERR(key)) {
> > +   pr_err("Request key error: %ld\n", PTR_ERR(key));
> > +   err = PTR_ERR(key);
> > +   return err;
> > +   }
> > +
> > +   down_write(>sem);
> > +   tkp = key->payload.data[0];
> > +   if (invalid_key(tkp->key, tkp->key_len)) {
> > +   err = -EINVAL;
> > +   goto key_invalid;
> > +   }
> > +   skey.key_len = tkp->key_len;
> > +   memcpy(skey.key, tkp->key, tkp->key_len);
> > +   /* burn the original key contents */
> > +   memzero_explicit(tkp->key, tkp->key_len);
> > +
> > +key_invalid:
> > +   up_write(>sem);
> > +   key_put(key);
> > +
> > +   return err;
> > +}
> > +
> > +static int user_key_init(void)
> 
> This function and trusted_key_init look very similar - could they be 
> collapsed 
> into one function?
>

The data structure is different between trusted key with user key. I will try to
extract the duplicate part but may not collapse into one.
 
> > +{
> > +   

Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-06 Thread Stephan Mueller
Am Sonntag, 6. Januar 2019, 09:01:27 CET schrieb Stephan Mueller:

Hi,

> > +   memcpy(skey.key, ukp->data, ukp->datalen);
> 
> Where would skey.key be destroyed again?

Now I see it - it is in patch 4. Please disregard my comment.

Ciao
Stephan




Re: [PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-06 Thread Stephan Mueller
Am Donnerstag, 3. Januar 2019, 15:32:23 CET schrieb Lee, Chun-Yi:

Hi Chun,

> This patch adds a snapshot keys handler for using the key retention
> service api to create keys for snapshot image encryption and
> authentication.
> 
> This handler uses TPM trusted key as the snapshot master key, and the
> encryption key and authentication key are derived from the snapshot
> key. The user defined key can also be used as the snapshot master key
> , but user must be aware that the security of user key relies on user
> space.
> 
> The name of snapshot key is fixed to "swsusp-kmk". User should use
> the keyctl tool to load the key blob to root's user keyring. e.g.
> 
>  # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u
> 
> or create a new user key. e.g.
> 
>  # /bin/keyctl add user swsusp-kmk password @u
> 
> Then the disk_kmk sysfs file can be used to trigger the initialization
> of snapshot key:
> 
>  # echo 1 > /sys/power/disk_kmk
> 
> After the initialization be triggered, the secret in the payload of
> swsusp-key will be copied by hibernation and be erased. Then user can
> use keyctl to remove swsusp-kmk key from root's keyring.
> 
> If user does not trigger the initialization by disk_kmk file after
> swsusp-kmk be loaded to kernel. Then the snapshot key will be
> initialled when hibernation be triggered.
> 
> v2:
> - Fixed bug of trusted_key_init's return value.
> - Fixed wording in Kconfig
> - Removed VLA usage
> - Removed the checking of capability for writing disk_kmk.
> - Fixed Kconfig, select trusted key.
> - Add memory barrier before setting key initialized flag.
> 
> Cc: "Rafael J. Wysocki" 
> Cc: Pavel Machek 
> Cc: Chen Yu 
> Cc: Oliver Neukum 
> Cc: Ryan Chen 
> Cc: David Howells 
> Cc: Giovanni Gherdovich 
> Cc: Randy Dunlap 
> Cc: Jann Horn 
> Cc: Andy Lutomirski 
> Signed-off-by: "Lee, Chun-Yi" 
> ---
>  kernel/power/Kconfig|  14 +++
>  kernel/power/Makefile   |   1 +
>  kernel/power/hibernate.c|  33 ++
>  kernel/power/power.h|  16 +++
>  kernel/power/snapshot_key.c | 245
>  5 files changed, 309
> insertions(+)
>  create mode 100644 kernel/power/snapshot_key.c
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index f8fe57d1022e..506a3c5a7a0d 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -76,6 +76,20 @@ config HIBERNATION
> 
> For more information take a look at
> .
> 
> +config HIBERNATION_ENC_AUTH
> + bool "Hibernation encryption and authentication"
> + depends on HIBERNATION
> + select TRUSTED_KEYS
> + select CRYPTO_AES
> + select CRYPTO_HMAC
> + select CRYPTO_SHA512
> + help
> +   This option will encrypt and authenticate the memory snapshot image
> +   of hibernation. It prevents that the snapshot image be arbitrarily
> +   modified. A user can use the TPM's trusted key or user defined key
> +   as the master key of hibernation. The TPM trusted key depends on TPM.
> +   The security of user defined key relies on user space.
> +
>  config ARCH_SAVE_PAGE_KEYS
>   bool
> 
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index e7e47d9be1e5..d949adbaf580 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_FREEZER)   += process.o
>  obj-$(CONFIG_SUSPEND)+= suspend.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)+= hibernate.o snapshot.o swap.o user.o
> +obj-$(CONFIG_HIBERNATION_ENC_AUTH)   += snapshot_key.o
>  obj-$(CONFIG_PM_AUTOSLEEP)   += autosleep.o
>  obj-$(CONFIG_PM_WAKELOCKS)   += wakelock.o
> 
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index abef759de7c8..ecc31e8e40d0 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -1034,6 +1034,36 @@ static ssize_t disk_store(struct kobject *kobj,
> struct kobj_attribute *attr,
> 
>  power_attr(disk);
> 
> +#ifdef CONFIG_HIBERNATION_ENC_AUTH
> +static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute
> *attr, +   char *buf)
> +{
> + if (snapshot_key_initialized())
> + return sprintf(buf, "initialized\n");
> + else
> + return sprintf(buf, "uninitialized\n");
> +}
> +
> +static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute
> *attr, +const char *buf, size_t n)
> +{
> + int error = 0;
> + char *p;
> + int len;
> +
> + p = memchr(buf, '\n', n);
> + len = p ? p - buf : n;
> + if (strncmp(buf, "1", len))
> + return -EINVAL;
> +
> + error = snapshot_key_init();
> +
> + return error ? error : n;
> +}
> +
> +power_attr(disk_kmk);
> +#endif /* !CONFIG_HIBERNATION_ENC_AUTH */
> +
>  static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute
> *attr, char *buf)
>  {
> @@ -1138,6 +1168,9 @@ power_attr(reserved_size);
> 

[PATCH 1/5 v2] PM / hibernate: Create snapshot keys handler

2019-01-03 Thread Lee, Chun-Yi
This patch adds a snapshot keys handler for using the key retention
service api to create keys for snapshot image encryption and
authentication.

This handler uses TPM trusted key as the snapshot master key, and the
encryption key and authentication key are derived from the snapshot
key. The user defined key can also be used as the snapshot master key
, but user must be aware that the security of user key relies on user
space.

The name of snapshot key is fixed to "swsusp-kmk". User should use
the keyctl tool to load the key blob to root's user keyring. e.g.

 # /bin/keyctl add trusted swsusp-kmk "load `cat swsusp-kmk.blob`" @u

or create a new user key. e.g.

 # /bin/keyctl add user swsusp-kmk password @u

Then the disk_kmk sysfs file can be used to trigger the initialization
of snapshot key:

 # echo 1 > /sys/power/disk_kmk

After the initialization be triggered, the secret in the payload of
swsusp-key will be copied by hibernation and be erased. Then user can
use keyctl to remove swsusp-kmk key from root's keyring.

If user does not trigger the initialization by disk_kmk file after
swsusp-kmk be loaded to kernel. Then the snapshot key will be
initialled when hibernation be triggered.

v2:
- Fixed bug of trusted_key_init's return value.
- Fixed wording in Kconfig
- Removed VLA usage
- Removed the checking of capability for writing disk_kmk.
- Fixed Kconfig, select trusted key.
- Add memory barrier before setting key initialized flag.

Cc: "Rafael J. Wysocki" 
Cc: Pavel Machek 
Cc: Chen Yu 
Cc: Oliver Neukum 
Cc: Ryan Chen 
Cc: David Howells 
Cc: Giovanni Gherdovich 
Cc: Randy Dunlap 
Cc: Jann Horn 
Cc: Andy Lutomirski 
Signed-off-by: "Lee, Chun-Yi" 
---
 kernel/power/Kconfig|  14 +++
 kernel/power/Makefile   |   1 +
 kernel/power/hibernate.c|  33 ++
 kernel/power/power.h|  16 +++
 kernel/power/snapshot_key.c | 245 
 5 files changed, 309 insertions(+)
 create mode 100644 kernel/power/snapshot_key.c

diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index f8fe57d1022e..506a3c5a7a0d 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -76,6 +76,20 @@ config HIBERNATION
 
  For more information take a look at 
.
 
+config HIBERNATION_ENC_AUTH
+   bool "Hibernation encryption and authentication"
+   depends on HIBERNATION
+   select TRUSTED_KEYS
+   select CRYPTO_AES
+   select CRYPTO_HMAC
+   select CRYPTO_SHA512
+   help
+ This option will encrypt and authenticate the memory snapshot image
+ of hibernation. It prevents that the snapshot image be arbitrarily
+ modified. A user can use the TPM's trusted key or user defined key
+ as the master key of hibernation. The TPM trusted key depends on TPM.
+ The security of user defined key relies on user space.
+
 config ARCH_SAVE_PAGE_KEYS
bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index e7e47d9be1e5..d949adbaf580 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_FREEZER) += process.o
 obj-$(CONFIG_SUSPEND)  += suspend.o
 obj-$(CONFIG_PM_TEST_SUSPEND)  += suspend_test.o
 obj-$(CONFIG_HIBERNATION)  += hibernate.o snapshot.o swap.o user.o
+obj-$(CONFIG_HIBERNATION_ENC_AUTH) += snapshot_key.o
 obj-$(CONFIG_PM_AUTOSLEEP) += autosleep.o
 obj-$(CONFIG_PM_WAKELOCKS) += wakelock.o
 
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index abef759de7c8..ecc31e8e40d0 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -1034,6 +1034,36 @@ static ssize_t disk_store(struct kobject *kobj, struct 
kobj_attribute *attr,
 
 power_attr(disk);
 
+#ifdef CONFIG_HIBERNATION_ENC_AUTH
+static ssize_t disk_kmk_show(struct kobject *kobj, struct kobj_attribute *attr,
+char *buf)
+{
+   if (snapshot_key_initialized())
+   return sprintf(buf, "initialized\n");
+   else
+   return sprintf(buf, "uninitialized\n");
+}
+
+static ssize_t disk_kmk_store(struct kobject *kobj, struct kobj_attribute 
*attr,
+ const char *buf, size_t n)
+{
+   int error = 0;
+   char *p;
+   int len;
+
+   p = memchr(buf, '\n', n);
+   len = p ? p - buf : n;
+   if (strncmp(buf, "1", len))
+   return -EINVAL;
+
+   error = snapshot_key_init();
+
+   return error ? error : n;
+}
+
+power_attr(disk_kmk);
+#endif /* !CONFIG_HIBERNATION_ENC_AUTH */
+
 static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr,
   char *buf)
 {
@@ -1138,6 +1168,9 @@ power_attr(reserved_size);
 
 static struct attribute * g[] = {
_attr.attr,
+#ifdef CONFIG_HIBERNATION_ENC_AUTH
+   _kmk_attr.attr,
+#endif
_offset_attr.attr,
_attr.attr,
_size_attr.attr,
diff --git a/kernel/power/power.h b/kernel/power/power.h
index