[RFC 0/2] add integrity and security to TPM2 transactions

[RFC 0/2] add integrity and security to TPM2 transactions

[James Bottomley]

By now, everybody knows we have a problem with the TPM2_RS_PW easy
button on TPM2 in that transactions on the TPM bus can be intercepted
and altered.  The way to fix this is to use real sessions for HMAC
capabilities to ensure integrity and to use parameter and response
encryption to ensure confidentiality of the data flowing over the TPM
bus.

This RFC is about adding a simple API which can ensure the above
properties as a layered addition to the existing TPM handling code.
 Eventually we can add this to the random number generator, the PCR
extensions and the trusted key handling, but this all depends on the
conversion to tpm_buf which is not yet upstream, so I've constructed a
second patch which demonstrates the new API in a test module for those
who wish to play with it.

This series is also dependent on additions to the crypto subsystem to
fix problems in the elliptic curve key handling and add the Cipher
FeedBack encryption scheme:

https://marc.info/?l=linux-crypto-vger&m=151994371015475

---

James Bottomley (2):
  tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
  tpm2-sessions: NOT FOR COMMITTING add sessions testing

 drivers/char/tpm/Kconfig              |   3 +
 drivers/char/tpm/Makefile             |   3 +-
 drivers/char/tpm/tpm-chip.c           |   1 +
 drivers/char/tpm/tpm.h                |  22 +
 drivers/char/tpm/tpm2-cmd.c           |  22 +-
 drivers/char/tpm/tpm2-sessions-test.c | 178 +++++++
 drivers/char/tpm/tpm2-sessions.c      | 907 ++++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm2-sessions.h      |  55 +++
 drivers/char/tpm/tpm2b.h              |  82 +++
 9 files changed, 1260 insertions(+), 13 deletions(-)
 create mode 100644 drivers/char/tpm/tpm2-sessions-test.c
 create mode 100644 drivers/char/tpm/tpm2-sessions.c
 create mode 100644 drivers/char/tpm/tpm2-sessions.h
 create mode 100644 drivers/char/tpm/tpm2b.h

-- 
2.12.3

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> new file mode 100644
> index 000000000000..c7726f2895aa
> --- /dev/null
> +++ b/drivers/char/tpm/tpm2b.h
> @@ -0,0 +1,82 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _TPM2_TPM2B_H
> +#define _TPM2_TPM2B_H
> +/*
> + * Handing for tpm2b structures to facilitate the building of commands
> + */
> +
> +#include "tpm.h"
> +
> +#include <asm/unaligned.h>
> +
> +struct tpm2b {
> +	struct tpm_buf buf;
> +};
> +
> +/* opaque structure, holds auth session parameters like the session key */
> +struct tpm2_auth;
> +
> +static inline int tpm2b_init(struct tpm2b *buf)
> +{
> +	return tpm_buf_init(&buf->buf, 0, 0);
> +}
> +
> +static inline void tpm2b_reset(struct tpm2b *buf)
> +{
> +	struct tpm_input_header *head;
> +
> +	head = (struct tpm_input_header *)buf->buf.data;
> +	head->length = cpu_to_be32(sizeof(*head));
> +}
> +
> +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char *data,
> +				unsigned int len)
> +{
> +	tpm_buf_append(&buf->buf, data, len);
> +}
> +
> +#define TPM2B_APPEND(type) \
> +	static inline void tpm2b_append_##type(struct tpm2b *buf, const type value) { tpm_buf_append_##type(&buf->buf, value); }
> +
> +TPM2B_APPEND(u8)
> +TPM2B_APPEND(u16)
> +TPM2B_APPEND(u32)
> +
> +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> +{
> +	return buf->buf.data + sizeof(struct tpm_input_header);
> +}
> +
> +static inline u16 tpm2b_len(struct tpm2b *buf)
> +{
> +	return tpm_buf_length(&buf->buf) - sizeof(struct tpm_input_header);
> +}
> +
> +static inline void tpm2b_destroy(struct tpm2b *buf)
> +{
> +	tpm_buf_destroy(&buf->buf);
> +}
> +
> +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b *tpm2b)
> +{
> +	u16 len = tpm2b_len(tpm2b);
> +
> +	tpm_buf_append_u16(buf, len);
> +	tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> +	/* clear the buf for reuse */
> +	tpm2b_reset(tpm2b);
> +}
> +
> +/* Macros for unmarshalling known size BE data */
> +#define GET_INC(type)					\
> +static inline u##type get_inc_##type(const u8 **ptr) {	\
> +	u##type val;					\
> +	val = get_unaligned_be##type(*ptr);		\
> +	*ptr += sizeof(val);				\
> +	return val;					\
> +}
> +
> +GET_INC(16)
> +GET_INC(32)
> +
> +#endif
> -- 
> 2.12.3
> 

