From 295d432175292dbaef572093d784aab55f5c0b8f Mon Sep 17 00:00:00 2001
From: Jakob Unterwurzacher <jakobunt@gmail.com>
Date: Sat, 15 Dec 2018 17:09:38 +0100
Subject: [PATCH] passfile: directly read file instead of invoking cat

Allows better error handling, gets rid of the call to an
external program, and fixes https://github.com/rfjakob/gocryptfs/issues/278 .
---
 Documentation/MANPAGE.md                      |  9 ++-
 cli_args.go                                   | 10 ++-
 gocryptfs-xray/xray_main.go                   |  2 +-
 init_dir.go                                   |  2 +-
 internal/readpassword/extpass_test.go         |  8 +-
 internal/readpassword/passfile.go             | 43 ++++++++++
 internal/readpassword/passfile_test.go        | 80 +++++++++++++++++++
 .../passfile_test_files/empty.txt             |  0
 .../passfile_test_files/empty_first_line.txt  |  2 +
 .../passfile_test_files/mypassword.txt        |  1 +
 .../mypassword_garbage.txt                    |  2 +
 .../mypassword_missing_newline.txt            |  1 +
 .../passfile_test_files/newline.txt           |  1 +
 internal/readpassword/read.go                 | 21 +++--
 main.go                                       |  4 +-
 masterkey.go                                  |  2 +-
 16 files changed, 163 insertions(+), 25 deletions(-)
 create mode 100644 internal/readpassword/passfile.go
 create mode 100644 internal/readpassword/passfile_test.go
 create mode 100644 internal/readpassword/passfile_test_files/empty.txt
 create mode 100644 internal/readpassword/passfile_test_files/empty_first_line.txt
 create mode 100644 internal/readpassword/passfile_test_files/mypassword.txt
 create mode 100644 internal/readpassword/passfile_test_files/mypassword_garbage.txt
 create mode 100644 internal/readpassword/passfile_test_files/mypassword_missing_newline.txt
 create mode 100644 internal/readpassword/passfile_test_files/newline.txt

diff --git a/Documentation/MANPAGE.md b/Documentation/MANPAGE.md
index baadb25..5ebb0b4 100644
--- a/Documentation/MANPAGE.md
+++ b/Documentation/MANPAGE.md
@@ -262,8 +262,13 @@ you are using Go 1.6+. In mode "auto", gocrypts chooses the faster
 option.
 
 #### -passfile string
-Read password from the specified file. This is a shortcut for
-specifying '-extpass="/bin/cat -- FILE"'.
+Read password from the specified file. A warning will be printed if there
+is more than one line, and only the first line will be used. A single
+trailing newline is allowed and does not cause a warning.
+
+Before gocryptfs v1.7, using `-passfile` was equivant to writing
+`-extpass="/bin/cat -- FILE"`.
+gocryptfs v1.7 and later directly read the file without invoking `cat`.
 
 #### -passwd
 Change the password. Will ask for the old password, check if it is
diff --git a/cli_args.go b/cli_args.go
index c073958..b3f2834 100644
--- a/cli_args.go
+++ b/cli_args.go
@@ -243,9 +243,13 @@ func parseCliOpts() (args argContainer) {
 		args.allow_other = false
 		args.ko = "noexec"
 	}
