Repository: google/go-tpm
Pull request #74 (2019-02-22 19:26:01)

Implement TPM2_EncryptDecrypt2
(Recreation of closed PR #37)

Symmetric encryption and decryption. There is some convenience chopping logic in case processed data can't fit into one command.

#37 was closed because of some concerns about exposing an API that is too easily misused and the PR lost priority. A strong need for this functionality within the Go interface has become apparent again and I think this is still an acceptable implementation for `TPM2_EncryptDecrypt2`. I believe tpm2.go should be the home for 1:1 mappings of all TPM2 commands even if the command doesn't effectively convey is security guarantees and could be easily misused. Interested to hear feedback! :smiley: 

awly: cc @tmroeder who raised concerns about this last time.
I agree with Sam, the fact that TPM exposes functionality that could be mis-used is a problem with the spec, and should be addressed there.
Omitting a useful piece of functionality because of this isn't the right solution IMO.

For context, our use-case is: storing a sensitive credential (TLS client private key, provisioned per-VM) on disk encrypted instead of plaintext. Using a TPM key for encryption binds it to the host and prevents exfiltration via filesystem access (although it could still be dumped from memory).
awly: > As I understand it, the additional chunking is just passing more bytes into the same encryption operation (that is, using the same IV and key), right?

Right, this is for encryption/decryption of data larger than the TPM command buffer size (~1KB).
TPM2_EncryptDecrypt also returns a new IV for the next block on each call.

> I would _really_ like to have a strong warning on this and any other potentially dangerous operations, though.

I agree. @samdamana could you add a big scary warning to both funcs?

> I hope that your encryption is also using integrity protection in addition to encryption, because there are otherwise attacks that might be possible, depending on the exact details of your threat model.

Can you clarify what the possible attacks are? And is there a TPM primitive that will give us encryption + integrity?

I (naively) assume that if encrypted blob gets modified, decryption fails or the resulting data is corrupt. The latter would just make the upstream user of the ecnrypted credential (TLS key) fail. But our main goal is exfiltration protection here.
tmroeder: > > As I understand it, the additional chunking is just passing more bytes into the same encryption operation (that is, using the same IV and key), right?
> 
> Right, this is for encryption/decryption of data larger than the TPM command buffer size (~1KB).
> TPM2_EncryptDecrypt also returns a new IV for the next block on each call.
> 
> > I would _really_ like to have a strong warning on this and any other potentially dangerous operations, though.
> 
> I agree. @samdamana could you add a big scary warning to both funcs?
> 
> > I hope that your encryption is also using integrity protection in addition to encryption, because there are otherwise attacks that might be possible, depending on the exact details of your threat model.
> 
> Can you clarify what the possible attacks are? And is there a TPM primitive that will give us encryption + integrity?
> 
> I (naively) assume that if encrypted blob gets modified, decryption fails or the resulting data is corrupt. The latter would just make the upstream user of the ecnrypted credential (TLS key) fail. But our main goal is exfiltration protection here.

Basic encryption modes like CTR, CFB or CBC don't detect ciphertext modification. Instead, the data returned is modified, sometimes in ways that the attacker can control. If you're just encrypting the bytes of the key itself, then modifications will corrupt the key, and you might be OK, if you're guaranteed that using a corrupted key won't cause some other problem, and will just fail. But if the plaintext has any structure to it (like a serialized protobuf), it might be possible to make controlled changes to the decrypted plaintext by XOR'ing bytes against the ciphertext.

The fundamental problem is that modes that provide encryption without integrity are not proven secure in a model where adversaries can modify ciphertext and pass it to a decryption oracle. They are only proven secure against adversaries that can cause encryption to happen and observe the ciphertexts.

TPM2_Create is the TPM 2.0 equivalent of the Seal operation, and that should provide both encryption and integrity. You use TPM2_Unseal to get the data back. Then you don't have to use 
low-level commands.

Support for this use case of TPM2_Create was added in PR #27.
awly: Attacker making controlled changes to the decrypted data _should_ be ok in our case. At worst, they cause minor DOS (they key is rotated eventually anyway) or exploit a vuln in key parsing code.
But that would require disk write access as root, which they can probably do worse things with.

TPM2_Create (Seal) is where we started. https://github.com/google/go-tpm/blob/master/examples/tpm2-seal-unseal/main.go following this example we can get only ~128 bytes of secret data in. That's less than the private key size we need to encrypt.

We were advised to: create a separate block cipher key, encrypt our private key with that and then seal the block cipher key with TPM2_Create.
@immutableT suggested that just using TPM2_EncryptDecrypt achieves the same without the need to store an additional key.

Either solution would improve the status quo (storing plaintext key on disk), we'd like to avoid extra complexity as much as possible.
tmroeder: > Attacker making controlled changes to the decrypted data _should_ be ok in our case. At worst, they cause minor DOS (they key is rotated eventually anyway) or exploit a vuln in key parsing code.
> But that would require disk write access as root, which they can probably do worse things with.
> 
> TPM2_Create (Seal) is where we started. https://github.com/google/go-tpm/blob/master/examples/tpm2-seal-unseal/main.go following this example we can get only ~128 bytes of secret data in. That's less than the private key size we need to encrypt.
> 
> We were advised to: create a separate block cipher key, encrypt our private key with that and then seal the block cipher key with TPM2_Create.
> @immutableT suggested that just using TPM2_EncryptDecrypt achieves the same without the need to store an additional key.
> 
> Either solution would improve the status quo (storing plaintext key on disk), we'd like to avoid extra complexity as much as possible.

Ah, OK.

In that case, the easiest solution that gives you encryption with integrity is probably to use an AEAD using a key that you seal with TPM2_Create. AFAIK, none of the modes in TPM2_EncryptDecrypt2 give you encryption with integrity, so that's not the same as TPM2_Create to wrap a key, then using that key with a mode that gives encryption and integrity.

Either ChaCha20-Poly1305 (https://godoc.org/golang.org/x/crypto/chacha20poly1305) or AES GCM (https://golang.org/pkg/crypto/cipher/#NewGCM) would be fine in this case.

I strongly recommend always using encryption with integrity, since even if you might be able to argue encryption without integrity works for you now, it's fragile and risks problems in the future. Given that TPM2_Create + a call to a common library is fairly simple, that would be my recommendation. However, if you haven't had our internal Google crypto folks review it, you might want to get their take, too.
immutableT: When discussing the scenario that @awly and @samdamana are working on, it seemed to me that sealing was not the appropriate primitive, since (at least now) we are not leveraging PCR measurements. Therefore, I suggested using symmetric encryption to protect tokens. 

I agree with @tmroeder that integrity validation is important and we should implement unless there is some technical blocker.
Assuming that we can't use TPM2_Create that gives confidentiality and integrity, we could combine symmetric encryption with HMAC to achieve both.

immutableT: When discussing the scenario that @awly and @samdamana are working on, it seemed to me that sealing was not the appropriate primitive, since (at least now) we are not leveraging PCR measurements. Therefore, I suggested using symmetric encryption to protect tokens. 

I agree with @tmroeder that integrity validation is important and we should implement unless there is some technical blocker.
Assuming that we can't use TPM2_Create that gives confidentiality and integrity, we could combine symmetric encryption with HMAC to achieve both.

awly: @tmroeder PTAL
We decided to go with HMAC in addition to TPM2_EncryptDecrypt as @immutableT described.
Integrity would be nice-to-have in our case, but not critical, so we'll implement it incrementally.
tmroeder: > @tmroeder PTAL
> We decided to go with HMAC in addition to TPM2_EncryptDecrypt as @immutableT described.
> Integrity would be nice-to-have in our case, but not critical, so we'll implement it incrementally.

FWIW, using encryption + HMAC can in fact give you an AEAD (i.e., encryption with integrity) just as well as the integrated AEAD modes can, as long as you do it right: if M is your message, along with IV, and keys K1 (for encryption) and K2 for (HMAC):

C = Enc(K1, M; IV)
T = HMAC(K2, IV || C)

and store IV || C || T  ( == initialization vector, ciphertext, and tag), then you have just as strong properties as with GCM or ChaCha20-Poly1305. The HMAC protects the ciphertext and IV from modification, and you always check it before trying to decrypt: Given IV || C || T, and K1, K2 as before:

if (HMAC(K2, IV ||  C) != T) // must check in constant time
  break

M = Dec(K1, C; IV)
immutableT: @tmroeder Yes this is what we discussed - use K2 for integrity validation via HMAC.

samdamana: Thanks @tmroeder that is a great description of the steps and will be useful to refer to when it is implemented.

Before I can do that the TPM2_HMAC primitive needs to be exposed in tpm2.go. I will probably end up making that PR in a few weeks or so. :+1: 

Patch:
diff --git a/tpm2/constants.go b/tpm2/constants.go
index 9c8ccc1..7819228 100644
--- a/tpm2/constants.go
+++ b/tpm2/constants.go
@@ -29,6 +29,11 @@ func init() {
 	tpmutil.UseTPM20LengthPrefixSize()
 }
 
+// MAX_DIGEST_BUFFER is the maximum size of []byte request or response fields.
+// Typically used for chunking of big blobs of data (such as for hashing or
+// encryption).
+const maxDigestBuffer = 1024
+
 // Algorithm represents a TPM_ALG_ID value.
 type Algorithm uint16
 
@@ -73,6 +78,7 @@ const (
 	AlgECDAA     Algorithm = 0x001A
 	AlgKDF2      Algorithm = 0x0021
 	AlgECC       Algorithm = 0x0023
+	AlgSymCipher Algorithm = 0x0025
 	AlgCTR       Algorithm = 0x0040
 	AlgOFB       Algorithm = 0x0041
 	AlgCBC       Algorithm = 0x0042
@@ -109,6 +115,7 @@ const (
 // SessionAttributes represents an attribute of a session.
 type SessionAttributes byte
 
+// Session Attributes (Structures 8.4 TPMA_SESSION)
 const (
 	AttrContinueSession SessionAttributes = 1 << iota
 	AttrAuditExclusive
@@ -274,6 +281,7 @@ const (
 	cmdUnseal           tpmutil.Command = 0x0000015E
 	cmdContextLoad      tpmutil.Command = 0x00000161
 	cmdContextSave      tpmutil.Command = 0x00000162
+	cmdEncryptDecrypt   tpmutil.Command = 0x00000164
 	cmdFlushContext     tpmutil.Command = 0x00000165
 	cmdLoadExternal     tpmutil.Command = 0x00000167
 	cmdMakeCredential   tpmutil.Command = 0x00000168
@@ -291,6 +299,7 @@ const (
 	cmdPCRExtend       tpmutil.Command = 0x00000182
 	cmdPolicyGetDigest tpmutil.Command = 0x00000189
 	cmdPolicyPassword  tpmutil.Command = 0x0000018C
+	cmdEncryptDecrypt2 tpmutil.Command = 0x00000193
 )
 
 // Regular TPM 2.0 devices use 24-bit mask (3 bytes) for PCR selection.
diff --git a/tpm2/structures.go b/tpm2/structures.go
index cb36e7c..6dd8b37 100644
--- a/tpm2/structures.go
+++ b/tpm2/structures.go
@@ -63,8 +63,9 @@ type Public struct {
 	// If Type is AlgKeyedHash, then do not set these.
 	// Otherwise, only one of the Parameters fields should be set. When encoding/decoding,
 	// one will be picked based on Type.
-	RSAParameters *RSAParams
-	ECCParameters *ECCParams
+	RSAParameters       *RSAParams
+	ECCParameters       *ECCParams
+	SymCipherParameters *SymCipherParams
 }
 
 // Encode serializes a Public structure in TPM wire format.
@@ -84,11 +85,13 @@ func (p Public) Encode() ([]byte, error) {
 		params, err = tpmutil.Pack(AlgNull, unique)
 	case AlgECC:
 		params, err = p.ECCParameters.encode()
+	case AlgSymCipher:
+		params, err = p.SymCipherParameters.encode()
 	default:
-		err = fmt.Errorf("unsupported type in TPMT_PUBLIC: %v", p.Type)
+		err = fmt.Errorf("unsupported type in TPMT_PUBLIC: 0x%x", p.Type)
 	}
 	if err != nil {
-		return nil, fmt.Errorf("encoding RSAParameters, ECCParameters or KeyedHash: %v", err)
+		return nil, fmt.Errorf("encoding RSAParameters, ECCParameters, SymCipherParameters or KeyedHash: %v", err)
 	}
 	return concat(head, params)
 }
@@ -132,8 +135,10 @@ func DecodePublic(buf []byte) (Public, error) {
 		pub.RSAParameters, err = decodeRSAParams(in)
 	case AlgECC:
 		pub.ECCParameters, err = decodeECCParams(in)
+	case AlgSymCipher:
+		pub.SymCipherParameters, err = decodeSymCipherParams(in)
 	default:
-		err = fmt.Errorf("unsupported type in TPMT_PUBLIC: %v", pub.Type)
+		err = fmt.Errorf("unsupported type in TPMT_PUBLIC: 0x%x", pub.Type)
 	}
 	return pub, err
 }
@@ -303,6 +308,37 @@ func decodeECCParams(in *bytes.Buffer) (*ECCParams, error) {
 	return &params, nil
 }
 
+// SymCipherParams represents parameters of a symmetric block cipher TPM object.
+type SymCipherParams struct {
+	Symmetric *SymScheme
+	Unique    []byte
+}
+
+func (p *SymCipherParams) encode() ([]byte, error) {
+	sym, err := p.Symmetric.encode()
+	if err != nil {
+		return nil, fmt.Errorf("encoding Symmetric: %v", err)
+	}
+	unique, err := tpmutil.Pack(p.Unique)
+	if err != nil {
+		return nil, fmt.Errorf("encoding Unique: %v", err)
+	}
+	return concat(sym, unique)
+}
+
+func decodeSymCipherParams(in *bytes.Buffer) (*SymCipherParams, error) {
+	var params SymCipherParams
+	var err error
+
+	if params.Symmetric, err = decodeSymScheme(in); err != nil {
+		return nil, fmt.Errorf("decoding Symmetric: %v", err)
+	}
+	if err := tpmutil.UnpackBuf(in, &params.Unique); err != nil {
+		return nil, fmt.Errorf("decoding Unique: %v", err)
+	}
+	return &params, nil
+}
+
 // SymScheme represents a symmetric encryption scheme.
 type SymScheme struct {
 	Alg     Algorithm
diff --git a/tpm2/tpm2.go b/tpm2/tpm2.go
index 1b9a2e6..e16c8d3 100644
--- a/tpm2/tpm2.go
+++ b/tpm2/tpm2.go
@@ -1312,8 +1312,8 @@ func CertifyCreation(rw io.ReadWriter, objectAuth string, object, signer tpmutil
 
 func runCommand(rw io.ReadWriter, tag tpmutil.Tag, cmd tpmutil.Command, in ...interface{}) ([]byte, error) {
 	resp, code, err := tpmutil.RunCommand(rw, tag, cmd, in...)
-	if err != nil {
-		return nil, err
+	if err != nil || code != tpmutil.RCSuccess {
+		return nil, decodeResponse(code)
 	}
 	return resp, decodeResponse(code)
 }
@@ -1368,3 +1368,126 @@ func ReadPCR(rw io.ReadWriter, pcr int, hashAlg Algorithm) ([]byte, error) {
 	}
 	return pcrVal, nil
 }
+
+// EncryptSymmetric encrypts data using a symmetric key.
+//
+// WARNING: This command performs low-level cryptographic operations.
+// Secure use of this command is subtle and requires careful analysis.
+// Please consult with experts in cryptography for how to use it securely.
+//
+// The iv is the initialization vector. The iv must not be empty and its size depends on the
+// details of the symmetric encryption scheme.
+//
+// The data may be longer than block size, EncryptSymmetric will chain
+// multiple TPM calls to encrypt the entire blob.
+//
+// Key handle should point at SymCipher object which is a child of the key (and
+// not e.g. RSA key itself).
+func EncryptSymmetric(rw io.ReadWriteCloser, keyAuth string, key tpmutil.Handle, iv, data []byte) ([]byte, error) {
+	return encryptDecryptSymmetric(rw, keyAuth, key, iv, data, false)
+}
+
+// DecryptSymmetric decrypts data using a symmetric key.
+//
+// WARNING: This command performs low-level cryptographic operations.
+// Secure use of this command is subtle and requires careful analysis.
+// Please consult with experts in cryptography for how to use it securely.
+//
+// The iv is the initialization vector. The iv must not be empty and its size
+// depends on the details of the symmetric encryption scheme.
+//
+// The data may be longer than block size, DecryptSymmetric will chain multiple
+// TPM calls to decrypt the entire blob.
+//
+// Key handle should point at SymCipher object which is a child of the key (and
+// not e.g. RSA key itself).
+func DecryptSymmetric(rw io.ReadWriteCloser, keyAuth string, key tpmutil.Handle, iv, data []byte) ([]byte, error) {
+	return encryptDecryptSymmetric(rw, keyAuth, key, iv, data, true)
+}
+
+func encodeEncryptDecrypt(keyAuth string, key tpmutil.Handle, iv, data []byte, decrypt bool) ([]byte, error) {
+	ha, err := tpmutil.Pack(key)
+	if err != nil {
+		return nil, err
+	}
+	auth, err := encodeAuthArea(AuthCommand{Session: HandlePasswordSession, Attributes: AttrContinueSession, Auth: []byte(keyAuth)})
+	if err != nil {
+		return nil, err
+	}
+	// Use encryption key's mode.
+	params, err := tpmutil.Pack(decrypt, AlgNull, iv, data)
+	if err != nil {
+		return nil, err
+	}
+	return concat(ha, auth, params)
+}
+
+func encodeEncryptDecrypt2(keyAuth string, key tpmutil.Handle, iv, data []byte, decrypt bool) ([]byte, error) {
+	ha, err := tpmutil.Pack(key)
+	if err != nil {
+		return nil, err
+	}
+	auth, err := encodeAuthArea(AuthCommand{Session: HandlePasswordSession, Attributes: AttrContinueSession, Auth: []byte(keyAuth)})
+	if err != nil {
+		return nil, err
+	}
+	// Use encryption key's mode.
+	params, err := tpmutil.Pack(data, decrypt, AlgNull, iv)
+	if err != nil {
+		return nil, err
+	}
+	return concat(ha, auth, params)
+}
+
+func decodeEncryptDecrypt(resp []byte) ([]byte, []byte, error) {
+	var paramSize uint32
+	var out, nextIV []byte
+	if _, err := tpmutil.Unpack(resp, &paramSize, &out, &nextIV); err != nil {
+		return nil, nil, err
+	}
+	return out, nextIV, nil
+}
+
+func encryptDecryptBlockSymmetric(rw io.ReadWriteCloser, keyAuth string, key tpmutil.Handle, iv, data []byte, decrypt bool) ([]byte, []byte, error) {
+	cmd, err := encodeEncryptDecrypt2(keyAuth, key, iv, data, decrypt)
+	if err != nil {
+		return nil, nil, err
+	}
+	resp, err := runCommand(rw, TagSessions, cmdEncryptDecrypt2, tpmutil.RawBytes(cmd))
+	if err != nil {
+		fmt0Err, ok := err.(Error)
+		if ok && fmt0Err.Code == RCCommandCode {
+			// If TPM2_EncryptDecrypt2 is not supported, fall back to
+			// TPM2_EncryptDecrypt.
+			cmd, err := encodeEncryptDecrypt(keyAuth, key, iv, data, decrypt)
+			if err != nil {
+				return nil, nil, err
+			}
+			resp, err = runCommand(rw, TagSessions, cmdEncryptDecrypt, tpmutil.RawBytes(cmd))
+		}
+	}
+	if err != nil {
+		return nil, nil, err
+	}
+	return decodeEncryptDecrypt(resp)
+}
+
+func encryptDecryptSymmetric(rw io.ReadWriteCloser, keyAuth string, key tpmutil.Handle, iv, data []byte, decrypt bool) ([]byte, error) {
+	var out, block []byte
+	var err error
+
+	for rest := data; len(rest) > 0; {
+		if len(rest) > maxDigestBuffer {
+			block, rest = rest[:maxDigestBuffer], rest[maxDigestBuffer:]
+		} else {
+			block, rest = rest, nil
+		}
+		block, iv, err = encryptDecryptBlockSymmetric(rw, keyAuth, key, iv, block, decrypt)
+		if err != nil {
+			return nil, err
+		}
+		out = append(out, block...)
+	}
+
+	return out, nil
+}


Test patch:
diff --git a/tpm2/tpm2_test.go b/tpm2/tpm2_test.go
index 00cdbd0..87ff380 100644
--- a/tpm2/tpm2_test.go
+++ b/tpm2/tpm2_test.go
@@ -905,7 +905,6 @@ func TestEncodeDecodeCreationData(t *testing.T) {
 		t.Errorf("got decoded value:\n%v\nwant:\n%v", decoded, cd)
 	}
 }
-
 func TestCreateAndCertifyCreation(t *testing.T) {
 	rw := openTPM(t)
 	defer rw.Close()
@@ -1164,3 +1163,65 @@ func TestReadPublicKey(t *testing.T) {
 		run(t, public, private, &pk.PublicKey)
 	})
 }
+
+func TestEncryptDecrypt(t *testing.T) {
+	rw := openTPM(t)
+	defer rw.Close()
+
+	parentHandle, _, err := CreatePrimary(rw, HandleOwner, pcrSelection, emptyPassword, defaultPassword, Public{
+		Type:       AlgRSA,
+		NameAlg:    AlgSHA256,
+		Attributes: FlagRestricted | FlagDecrypt | FlagUserWithAuth | FlagFixedParent | FlagFixedTPM | FlagSensitiveDataOrigin,
+		RSAParameters: &RSAParams{
+			Symmetric: &SymScheme{
+				Alg:     AlgAES,
+				KeyBits: 128,
+				Mode:    AlgCFB,
+			},
+			KeyBits: 2048,
+			Modulus: big.NewInt(0),
+		},
+	})
+	if err != nil {
+		t.Fatalf("CreatePrimary failed: %s", err)
+	}
+	defer FlushContext(rw, parentHandle)
+	privateBlob, publicBlob, err := CreateKey(rw, parentHandle, pcrSelection, defaultPassword, defaultPassword, Public{
+		Type:       AlgSymCipher,
+		NameAlg:    AlgSHA256,
+		Attributes: FlagDecrypt | FlagSign | FlagUserWithAuth | FlagFixedParent | FlagFixedTPM | FlagSensitiveDataOrigin,
+		SymCipherParameters: &SymCipherParams{
+			Symmetric: &SymScheme{
+				Alg:     AlgAES,
+				KeyBits: 128,
+				Mode:    AlgCFB,
+			},
+		},
+	})
+	if err != nil {
+		t.Fatalf("CreateKey failed: %s", err)
+	}
+	key, _, err := Load(rw, parentHandle, defaultPassword, publicBlob, privateBlob)
+	if err != nil {
+		t.Fatalf("Load failed: %s", err)
+	}
+	defer FlushContext(rw, key)
+
+	data := bytes.Repeat([]byte("a"), 1e4) // 10KB
+	iv := make([]byte, 16)
+
+	encrypted, err := EncryptSymmetric(rw, defaultPassword, key, iv, data)
+	if err != nil {
+		t.Fatalf("EncryptSymmetric failed: %s", err)
+	}
+	if bytes.Equal(encrypted, data) {
+		t.Error("encrypted blob matches unenecrypted data")
+	}
+	decrypted, err := DecryptSymmetric(rw, defaultPassword, key, iv, encrypted)
+	if err != nil {
+		t.Fatalf("DecryptSymmetric failed: %s", err)
+	}
+	if !bytes.Equal(decrypted, data) {
+		t.Errorf("got decrypted data: %q, want: %q", decrypted, data)
+	}
+}