add integrity and security to TPM2 transactions

RFC In review James Bottomley 4 participants 8 messages Mar 3, 2018 – Apr 8, 2018 1 message not in the archive

Subsystems (2)

Security (LSM) Keys-trusted

lore

Share & tools

lore

James Bottomley

author maintainer

2018-03-03 06:04 UTC #0 lore raw

cover add integrity and security to TPM2 transactions

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

Signature · 1 line

2.12.3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jarkko Sakkinen

2018-03-05 11:35 UTC #2 lore raw

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

Quoted 91 lines · James Bottomley

drivers/char/tpm/tpm2b.h new file +82 -0

new file mode 100644
index 000000000000..c7726f2895aa
@@ -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

Signature · 1 line

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

Jarkko Sakkinen

2018-03-05 11:50 UTC #3 lore raw

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

Quoted 95 lines · Jarkko Sakkinen

Quoted 91 lines · James Bottomley

drivers/char/tpm/tpm2b.h new file +82 -0

new file mode 100644
index 000000000000..c7726f2895aa
@@ -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

Signature · 1 line

2.12.3

Some meta stuff:

Quoted 7 lines

* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

author maintainer

2018-03-05 14:58 UTC #4 lore raw

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

In reply to Jarkko Sakkinen

Quoted 102 lines · Jarkko Sakkinen

Quoted 98 lines · James Bottomley

drivers/char/tpm/tpm2b.h new file +5 -0

new file mode 100644
index 000000000000..c7726f2895aa
@@ -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
+ */

drivers/char/tpm/tpm2b.h new file +10 -0

+
+#include "tpm.h"
+
+#include <asm/unaligned.h>
+
+struct tpm2b {
+	struct tpm_buf buf;
+};
+
+/* opaque structure, holds auth session parameters like the

session key */

drivers/char/tpm/tpm2b.h new file +16 -0

+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,

drivers/char/tpm/tpm2b.h new file +7 -0

+				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); }

drivers/char/tpm/tpm2b.h new file +13 -0

+
+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);

drivers/char/tpm/tpm2b.h new file +8 -0

+}
+
+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)

drivers/char/tpm/tpm2b.h new file +22 -0

+{
+	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

Signature · 1 line

2.12.3

Some meta stuff:

Quoted 2 lines

* 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).

Quoted 1 line

* 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.

Quoted 2 lines

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

There is an RFC tag on 0/2

Quoted 1 line

* 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

Jarkko Sakkinen

2018-03-05 17:41 UTC #5 lore raw

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

Quoted 103 lines · James Bottomley

Quoted 102 lines · Jarkko Sakkinen

Quoted 98 lines · James Bottomley

drivers/char/tpm/tpm2b.h new file +5 -0

new file mode 100644
index 000000000000..c7726f2895aa
@@ -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
+ */

drivers/char/tpm/tpm2b.h new file +10 -0

+
+#include "tpm.h"
+
+#include <asm/unaligned.h>
+
+struct tpm2b {
+	struct tpm_buf buf;
+};
+
+/* opaque structure, holds auth session parameters like the

session key */

drivers/char/tpm/tpm2b.h new file +16 -0

+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,

drivers/char/tpm/tpm2b.h new file +7 -0

+				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); }

drivers/char/tpm/tpm2b.h new file +13 -0

+
+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);

drivers/char/tpm/tpm2b.h new file +8 -0

+}
+
+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)

drivers/char/tpm/tpm2b.h new file +22 -0

+{
+	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

Signature · 1 line

2.12.3

Some meta stuff:

Quoted 5 lines

Quoted 2 lines
* 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.

Quoted 7 lines

Quoted 1 line
* 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.

Quoted 5 lines

Quoted 3 lines
* 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.

Quoted 4 lines

Quoted 1 line
* 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.

Quoted 1 line

James

/Jarkko

Jason Gunthorpe

2018-03-05 14:04 UTC #6 lore raw

In reply to James Bottomley

Quoted 6 lines · 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.

We have the same issue for TPM1 then right?

Jason

James Bottomley

author maintainer

2018-03-05 15:42 UTC #7 lore raw

Quoted 10 lines · Jason Gunthorpe

Quoted 7 lines · 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.
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

Ken Goldman

2018-04-08 20:28 UTC #8 lore raw

In reply to Jason Gunthorpe

Quoted 9 lines · Jason Gunthorpe

Quoted 6 lines · 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.
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.