-	// '-passfile FILE' is a shortcut for -extpass='/bin/cat -- FILE'
-	if args.passfile != "" {
-		args.extpass = "/bin/cat -- " + args.passfile
+	if args.extpass != "" && args.passfile != "" {
+		tlog.Fatal.Printf("The options -extpass and -passfile cannot be used at the same time")
+		os.Exit(exitcodes.Usage)
+	}
+	if args.passfile != "" && args.masterkey != "" {
+		tlog.Fatal.Printf("The options -passfile and -masterkey cannot be used at the same time")
+		os.Exit(exitcodes.Usage)
 	}
 	if args.extpass != "" && args.masterkey != "" {
 		tlog.Fatal.Printf("The options -extpass and -masterkey cannot be used at the same time")
diff --git a/gocryptfs-xray/xray_main.go b/gocryptfs-xray/xray_main.go
index 569776d..74c9fb3 100644
--- a/gocryptfs-xray/xray_main.go
+++ b/gocryptfs-xray/xray_main.go
@@ -60,7 +60,7 @@ func main() {
 
 func dumpMasterKey(fn string) {
 	tlog.Info.Enabled = false
-	pw := readpassword.Once("", "")
+	pw := readpassword.Once("", "", "")
 	masterkey, _, err := configfile.LoadAndDecrypt(fn, pw)
 	if err != nil {
 		fmt.Fprintln(os.Stderr, err)
diff --git a/init_dir.go b/init_dir.go
index 6bc977f..5d5cbc9 100644
--- a/init_dir.go
+++ b/init_dir.go
@@ -78,7 +78,7 @@ func initDir(args *argContainer) {
 			password = readpassword.Trezor(trezorPayload)
 		} else {
 			// Normal password entry
-			password = readpassword.Twice(args.extpass)
+			password = readpassword.Twice(args.extpass, args.passfile)
 			readpassword.CheckTrailingGarbage()
 		}
 		creator := tlog.ProgramName + " " + GitVersion
diff --git a/internal/readpassword/extpass_test.go b/internal/readpassword/extpass_test.go
index b35153f..037d111 100644
--- a/internal/readpassword/extpass_test.go
+++ b/internal/readpassword/extpass_test.go
@@ -26,7 +26,7 @@ func TestExtpass(t *testing.T) {
 
 func TestOnceExtpass(t *testing.T) {
 	p1 := "lkadsf0923rdfi48rqwhdsf"
-	p2 := string(Once("echo "+p1, ""))
+	p2 := string(Once("echo "+p1, "", ""))
 	if p1 != p2 {
 		t.Errorf("p1=%q != p2=%q", p1, p2)
 	}
@@ -34,14 +34,16 @@ func TestOnceExtpass(t *testing.T) {
 
 func TestTwiceExtpass(t *testing.T) {
 	p1 := "w5w44t3wfe45srz434"
-	p2 := string(Once("echo "+p1, ""))
+	p2 := string(Once("echo "+p1, "", ""))
 	if p1 != p2 {
 		t.Errorf("p1=%q != p2=%q", p1, p2)
 	}
 }
 
 // When extpass returns an empty string, we should crash.
-// https://talks.golang.org/2014/testing.slide#23
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 .
 func TestExtpassEmpty(t *testing.T) {
 	if os.Getenv("TEST_SLAVE") == "1" {
 		readPasswordExtpass("echo")
diff --git a/internal/readpassword/passfile.go b/internal/readpassword/passfile.go
new file mode 100644
index 0000000..73af279
--- /dev/null
+++ b/internal/readpassword/passfile.go
@@ -0,0 +1,43 @@
+package readpassword
+
+import (
+	"bytes"
+	"os"
+
+	"github.com/rfjakob/gocryptfs/internal/exitcodes"
+	"github.com/rfjakob/gocryptfs/internal/tlog"
+)
+
+func readPassFile(passfile string) []byte {
+	tlog.Info.Printf("passfile: reading from file %q", passfile)
+	f, err := os.Open(passfile)
+	if err != nil {
+		tlog.Fatal.Printf("fatal: passfile: could not open %q: %v", passfile, err)
+		os.Exit(exitcodes.ReadPassword)
+	}
+	defer f.Close()
+	// +1 for an optional trailing newline,
+	// +2 so we can detect if maxPasswordLen is exceeded.
+	buf := make([]byte, maxPasswordLen+2)
+	n, err := f.Read(buf)
+	if err != nil {
+		tlog.Fatal.Printf("fatal: passfile: could not read from %q: %v", passfile, err)
+		os.Exit(exitcodes.ReadPassword)
+	}
+	buf = buf[:n]
+	// Split into first line and "trailing garbage"
+	lines := bytes.SplitN(buf, []byte("\n"), 2)
+	if len(lines[0]) == 0 {
+		tlog.Fatal.Printf("fatal: passfile: empty first line in %q", passfile)
+		os.Exit(exitcodes.ReadPassword)
+	}
+	if len(lines[0]) > maxPasswordLen {
+		tlog.Fatal.Printf("fatal: passfile: max password length (%d bytes) exceeded", maxPasswordLen)
+		os.Exit(exitcodes.ReadPassword)
+	}
+	if len(lines) > 1 && len(lines[1]) > 0 {
+		tlog.Warn.Printf("passfile: ignoring trailing garbage (%d bytes) after first line",
+			len(lines[1]))
+	}
+	return lines[0]
+}
diff --git a/internal/readpassword/passfile_test.go b/internal/readpassword/passfile_test.go
new file mode 100644
index 0000000..457170b
--- /dev/null
+++ b/internal/readpassword/passfile_test.go
@@ -0,0 +1,80 @@
+package readpassword
+
+import (
+	"os"
+	"os/exec"
+	"testing"
+)
+
+func TestPassfile(t *testing.T) {
+	testcases := []struct {
+		file string
+		want string
+	}{
+		{"mypassword.txt", "mypassword"},
+		{"mypassword_garbage.txt", "mypassword"},
+		{"mypassword_missing_newline.txt", "mypassword"},
+	}
+	for _, tc := range testcases {
+		pw := readPassFile("passfile_test_files/" + tc.file)
+		if string(pw) != tc.want {
+			t.Errorf("Wrong result: want=%q have=%q", tc.want, pw)
+		}
+	}
+}
+
+// readPassFile() should exit instead of returning an empty string.
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 .
+func TestPassfileEmpty(t *testing.T) {
+	if os.Getenv("TEST_SLAVE") == "1" {
+		readPassFile("passfile_test_files/empty.txt")
+		return
+	}
+	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileEmpty$")
+	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
+	err := cmd.Run()
+	if err != nil {
+		return
+	}
+	t.Fatal("should have exited")
+}
+
+// File containing just a newline.
+// readPassFile() should exit instead of returning an empty string.
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 .
+func TestPassfileNewline(t *testing.T) {
+	if os.Getenv("TEST_SLAVE") == "1" {
+		readPassFile("passfile_test_files/newline.txt")
+		return
+	}
+	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileEmpty$")
+	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
+	err := cmd.Run()
+	if err != nil {
+		return
+	}
+	t.Fatal("should have exited")
+}
+
+// File containing "\ngarbage".
+// readPassFile() should exit instead of returning an empty string.
+//
+// The TEST_SLAVE magic is explained at
+// https://talks.golang.org/2014/testing.slide#23 .
+func TestPassfileEmptyFirstLine(t *testing.T) {
+	if os.Getenv("TEST_SLAVE") == "1" {
+		readPassFile("passfile_test_files/empty_first_line.txt")
+		return
+	}
+	cmd := exec.Command(os.Args[0], "-test.run=TestPassfileEmptyFirstLine$")
+	cmd.Env = append(os.Environ(), "TEST_SLAVE=1")
+	err := cmd.Run()
+	if err != nil {
+		return
+	}
+	t.Fatal("should have exited")
+}
diff --git a/internal/readpassword/passfile_test_files/empty.txt b/internal/readpassword/passfile_test_files/empty.txt
new file mode 100644
index 0000000..e69de29
diff --git a/internal/readpassword/passfile_test_files/empty_first_line.txt b/internal/readpassword/passfile_test_files/empty_first_line.txt
new file mode 100644
index 0000000..a607e80
--- /dev/null
+++ b/internal/readpassword/passfile_test_files/empty_first_line.txt
@@ -0,0 +1,2 @@
+
+garbage
diff --git a/internal/readpassword/passfile_test_files/mypassword.txt b/internal/readpassword/passfile_test_files/mypassword.txt
new file mode 100644
index 0000000..48d23cf
--- /dev/null
+++ b/internal/readpassword/passfile_test_files/mypassword.txt
@@ -0,0 +1 @@
+mypassword
diff --git a/internal/readpassword/passfile_test_files/mypassword_garbage.txt b/internal/readpassword/passfile_test_files/mypassword_garbage.txt
new file mode 100644
index 0000000..74ba741
--- /dev/null
+++ b/internal/readpassword/passfile_test_files/mypassword_garbage.txt
@@ -0,0 +1,2 @@
+mypassword
+garbage
diff --git a/internal/readpassword/passfile_test_files/mypassword_missing_newline.txt b/internal/readpassword/passfile_test_files/mypassword_missing_newline.txt
new file mode 100644
index 0000000..b3c42b5
--- /dev/null
+++ b/internal/readpassword/passfile_test_files/mypassword_missing_newline.txt
@@ -0,0 +1 @@
+mypassword
\ No newline at end of file
diff --git a/internal/readpassword/passfile_test_files/newline.txt b/internal/readpassword/passfile_test_files/newline.txt
new file mode 100644
index 0000000..8b13789
--- /dev/null
+++ b/internal/readpassword/passfile_test_files/newline.txt
@@ -0,0 +1 @@
+
diff --git a/internal/readpassword/read.go b/internal/readpassword/read.go
index c99be5d..0378e53 100644
--- a/internal/readpassword/read.go
+++ b/internal/readpassword/read.go
@@ -24,7 +24,10 @@ const (
 
 // Once tries to get a password from the user, either from the terminal, extpass
 // or stdin. Leave "prompt" empty to use the default "Password: " prompt.
-func Once(extpass string, prompt string) []byte {
+func Once(extpass string, passfile string, prompt string) []byte {
+	if passfile != "" {
+		return readPassFile(passfile)
+	}
 	if extpass != "" {
 		return readPasswordExtpass(extpass)
 	}
@@ -39,7 +42,10 @@ func Once(extpass string, prompt string) []byte {
 
 // Twice is the same as Once but will prompt twice if we get the password from
 // the terminal.
-func Twice(extpass string) []byte {
+func Twice(extpass string, passfile string) []byte {
+	if passfile != "" {
+		return readPassFile(passfile)
+	}
 	if extpass != "" {
 		return readPasswordExtpass(extpass)
 	}
@@ -95,16 +101,7 @@ func readPasswordStdin(prompt string) []byte {
 // Exits on read error or empty result.
 func readPasswordExtpass(extpass string) []byte {
 	tlog.Info.Println("Reading password from extpass program")
-	var parts []string
-	// The option "-passfile=FILE" gets transformed to
-	// "-extpass="/bin/cat -- FILE". We don't want to split FILE on spaces,
-	// so let's handle it manually.
-	passfileCat := "/bin/cat -- "
-	if strings.HasPrefix(extpass, passfileCat) {
-		parts = []string{"/bin/cat", "--", extpass[len(passfileCat):]}
-	} else {
-		parts = strings.Split(extpass, " ")
-	}
+	parts := strings.Split(extpass, " ")
 	cmd := exec.Command(parts[0], parts[1:]...)
 	cmd.Stderr = os.Stderr
 	pipe, err := cmd.StdoutPipe()
diff --git a/main.go b/main.go
index fe38a87..48474d5 100644
--- a/main.go
+++ b/main.go
@@ -53,7 +53,7 @@ func loadConfig(args *argContainer) (masterkey []byte, cf *configfile.ConfFile,
 		pw = readpassword.Trezor(cf.TrezorPayload)
 	} else {
 		// Normal password entry
-		pw = readpassword.Once(args.extpass, "")
+		pw = readpassword.Once(args.extpass, args.passfile, "")
 	}
 	tlog.Info.Println("Decrypting master key")
 	masterkey, err = cf.DecryptMasterKey(pw)
@@ -93,7 +93,7 @@ func changePassword(args *argContainer) {
 			log.Panic("empty masterkey")
 		}
 		tlog.Info.Println("Please enter your new password.")
-		newPw := readpassword.Twice(args.extpass)
+		newPw := readpassword.Twice(args.extpass, args.passfile)
 		readpassword.CheckTrailingGarbage()
 		confFile.EncryptKey(masterkey, newPw, confFile.ScryptObject.LogN())
 		for i := range newPw {
diff --git a/masterkey.go b/masterkey.go
index 42a27be..332a673 100644
--- a/masterkey.go
+++ b/masterkey.go
@@ -43,7 +43,7 @@ func getMasterKey(args *argContainer) (masterkey []byte, confFile *configfile.Co
 	masterkeyFromStdin := false
 	// "-masterkey=stdin"
 	if args.masterkey == "stdin" {
-		args.masterkey = string(readpassword.Once("", "Masterkey"))
+		args.masterkey = string(readpassword.Once("", "", "Masterkey"))
 		masterkeyFromStdin = true
 	}
 	// "-masterkey=941a6029-3adc6a1c-..."