From 295d432175292dbaef572093d784aab55f5c0b8f Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 15 Dec 2018 17:09:38 +0100 Subject: 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 ++++++++++++++++++++++ .../readpassword/passfile_test_files/empty.txt | 0 .../passfile_test_files/empty_first_line.txt | 2 + .../passfile_test_files/mypassword.txt | 1 + .../passfile_test_files/mypassword_garbage.txt | 2 + .../mypassword_missing_newline.txt | 1 + .../readpassword/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-..." -- cgit v1.2.3