Some meta stuff:

* Add me to TO-field because I should probably review and eventually
  test these, right?
* CC to linux-security-module
* Why there is no RFC tag given that these are so quite large changes?
* Why in hell tpm2b.h?

/Jarkko

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 01:35:33PM +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> > new file mode 100644
> > index 000000000000..c7726f2895aa
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm2b.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TPM2_TPM2B_H
> > +#define _TPM2_TPM2B_H
> > +/*
> > + * Handing for tpm2b structures to facilitate the building of commands
> > + */
> > +
> > +#include "tpm.h"
> > +
> > +#include <asm/unaligned.h>
> > +
> > +struct tpm2b {
> > +	struct tpm_buf buf;
> > +};
> > +
> > +/* opaque structure, holds auth session parameters like the session key */
> > +struct tpm2_auth;
> > +
> > +static inline int tpm2b_init(struct tpm2b *buf)
> > +{
> > +	return tpm_buf_init(&buf->buf, 0, 0);
> > +}
> > +
> > +static inline void tpm2b_reset(struct tpm2b *buf)
> > +{
> > +	struct tpm_input_header *head;
> > +
> > +	head = (struct tpm_input_header *)buf->buf.data;
> > +	head->length = cpu_to_be32(sizeof(*head));
> > +}
> > +
> > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char *data,
> > +				unsigned int len)
> > +{
> > +	tpm_buf_append(&buf->buf, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > +	static inline void tpm2b_append_##type(struct tpm2b *buf, const type value) { tpm_buf_append_##type(&buf->buf, value); }
> > +
> > +TPM2B_APPEND(u8)
> > +TPM2B_APPEND(u16)
> > +TPM2B_APPEND(u32)
> > +
> > +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> > +{
> > +	return buf->buf.data + sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline u16 tpm2b_len(struct tpm2b *buf)
> > +{
> > +	return tpm_buf_length(&buf->buf) - sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > +	tpm_buf_destroy(&buf->buf);
> > +}
> > +
> > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b *tpm2b)
> > +{
> > +	u16 len = tpm2b_len(tpm2b);
> > +
> > +	tpm_buf_append_u16(buf, len);
> > +	tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> > +	/* clear the buf for reuse */
> > +	tpm2b_reset(tpm2b);
> > +}
> > +
> > +/* Macros for unmarshalling known size BE data */
> > +#define GET_INC(type)					\
> > +static inline u##type get_inc_##type(const u8 **ptr) {	\
> > +	u##type val;					\
> > +	val = get_unaligned_be##type(*ptr);		\
> > +	*ptr += sizeof(val);				\
> > +	return val;					\
> > +}
> > +
> > +GET_INC(16)
> > +GET_INC(32)
> > +
> > +#endif
> > -- 
> > 2.12.3
> > 
> 
> Some meta stuff:
> 
> * Add me to TO-field because I should probably review and eventually
>   test these, right?
> * CC to linux-security-module
> * Why there is no RFC tag given that these are so quite large changes?
> * Why in hell tpm2b.h?
> 
> /Jarkko

Some other large scale level stuff that I saw:

* Do not document functions in the file header.
* Make the stuff in the header (expect functio descriptions) as
  documentation block:
  https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
  If you don't want to do this, then just move the documentation to
  the commit message. I rather do not have the documentation at all
  in the files than have it in unmaintanable form.
* Create structs for complex things that you add inside TPM commands
  and declare these helpers right before the function and add them
  with tpm_buf_append(). A good metric for this is when you see your
  self adding field names as a comment. This should make these patches
  a factor cleaner. We use this approach in some places such as
  tpm2_get_pcr_allocation().
* Please, oh please get rid of this tpm2b.h. It is a definitive NAK (OK
  I said it already above but cannot really over emphasize it).
* Short summary should be "tpm: add encrypted and integrity protected
  sessions" or something along the lines.

I guess this is enough for first review round?

/Jarkko

Re: [RFC 0/2] add integrity and security to TPM2 transactions

[Jason Gunthorpe]

On Fri, Mar 02, 2018 at 10:04:54PM -0800, James Bottomley wrote:
> By now, everybody knows we have a problem with the TPM2_RS_PW easy
> button on TPM2 in that transactions on the TPM bus can be intercepted
> and altered.  The way to fix this is to use real sessions for HMAC
> capabilities to ensure integrity and to use parameter and response
> encryption to ensure confidentiality of the data flowing over the TPM
> bus.

We have the same issue for TPM1 then right?

Jason

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[James Bottomley]

On Mon, 2018-03-05 at 13:35 +0200, Jarkko Sakkinen wrote:
> On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> > 
> > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> > new file mode 100644
> > index 000000000000..c7726f2895aa
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm2b.h
> > @@ -0,0 +1,82 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _TPM2_TPM2B_H
> > +#define _TPM2_TPM2B_H
> > +/*
> > + * Handing for tpm2b structures to facilitate the building of
> > commands
> > + */
> > +
> > +#include "tpm.h"
> > +
> > +#include <asm/unaligned.h>
> > +
> > +struct tpm2b {
> > +	struct tpm_buf buf;
> > +};
> > +
> > +/* opaque structure, holds auth session parameters like the
> > session key */
> > +struct tpm2_auth;
> > +
> > +static inline int tpm2b_init(struct tpm2b *buf)
> > +{
> > +	return tpm_buf_init(&buf->buf, 0, 0);
> > +}
> > +
> > +static inline void tpm2b_reset(struct tpm2b *buf)
> > +{
> > +	struct tpm_input_header *head;
> > +
> > +	head = (struct tpm_input_header *)buf->buf.data;
> > +	head->length = cpu_to_be32(sizeof(*head));
> > +}
> > +
> > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned
> > char *data,
> > +				unsigned int len)
> > +{
> > +	tpm_buf_append(&buf->buf, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > +	static inline void tpm2b_append_##type(struct tpm2b *buf,
> > const type value) { tpm_buf_append_##type(&buf->buf, value); }
> > +
> > +TPM2B_APPEND(u8)
> > +TPM2B_APPEND(u16)
> > +TPM2B_APPEND(u32)
> > +
> > +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> > +{
> > +	return buf->buf.data + sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline u16 tpm2b_len(struct tpm2b *buf)
> > +{
> > +	return tpm_buf_length(&buf->buf) - sizeof(struct
> > tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > +	tpm_buf_destroy(&buf->buf);
> > +}
> > +
> > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct
> > tpm2b *tpm2b)
> > +{
> > +	u16 len = tpm2b_len(tpm2b);
> > +
> > +	tpm_buf_append_u16(buf, len);
> > +	tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> > +	/* clear the buf for reuse */
> > +	tpm2b_reset(tpm2b);
> > +}
> > +
> > +/* Macros for unmarshalling known size BE data */
> > +#define GET_INC(type)					\
> > +static inline u##type get_inc_##type(const u8 **ptr) {	\
> > +	u##type val;					\
> > +	val = get_unaligned_be##type(*ptr);		\
> > +	*ptr += sizeof(val);				\
> > +	return val;					\
> > +}
> > +
> > +GET_INC(16)
> > +GET_INC(32)
> > +
> > +#endif
> > -- 
> > 2.12.3
> > 
> 
> Some meta stuff:
> 
> * Add me to TO-field because I should probably review and eventually
>   test these, right?

Eventually; they're an RFC because we need to get the API right first
(I've already got a couple of fixes to it).

> * CC to linux-security-module

There's no change to anything in security module, so why?  All security
module people who care about the TPM should be on linux-integrity and
those who don't likely don't want to see the changes.  The reason
linux-crypto is on the cc is because I want them to make sure I'm using
their crypto system correctly.

> * Why there is no RFC tag given that these are so quite large
> changes?

There is an RFC tag on 0/2

> * Why in hell tpm2b.h?

Because all sized TPM structures are in 2B form and manipulating them
can be made a lot easier with helper routines.

James

Re: [RFC 0/2] add integrity and security to TPM2 transactions

[James Bottomley]

On Mon, 2018-03-05 at 07:04 -0700, Jason Gunthorpe wrote:
> On Fri, Mar 02, 2018 at 10:04:54PM -0800, James Bottomley wrote:
> > 
> > By now, everybody knows we have a problem with the TPM2_RS_PW easy
> > button on TPM2 in that transactions on the TPM bus can be
> > intercepted and altered.  The way to fix this is to use real
> > sessions for HMAC capabilities to ensure integrity and to use
> > parameter and response encryption to ensure confidentiality of the
> > data flowing over the TPM bus.
> 
> We have the same issue for TPM1 then right?

Sort of.  HMAC authentication isn't optional in TPM1 like it is in
TPM2, so we do already use it (in the trusted keys code, for instance),
so we have less of a problem becasuse it doesn't have the insecure
TPM_RS_PW option.

However, TPM1 also has a specific weakness here in that if you don't
have authority in the object (i.e. no shared secret), the HMAC provides
no protection against an intelligent attacker.  The only way to get the
same security as we have with TPM2 in this situation is to use
transport encryption.

Given that sha1 is already compromised, TPM1 has a strictly limited
shelf life, especially as all laptops are being shipped with TPM2 now,
so I don't think it's unreasonable to say if you're worried about this
compromise you should use TPM2.

James

Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

On Mon, Mar 05, 2018 at 06:58:32AM -0800, James Bottomley wrote:
> On Mon, 2018-03-05 at 13:35 +0200, Jarkko Sakkinen wrote:
> > On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote:
> > > 
> > > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h
> > > new file mode 100644
> > > index 000000000000..c7726f2895aa
> > > --- /dev/null
> > > +++ b/drivers/char/tpm/tpm2b.h
> > > @@ -0,0 +1,82 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef _TPM2_TPM2B_H
> > > +#define _TPM2_TPM2B_H
> > > +/*
> > > + * Handing for tpm2b structures to facilitate the building of
> > > commands
> > > + */
> > > +
> > > +#include "tpm.h"
> > > +
> > > +#include <asm/unaligned.h>
> > > +
> > > +struct tpm2b {
> > > +	struct tpm_buf buf;
> > > +};
> > > +
> > > +/* opaque structure, holds auth session parameters like the
> > > session key */
> > > +struct tpm2_auth;
> > > +
> > > +static inline int tpm2b_init(struct tpm2b *buf)
> > > +{
> > > +	return tpm_buf_init(&buf->buf, 0, 0);
> > > +}
> > > +
> > > +static inline void tpm2b_reset(struct tpm2b *buf)
> > > +{
> > > +	struct tpm_input_header *head;
> > > +
> > > +	head = (struct tpm_input_header *)buf->buf.data;
> > > +	head->length = cpu_to_be32(sizeof(*head));
> > > +}
> > > +
> > > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned
> > > char *data,
> > > +				unsigned int len)
> > > +{
> > > +	tpm_buf_append(&buf->buf, data, len);
> > > +}
> > > +
> > > +#define TPM2B_APPEND(type) \
> > > +	static inline void tpm2b_append_##type(struct tpm2b *buf,
> > > const type value) { tpm_buf_append_##type(&buf->buf, value); }
> > > +
> > > +TPM2B_APPEND(u8)
> > > +TPM2B_APPEND(u16)
> > > +TPM2B_APPEND(u32)
> > > +
> > > +static inline void *tpm2b_buffer(const struct tpm2b *buf)
> > > +{
> > > +	return buf->buf.data + sizeof(struct tpm_input_header);
> > > +}
> > > +
> > > +static inline u16 tpm2b_len(struct tpm2b *buf)
> > > +{
> > > +	return tpm_buf_length(&buf->buf) - sizeof(struct
> > > tpm_input_header);
> > > +}
> > > +
> > > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > > +{
> > > +	tpm_buf_destroy(&buf->buf);
> > > +}
> > > +
> > > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct
> > > tpm2b *tpm2b)
> > > +{
> > > +	u16 len = tpm2b_len(tpm2b);
> > > +
> > > +	tpm_buf_append_u16(buf, len);
> > > +	tpm_buf_append(buf, tpm2b_buffer(tpm2b), len);
> > > +	/* clear the buf for reuse */
> > > +	tpm2b_reset(tpm2b);
> > > +}
> > > +
> > > +/* Macros for unmarshalling known size BE data */
> > > +#define GET_INC(type)					\
> > > +static inline u##type get_inc_##type(const u8 **ptr) {	\
> > > +	u##type val;					\
> > > +	val = get_unaligned_be##type(*ptr);		\
> > > +	*ptr += sizeof(val);				\
> > > +	return val;					\
> > > +}
> > > +
> > > +GET_INC(16)
> > > +GET_INC(32)
> > > +
> > > +#endif
> > > -- 
> > > 2.12.3
> > > 
> > 
> > Some meta stuff:
> > 
> > * Add me to TO-field because I should probably review and eventually
> >   test these, right?
> 
> Eventually; they're an RFC because we need to get the API right first
> (I've already got a couple of fixes to it).

For me the big picture looks good. You saw my comments about details.
Refine those and I think this would already be digestable change.

> > * CC to linux-security-module
> 
> There's no change to anything in security module, so why?  All security
> module people who care about the TPM should be on linux-integrity and
> those who don't likely don't want to see the changes.  The reason
> linux-crypto is on the cc is because I want them to make sure I'm using
> their crypto system correctly.

See:

https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity

The big changes that affect the whole security tree in direct or
indirect ways should go through that list. This was a wish from
James Morris.

> 
> > * Why there is no RFC tag given that these are so quite large
> > changes?
> 
> There is an RFC tag on 0/2

Ah, sorry, so it is.

> > * Why in hell tpm2b.h?
> 
> Because all sized TPM structures are in 2B form and manipulating them
> can be made a lot easier with helper routines.

I see it now that I looked the code in more detail.

Suggestions to move forward:

* Add enum tpm_buf_type { TPM_BUF_COMMAND, TPM_BUF_2B } and use
  struct tpm_buf for both.
* Move tpm_buf_* stuff from tpm.h and tpm2-cmd.c to tpm_buf_*.[ch].

I would even suggest to move current inline functions to tpm_buf.c
so that they can be traced. Performance impact is neglible but
tracing is an useful asset for testing.

For get_inc* I would just roll out two separate functions as the
redudancy is quite neglibe. I just want to keep things as simple
and trivial as possible.

> James

/Jarkko

Re: [RFC 0/2] add integrity and security to TPM2 transactions

[Ken Goldman]

On 3/5/2018 9:04 AM, Jason Gunthorpe wrote:
> On Fri, Mar 02, 2018 at 10:04:54PM -0800, James Bottomley wrote:
>> By now, everybody knows we have a problem with the TPM2_RS_PW easy
>> button on TPM2 in that transactions on the TPM bus can be intercepted
>> and altered.  The way to fix this is to use real sessions for HMAC
>> capabilities to ensure integrity and to use parameter and response
>> encryption to ensure confidentiality of the data flowing over the TPM
>> bus.
> 
> We have the same issue for TPM1 then right?

TPM 1.2 did not have the concept of plaintext password sessions.  If 
authorization was used, it was always with an HMAC.