Repository: google/go-tpm
Pull request #97 (2019-04-30 17:15:56)

tpm2: actually fail when tpmutil.RunCommand returns an error
This is embarrassing :(
tpmutil.RCSuccess matching the zero value doesn't help either.


Patch:
diff --git a/tpm2/tpm2.go b/tpm2/tpm2.go
index b34bfa4..5ae0064 100644
--- a/tpm2/tpm2.go
+++ b/tpm2/tpm2.go
@@ -1308,7 +1308,10 @@ 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 || code != tpmutil.RCSuccess {
+	if err != nil {
+		return nil, err
+	}
+	if code != tpmutil.RCSuccess {
 		return nil, decodeResponse(code)
 	}
 	return resp, decodeResponse(code)
diff --git a/tpmutil/run_other.go b/tpmutil/run_other.go
index 83fc646..1b8dc2e 100644
--- a/tpmutil/run_other.go
+++ b/tpmutil/run_other.go
@@ -42,14 +42,70 @@ func OpenTPM(path string) (io.ReadWriteCloser, error) {
 		}
 		rwc = io.ReadWriteCloser(f)
 	} else if fi.Mode()&os.ModeSocket != 0 {
-		uc, err := net.DialUnix("unix", nil, &net.UnixAddr{Name: path, Net: "unix"})
-		if err != nil {
-			return nil, err
-		}
-		rwc = io.ReadWriteCloser(uc)
+		rwc = NewEmulatorReadWriteCloser(path)
 	} else {
 		return nil, fmt.Errorf("unsupported TPM file mode %s", fi.Mode().String())
 	}
 
 	return rwc, nil
 }
+
+// dialer abstracts the net.Dial call so test code can provide its own net.Conn
+// implementation.
+type dialer func(network, path string) (net.Conn, error)
+
+// EmulatorReadWriteCloser manages connections with a TPM emulator over a Unix
+// domain socket. These emulators often operate in a write/read/disconnect
+// sequence, so the Write method always connects, and the Read method always
+// closes. EmulatorReadWriteCloser is not thread safe.
+type EmulatorReadWriteCloser struct {
+	path   string
+	conn   net.Conn
+	dialer dialer
+}
+
+// NewEmulatorReadWriteCloser stores information about a Unix domain socket to
+// write to and read from.
+func NewEmulatorReadWriteCloser(path string) *EmulatorReadWriteCloser {
+	return &EmulatorReadWriteCloser{
+		path:   path,
+		dialer: net.Dial,
+	}
+}
+
+// Read implements io.Reader by reading from the Unix domain socket and closing
+// it.
+func (erw *EmulatorReadWriteCloser) Read(p []byte) (int, error) {
+	// Read is always the second operation in a Write/Read sequence.
+	if erw.conn == nil {
+		return 0, fmt.Errorf("Must call Write then Read in an alternating sequence")
+	}
+	n, err := erw.conn.Read(p)
+	erw.conn.Close()
+	erw.conn = nil
+	return n, err
+}
+
+// Write implements io.Writer by connecting to the Unix domain socket and
+// writing.
+func (erw *EmulatorReadWriteCloser) Write(p []byte) (int, error) {
+	if erw.conn != nil {
+		return 0, fmt.Errorf("Must call Write then Read in an alternating sequence")
+	}
+	var err error
+	erw.conn, err = erw.dialer("unix", erw.path)
+	if err != nil {
+		return 0, err
+	}
+	return erw.conn.Write(p)
+}
+
+// Close implements io.Closer by closing the Unix domain socket if one is open.
+func (erw *EmulatorReadWriteCloser) Close() error {
+	if erw.conn == nil {
+		return fmt.Errorf("Cannot call Close when no connection is open")
+	}
+	err := erw.conn.Close()
+	erw.conn = nil
+	return err
+}


Test patch:
diff --git a/tpm2/tpm2_test.go b/tpm2/tpm2_test.go
index 41f322a..54fe933 100644
--- a/tpm2/tpm2_test.go
+++ b/tpm2/tpm2_test.go
@@ -99,6 +99,14 @@ var (
 	emptyPassword   = ""
 )
 
