Repository: google/go-tpm (go)

Issue:
tpm2: CreateKey should not return a tpmutil.Handle
The TPM2_Create command does not return a handle, but tpm2.CreateKey tries to decode and return one anyway, resulting in a bogus tpmutil.Handle value.

Relatedly, the TPM2_CreateLoaded command doesn't seem available.

Pull request:
Fix parameter decoding on TPM2
Fixes #102

[Part 3 of the Spec](https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-3-Commands-01.38-code.pdf) (sections 4.2.1 and 4.3) describe how, when a command uses Authorization Tags, there is an additional UINT32 `paramSize` in the response, between the handles and parameters. We usually properly deal with this parameter, but not always.

This [spreadsheet of TPM2 commands](https://docs.google.com/spreadsheets/d/1WfZCb1G7wK0tZetN51lI7bPWbazSY3m5AlUYIzRjhK0/edit?usp=sharing) lists all the TPM2 commands (including if they should have authorization sessions). We only care about commands where:
  - An authorization session is required
  - We care about the return parameters (otherwise we won't even bother decoding the response).

The only commands we were messing up were TPM2_Create and TPM2_PolicySecret. These are now fixed.

EDIT: Note that the changes to `Create` are backwards incompatible, so incrementing the version will be required. 

Patch:
diff --git a/.gitignore b/.gitignore
index 9ed3b07..d358bd8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,1 +1,2 @@
 *.test
+.vscode*
diff --git a/tpm2/tpm2.go b/tpm2/tpm2.go
index a3d089c..5f31916 100644
--- a/tpm2/tpm2.go
+++ b/tpm2/tpm2.go
@@ -534,31 +534,31 @@ func ReadPublic(rw io.ReadWriter, handle tpmutil.Handle) (Public, []byte, []byte
 	return decodeReadPublic(resp)
 }
 
-func decodeCreate(in []byte) (handle tpmutil.Handle, private, public, creationData, creationHash tpmutil.U16Bytes, creationTicket Ticket, err error) {
+func decodeCreate(in []byte) (private, public, creationData, creationHash tpmutil.U16Bytes, creationTicket Ticket, err error) {
 	buf := bytes.NewBuffer(in)
-
-	if err := tpmutil.UnpackBuf(buf, &handle, &private, &public, &creationData, &creationHash); err != nil {
-		return 0, nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding Handle, Private, Public, CreationData, CreationHash: %v", err)
+	var paramSize uint32
+	if err := tpmutil.UnpackBuf(buf, &paramSize, &private, &public, &creationData, &creationHash); err != nil {
+		return nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding Handle, Private, Public, CreationData, CreationHash: %v", err)
 	}
 	cr, err := decodeTicket(buf)
 	if err != nil {
-		return 0, nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding CreationTicket: %v", err)
+		return nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding CreationTicket: %v", err)
 	}
 	creationTicket = *cr
 	if _, err := DecodeCreationData(creationData); err != nil {
-		return 0, nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding CreationData: %v", err)
+		return nil, nil, nil, nil, Ticket{}, fmt.Errorf("decoding CreationData: %v", err)
 	}
-	return handle, private, public, creationData, creationHash, creationTicket, nil
+	return private, public, creationData, creationHash, creationTicket, nil
 }
 
-func create(rw io.ReadWriter, parentHandle tpmutil.Handle, parentPassword, objectPassword string, sensitiveData []byte, pub Public, pcrSelection PCRSelection) (handle tpmutil.Handle, private, public, creationData, creationHash []byte, creationTicket Ticket, err error) {
+func create(rw io.ReadWriter, parentHandle tpmutil.Handle, parentPassword, objectPassword string, sensitiveData []byte, pub Public, pcrSelection PCRSelection) (private, public, creationData, creationHash []byte, creationTicket Ticket, err error) {
 	cmd, err := encodeCreate(parentHandle, pcrSelection, parentPassword, objectPassword, sensitiveData, pub)
 	if err != nil {
-		return 0, nil, nil, nil, nil, Ticket{}, err
+		return nil, nil, nil, nil, Ticket{}, err
 	}
 	resp, err := runCommand(rw, TagSessions, cmdCreate, tpmutil.RawBytes(cmd))
 	if err != nil {
-		return 0, nil, nil, nil, nil, Ticket{}, err
+		return nil, nil, nil, nil, Ticket{}, err
 	}
 	return decodeCreate(resp)
 }
@@ -566,7 +566,7 @@ func create(rw io.ReadWriter, parentHandle tpmutil.Handle, parentPassword, objec
 // CreateKey creates a new key pair under the owner handle.
 // Returns private key and public key blobs as well as the
 // creation data, a hash of said data and the creation ticket.
-func CreateKey(rw io.ReadWriter, owner tpmutil.Handle, sel PCRSelection, parentPassword, ownerPassword string, pub Public) (handle tpmutil.Handle, private, public, creationData, creationHash []byte, creationTicket Ticket, err error) {
+func CreateKey(rw io.ReadWriter, owner tpmutil.Handle, sel PCRSelection, parentPassword, ownerPassword string, pub Public) (private, public, creationData, creationHash []byte, creationTicket Ticket, err error) {
 	return create(rw, owner, parentPassword, ownerPassword, nil /*inSensitive*/, pub, sel)
 }
 
@@ -580,7 +580,7 @@ func Seal(rw io.ReadWriter, parentHandle tpmutil.Handle, parentPassword, objectP
 		Attributes: FlagFixedTPM | FlagFixedParent,
 		AuthPolicy: objectAuthPolicy,
 	}
-	_, private, public, _, _, _, err := create(rw, parentHandle, parentPassword, objectPassword, sensitiveData, inPublic, PCRSelection{})
+	private, public, _, _, _, err := create(rw, parentHandle, parentPassword, objectPassword, sensitiveData, inPublic, PCRSelection{})
 	if err != nil {
 		return nil, nil, err
 	}
@@ -691,6 +691,22 @@ func encodePolicySecret(entityHandle tpmutil.Handle, entityAuth AuthCommand, pol
 	return concat(handles, auth, params)
 }
 
+func decodePolicySecret(in []byte) (*Ticket, error) {
+	buf := bytes.NewBuffer(in)
+
+	var paramSize uint32
+	var timeout tpmutil.U16Bytes
+	if err := tpmutil.UnpackBuf(buf, &paramSize, &timeout); err != nil {
+		return nil, fmt.Errorf("decoding timeout: %v", err)
+	}
+
+	ticket, err := decodeTicket(buf)
+	if err != nil {
+		return nil, fmt.Errorf("decoding ticket: %v", err)
+	}
+	return ticket, nil
+}
+
 // PolicySecret sets a secret authorization requirement on the provided entity.
 // If expiry is non-zero, the authorization is valid for expiry seconds.
 func PolicySecret(rw io.ReadWriter, entityHandle tpmutil.Handle, entityAuth AuthCommand, policyHandle tpmutil.Handle, policyNonce, cpHash, policyRef []byte, expiry int32) (*Ticket, error) {
@@ -705,11 +721,7 @@ func PolicySecret(rw io.ReadWriter, entityHandle tpmutil.Handle, entityAuth Auth
 
 	// Tickets are only provided if expiry is set.
 	if expiry != 0 {
-		tk, err := decodeTicket(bytes.NewBuffer(resp))
-		if err != nil {
-			return nil, fmt.Errorf("decoding ticket: %v", err)
-		}
-		return tk, nil
+		return decodePolicySecret(resp)
 	}
 	return nil, nil
 }
@@ -1126,9 +1138,9 @@ func NVReadPublic(rw io.ReadWriter, index tpmutil.Handle) (NVPublic, error) {
 }
 
 func decodeNVRead(in []byte) ([]byte, error) {
-	var sessionAttributes uint32
+	var paramSize uint32
 	var data tpmutil.U16Bytes
-	if _, err := tpmutil.Unpack(in, &sessionAttributes, &data); err != nil {
+	if _, err := tpmutil.Unpack(in, &paramSize, &data); err != nil {
 		return nil, err
 	}
 	return data, nil


Test patch:
diff --git a/tpm2/encoding_test.go b/tpm2/encoding_test.go
index f142b03..1df74f5 100644
--- a/tpm2/encoding_test.go
+++ b/tpm2/encoding_test.go
@@ -219,7 +219,7 @@ func TestDecodeCreate(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if _, _, _, _, _, _, err = decodeCreate(testRespBytes[10:]); err != nil {
+	if _, _, _, _, _, err = decodeCreate(testRespBytes[10:]); err != nil {
 		t.Fatal(err)
 	}
 }
diff --git a/tpm2/tpm2_test.go b/tpm2/tpm2_test.go
index 9800dfd..4bf0981 100644
--- a/tpm2/tpm2_test.go
+++ b/tpm2/tpm2_test.go
@@ -179,7 +179,7 @@ func TestCombinedKeyTest(t *testing.T) {
 	}
 	defer FlushContext(rw, parentHandle)
 
-	_, privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, defaultPassword, defaultPassword, defaultKeyParams)
+	privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, defaultPassword, defaultPassword, defaultKeyParams)
 	if err != nil {
 		t.Fatalf("CreateKey failed: %s", err)
 	}
@@ -205,7 +205,7 @@ func TestCombinedEndorsementTest(t *testing.T) {
 	}
 	defer FlushContext(rw, parentHandle)
 
-	_, privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, emptyPassword, defaultPassword, defaultKeyParams)
+	privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, emptyPassword, defaultPassword, defaultKeyParams)
 	if err != nil {
 		t.Fatalf("CreateKey failed: %s", err)
 	}
@@ -313,7 +313,7 @@ func TestCombinedContextTest(t *testing.T) {
 	defer FlushContext(rw, rootHandle)
 
 	// CreateKey (Quote Key)
-	_, quotePrivate, quotePublic, _, _, _, err := CreateKey(rw, rootHandle, pcrSelection7, emptyPassword, emptyPassword, defaultKeyParams)
+	quotePrivate, quotePublic, _, _, _, err := CreateKey(rw, rootHandle, pcrSelection7, emptyPassword, emptyPassword, defaultKeyParams)
 	if err != nil {
 		t.Fatalf("CreateKey failed: %v", err)
 	}
@@ -347,7 +347,7 @@ func TestEvictControl(t *testing.T) {
 	defer FlushContext(rw, rootHandle)
 
 	// CreateKey (Quote Key)
-	_, quotePrivate, quotePublic, _, _, _, err := CreateKey(rw, rootHandle, pcrSelection7, emptyPassword, emptyPassword, defaultKeyParams)
+	quotePrivate, quotePublic, _, _, _, err := CreateKey(rw, rootHandle, pcrSelection7, emptyPassword, emptyPassword, defaultKeyParams)
 	if err != nil {
 		t.Fatalf("CreateKey failed: %v", err)
 	}
@@ -1229,7 +1229,7 @@ func TestEncryptDecrypt(t *testing.T) {
 		t.Fatalf("CreatePrimary failed: %s", err)
 	}
 	defer FlushContext(rw, parentHandle)
-	_, privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, defaultPassword, defaultPassword, Public{
+	privateBlob, publicBlob, _, _, _, err := CreateKey(rw, parentHandle, pcrSelection7, defaultPassword, defaultPassword, Public{
 		Type:       AlgSymCipher,
 		NameAlg:    AlgSHA256,
 		Attributes: FlagDecrypt | FlagSign | FlagUserWithAuth | FlagFixedParent | FlagFixedTPM | FlagSensitiveDataOrigin,