+func TestRunCommandErr(t *testing.T) {
+	// nil ReadWriter handle will cause tpmutil.RunCommand to return an error
+	// immediately.
+	if _, err := runCommand(nil, TagSessions, cmdSign); err == nil {
+		t.Error("runCommand returned nil error on error from tpmutil.RunCommand")
+	}
+}
+
 func TestGetRandom(t *testing.T) {
 	rw := openTPM(t)
 	defer rw.Close()
diff --git a/tpmutil/emulator_read_write_closer_test.go b/tpmutil/emulator_read_write_closer_test.go
new file mode 100644
index 0000000..542e0c5
--- /dev/null
+++ b/tpmutil/emulator_read_write_closer_test.go
@@ -0,0 +1,197 @@
+// +build !windows
+
+// Copyright (c) 2019, Google LLC All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package tpmutil
+
+import (
+	"fmt"
+	"net"
+	"testing"
+	"time"
+)
+
+// mockConn records the number of bytes that are read from it and written to it
+// and tracks whether or not it has been closed.
+type mockConn struct {
+	network string
+	path    string
+	open    bool
+}
+
+// dialMockConn returns a mockConn that holds the given network and path info.
+func dialMockConn(network, path string) (net.Conn, error) {
+	return &mockConn{
+		network: network,
+		path:    path,
+		open:    true,
+	}, nil
+}
+
+// Read implements a mock version of Read.
+func (mc *mockConn) Read(b []byte) (int, error) {
+	// Always read zeros into the bytes for the given length.
+	for i := range b {
+		b[i] = 0
+	}
+	return len(b), nil
+}
+
+// Write implements a mock version of Write.
+func (mc *mockConn) Write(b []byte) (int, error) {
+	return len(b), nil
+}
+
+// Close implements a mock version of Close.
+func (mc *mockConn) Close() error {
+	if !mc.open {
+		return fmt.Errorf("mockConn is already closed")
+	}
+	mc.open = false
+	return nil
+}
+
+// LocalAddr returns nil.
+func (mc *mockConn) LocalAddr() net.Addr {
+	return nil
+}
+
+// RemoteAddr returns nil.
+func (mc *mockConn) RemoteAddr() net.Addr {
+	return nil
+}
+
+// SetDeadline returns nil.
+func (mc *mockConn) SetDeadline(t time.Time) error {
+	return nil
+}
+
+// SetReadDeadline returns nil.
+func (mc *mockConn) SetReadDeadline(t time.Time) error {
+	return nil
+}
+
+// SetWriteDeadline returns nil.
+func (mc *mockConn) SetWriteDeadline(t time.Time) error {
+	return nil
+}
+
+func newMockEmulator() *EmulatorReadWriteCloser {
+	path := "/dev/null/fake"
+	rwc := NewEmulatorReadWriteCloser(path)
+	rwc.dialer = dialMockConn
+	return rwc
+}
+
+var (
+	input  = []byte(`input`)
+	output = make([]byte, 1)
+)
+
+func TestEmulatorReadWriteCloserMultipleReads(t *testing.T) {
+	rwc := newMockEmulator()
+	n, err := rwc.Write(input)
+	if err != nil {
+		t.Errorf("failed to write: %v", err)
+	}
+	if n != len(input) {
+		t.Errorf("wrong number of bytes written: got %d, expected %d", n, len(input))
+	}
+
+	n, err = rwc.Read(output)
+	if err != nil {
+		t.Errorf("failed to read: %v", err)
+	}
+	if n != len(output) {
+		t.Errorf("wrong number of bytes read: got %d, expected %d", n, len(output))
+	}
+
+	n, err = rwc.Write(input)
+	if err != nil {
+		t.Errorf("failed to write: %v", err)
+	}
+	if n != len(input) {
+		t.Errorf("wrong number of bytes written: got %d, expected %d", n, len(input))
+	}
+
+	n, err = rwc.Read(output)
+	if err != nil {
+		t.Errorf("failed to read: %v", err)
+	}
+	if n != len(output) {
+		t.Errorf("wrong number of bytes read: got %d, expected %d", n, len(output))
+	}
+}
+
+func TestEmulatorReadWriteCloserClose(t *testing.T) {
+	rwc := newMockEmulator()
+	if err := rwc.Close(); err == nil {
+		t.Errorf("incorrectly closed a connection that hadn't been opened")
+	}
+
+	if _, err := rwc.Write(input); err != nil {
+		t.Errorf("failed to write: %v", err)
+	}
+
+	if err := rwc.Close(); err != nil {
+		t.Errorf("failed to close an open connection: %v", err)
+	}
+
+	if err := rwc.Close(); err == nil {
+		t.Errorf("incorrectly closed a connection that had already been closed")
+	}
+}
+
+func TestEmulatorReadWriteCloseReadAfterClose(t *testing.T) {
+	rwc := newMockEmulator()
+	if _, err := rwc.Write(input); err != nil {
+		t.Errorf("failed to write: %v", err)
+	}
+
+	if err := rwc.Close(); err != nil {
+		t.Errorf("failed to close the connection: %v", err)
+	}
+
+	if _, err := rwc.Read(output); err == nil {
+		t.Errorf("incorrectly read on a closed connection")
+	}
+}
+
+func TestEmulatorReadWriteCloserReadBeforeWrite(t *testing.T) {
+	rwc := newMockEmulator()
+	b := make([]byte, 1)
+	if _, err := rwc.Read(b); err == nil {
+		t.Errorf("incorrectly read on a connection before writing")
+	}
+}
+
+func TestEmulatorReadWriteCloserDoubleWrite(t *testing.T) {
+	rwc := newMockEmulator()
+	if _, err := rwc.Write(input); err != nil {
+		t.Errorf("failed to write: %v", err)
+	}
+	if _, err := rwc.Write(input); err == nil {
+		t.Errorf("incorrectly wrote a second time without reading in between")
+	}
+}
+
+func TestEmulatorReadWriteCloserDialerError(t *testing.T) {
+	rwc := newMockEmulator()
+	rwc.dialer = func(_, _ string) (net.Conn, error) { return nil, fmt.Errorf("invalid") }
+
+	if _, err := rwc.Write(input); err == nil {
+		t.Errorf("incorrectly wrote when the dialer returned an error")
+	}
+}