From 4b251f3ce1f0a0472ed10a00aeef70c69ba03a5d Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Mon, 3 Jan 2022 15:18:59 +0100 Subject: [PATCH] readpassword: bubble up errors instead of exiting the process This allows cleanups to happen in the caller, like removing the control socket. Fixes https://github.com/rfjakob/gocryptfs/issues/634 --- gocryptfs-xray/xray_main.go | 6 +- init_dir.go | 6 +- internal/readpassword/extpass_test.go | 73 +++++++++++++----------- internal/readpassword/passfile.go | 28 +++++----- internal/readpassword/passfile_test.go | 76 +++++++++---------------- internal/readpassword/read.go | 77 +++++++++++++------------- internal/readpassword/stdin_test.go | 33 ++++++++--- main.go | 14 ++++- masterkey.go | 8 ++- 9 files changed, 171 insertions(+), 150 deletions(-) diff --git a/gocryptfs-xray/xray_main.go b/gocryptfs-xray/xray_main.go index 35f409f..534a400 100644 --- a/gocryptfs-xray/xray_main.go +++ b/gocryptfs-xray/xray_main.go @@ -148,7 +148,11 @@ func dumpMasterKey(fn string, fido2Path string) { } pw = fido2.Secret(fido2Path, cf.FIDO2.CredentialID, cf.FIDO2.HMACSalt) } else { - pw = readpassword.Once(nil, nil, "") + pw, err = readpassword.Once(nil, nil, "") + if err != nil { + tlog.Fatal.Println(err) + os.Exit(exitcodes.ReadPassword) + } } masterkey, err := cf.DecryptMasterKey(pw) // Purge password from memory diff --git a/init_dir.go b/init_dir.go index 9658bab..5ade692 100644 --- a/init_dir.go +++ b/init_dir.go @@ -87,7 +87,11 @@ func initDir(args *argContainer) { password = fido2.Secret(args.fido2, fido2CredentialID, fido2HmacSalt) } else { // normal password entry - password = readpassword.Twice([]string(args.extpass), []string(args.passfile)) + password, err = readpassword.Twice([]string(args.extpass), []string(args.passfile)) + if err != nil { + tlog.Fatal.Println(err) + os.Exit(exitcodes.ReadPassword) + } fido2CredentialID = nil fido2HmacSalt = nil } diff --git a/internal/readpassword/extpass_test.go b/internal/readpassword/extpass_test.go index 5c23ab3..d38f42f 100644 --- a/internal/readpassword/extpass_test.go +++ b/internal/readpassword/extpass_test.go @@ -2,7 +2,6 @@ package readpassword import ( "os" - "os/exec" "testing" "github.com/rfjakob/gocryptfs/v2/internal/tlog" @@ -16,68 +15,76 @@ func TestMain(m *testing.M) { func TestExtpass(t *testing.T) { p1 := "ads2q4tw41reg52" - p2 := string(readPasswordExtpass([]string{"echo " + p1})) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := readPasswordExtpass([]string{"echo " + p1}) + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } func TestOnceExtpass(t *testing.T) { p1 := "lkadsf0923rdfi48rqwhdsf" - p2 := string(Once([]string{"echo " + p1}, nil, "")) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := Once([]string{"echo " + p1}, nil, "") + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } // extpass with two arguments func TestOnceExtpass2(t *testing.T) { p1 := "foo" - p2 := string(Once([]string{"echo", p1}, nil, "")) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := Once([]string{"echo", p1}, nil, "") + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } // extpass with three arguments func TestOnceExtpass3(t *testing.T) { p1 := "foo bar baz" - p2 := string(Once([]string{"echo", "foo", "bar", "baz"}, nil, "")) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := Once([]string{"echo", "foo", "bar", "baz"}, nil, "") + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } func TestOnceExtpassSpaces(t *testing.T) { p1 := "mypassword" - p2 := string(Once([]string{"cat", "passfile_test_files/file with spaces.txt"}, nil, "")) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := Once([]string{"cat", "passfile_test_files/file with spaces.txt"}, nil, "") + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } func TestTwiceExtpass(t *testing.T) { p1 := "w5w44t3wfe45srz434" - p2 := string(Once([]string{"echo " + p1}, nil, "")) - if p1 != p2 { - t.Errorf("p1=%q != p2=%q", p1, p2) + p2, err := Once([]string{"echo " + p1}, nil, "") + if err != nil { + t.Fatal(err) + } + if p1 != string(p2) { + t.Errorf("p1=%q != p2=%q", p1, string(p2)) } } -// When extpass returns an empty string, we should crash. -// -// The TEST_SLAVE magic is explained at -// https://talks.golang.org/2014/testing.slide#23 . +// Empty extpass should fail func TestExtpassEmpty(t *testing.T) { - if os.Getenv("TEST_SLAVE") == "1" { - readPasswordExtpass([]string{"echo"}) - return + _, err := readPasswordExtpass([]string{"echo"}) + if err == nil { + t.Fatal("empty password should have failed") } - cmd := exec.Command(os.Args[0], "-test.run=TestExtpassEmpty$") - cmd.Env = append(os.Environ(), "TEST_SLAVE=1") - err := cmd.Run() - if err != nil { - return - } - t.Fatal("empty password should have failed") } diff --git a/internal/readpassword/passfile.go b/internal/readpassword/passfile.go index 29fde6c..60902b7 100644 --- a/internal/readpassword/passfile.go +++ b/internal/readpassword/passfile.go @@ -2,28 +2,31 @@ package readpassword import ( "bytes" + "fmt" "os" - "github.com/rfjakob/gocryptfs/v2/internal/exitcodes" "github.com/rfjakob/gocryptfs/v2/internal/tlog" ) // readPassFileConcatenate reads the first line from each file name and // concatenates the results. The result does not contain any newlines. -func readPassFileConcatenate(passfileSlice []string) (result []byte) { +func readPassFileConcatenate(passfileSlice []string) (result []byte, err error) { for _, e := range passfileSlice { - result = append(result, readPassFile(e)...) + add, err := readPassFile(e) + if err != nil { + return nil, err + } + result = append(result, add...) } - return result + return result, nil } // readPassFile reads the first line from the passed file name. -func readPassFile(passfile string) []byte { +func readPassFile(passfile string) ([]byte, error) { 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) + return nil, fmt.Errorf("fatal: passfile: could not open %q: %v", passfile, err) } defer f.Close() // +1 for an optional trailing newline, @@ -31,23 +34,20 @@ func readPassFile(passfile string) []byte { 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) + return nil, fmt.Errorf("fatal: passfile: could not read from %q: %v", passfile, err) } 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) + return nil, fmt.Errorf("fatal: passfile: empty first line in %q", passfile) } if len(lines[0]) > maxPasswordLen { - tlog.Fatal.Printf("fatal: passfile: max password length (%d bytes) exceeded", maxPasswordLen) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("fatal: passfile: max password length (%d bytes) exceeded", maxPasswordLen) } if len(lines) > 1 && len(lines[1]) > 0 { tlog.Warn.Printf("warning: passfile: ignoring trailing garbage (%d bytes) after first line", len(lines[1])) } - return lines[0] + return lines[0], nil } diff --git a/internal/readpassword/passfile_test.go b/internal/readpassword/passfile_test.go index dbfe159..425e4e5 100644 --- a/internal/readpassword/passfile_test.go +++ b/internal/readpassword/passfile_test.go @@ -1,8 +1,6 @@ package readpassword import ( - "os" - "os/exec" "testing" ) @@ -17,76 +15,49 @@ func TestPassfile(t *testing.T) { {"file with spaces.txt", "mypassword"}, } for _, tc := range testcases { - pw := readPassFile("passfile_test_files/" + tc.file) + pw, err := readPassFile("passfile_test_files/" + tc.file) + if err != nil { + t.Fatal(err) + } if string(pw) != tc.want { t.Errorf("Wrong result: want=%q have=%q", tc.want, pw) } // Calling readPassFileConcatenate with only one element should give the // same result - pw = readPassFileConcatenate([]string{"passfile_test_files/" + tc.file}) + pw, err = readPassFileConcatenate([]string{"passfile_test_files/" + tc.file}) + if err != nil { + t.Fatal(err) + } 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 , mirror: -// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23 +// readPassFile() should fail instead of returning an empty string. func TestPassfileEmpty(t *testing.T) { - if os.Getenv("TEST_SLAVE") == "1" { - readPassFile("passfile_test_files/empty.txt") - return + _, err := readPassFile("passfile_test_files/empty.txt") + if err == nil { + t.Fatal("should have failed") } - 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 , mirror: -// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23 +// readPassFile() should fal instead of returning an empty string. func TestPassfileNewline(t *testing.T) { - if os.Getenv("TEST_SLAVE") == "1" { - readPassFile("passfile_test_files/newline.txt") - return + _, err := readPassFile("passfile_test_files/newline.txt") + if err == nil { + t.Fatal("should have failed") } - cmd := exec.Command(os.Args[0], "-test.run=TestPassfileNewline$") - 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 , mirror: -// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23 +// readPassFile() should return an error. func TestPassfileEmptyFirstLine(t *testing.T) { - if os.Getenv("TEST_SLAVE") == "1" { - readPassFile("passfile_test_files/empty_first_line.txt") - return + _, err := readPassFile("passfile_test_files/empty_first_line.txt") + if err == nil { + t.Fatal("should have failed") } - 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") } // TestPassFileConcatenate tests readPassFileConcatenate @@ -95,8 +66,11 @@ func TestPassFileConcatenate(t *testing.T) { "passfile_test_files/file with spaces.txt", "passfile_test_files/mypassword_garbage.txt", } - res := string(readPassFileConcatenate(files)) - if res != "mypasswordmypassword" { + res, err := readPassFileConcatenate(files) + if err != nil { + t.Fatal(err) + } + if string(res) != "mypasswordmypassword" { t.Errorf("wrong result: %q", res) } } diff --git a/internal/readpassword/read.go b/internal/readpassword/read.go index c0dce43..498d09b 100644 --- a/internal/readpassword/read.go +++ b/internal/readpassword/read.go @@ -11,7 +11,6 @@ import ( "golang.org/x/crypto/ssh/terminal" - "github.com/rfjakob/gocryptfs/v2/internal/exitcodes" "github.com/rfjakob/gocryptfs/v2/internal/tlog" ) @@ -22,7 +21,7 @@ const ( // Once tries to get a password from the user, either from the terminal, extpass, passfile // or stdin. Leave "prompt" empty to use the default "Password: " prompt. -func Once(extpass []string, passfile []string, prompt string) []byte { +func Once(extpass []string, passfile []string, prompt string) ([]byte, error) { if len(passfile) != 0 { return readPassFileConcatenate(passfile) } @@ -40,7 +39,7 @@ func Once(extpass []string, passfile []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, passfile []string) []byte { +func Twice(extpass []string, passfile []string) ([]byte, error) { if len(passfile) != 0 { return readPassFileConcatenate(passfile) } @@ -50,54 +49,59 @@ func Twice(extpass []string, passfile []string) []byte { if !terminal.IsTerminal(int(os.Stdin.Fd())) { return readPasswordStdin("Password") } - p1 := readPasswordTerminal("Password: ") - p2 := readPasswordTerminal("Repeat: ") + p1, err := readPasswordTerminal("Password: ") + if err != nil { + return nil, err + } + p2, err := readPasswordTerminal("Repeat: ") + if err != nil { + return nil, err + } if !bytes.Equal(p1, p2) { - tlog.Fatal.Println("Passwords do not match") - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("Passwords do not match") } // Wipe the password duplicate from memory for i := range p2 { p2[i] = 0 } - return p1 + return p1, nil } // readPasswordTerminal reads a line from the terminal. // Exits on read error or empty result. -func readPasswordTerminal(prompt string) []byte { +func readPasswordTerminal(prompt string) ([]byte, error) { fd := int(os.Stdin.Fd()) fmt.Fprintf(os.Stderr, prompt) // terminal.ReadPassword removes the trailing newline p, err := terminal.ReadPassword(fd) if err != nil { - tlog.Fatal.Printf("Could not read password from terminal: %v\n", err) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("Could not read password from terminal: %v\n", err) } fmt.Fprintf(os.Stderr, "\n") if len(p) == 0 { - tlog.Fatal.Println("Password is empty") - os.Exit(exitcodes.PasswordEmpty) + return nil, fmt.Errorf("Password is empty") } - return p + return p, nil } // readPasswordStdin reads a line from stdin. // It exits with a fatal error on read error or empty result. -func readPasswordStdin(prompt string) []byte { +func readPasswordStdin(prompt string) ([]byte, error) { tlog.Info.Printf("Reading %s from stdin", prompt) - p := readLineUnbuffered(os.Stdin) - if len(p) == 0 { - tlog.Fatal.Printf("Got empty %s from stdin", prompt) - os.Exit(exitcodes.ReadPassword) + p, err := readLineUnbuffered(os.Stdin) + if err != nil { + return nil, err } - return p + if len(p) == 0 { + return nil, fmt.Errorf("Got empty %s from stdin", prompt) + } + return p, nil } // readPasswordExtpass executes the "extpass" program and returns the first line // of the output. // Exits on read error or empty result. -func readPasswordExtpass(extpass []string) []byte { +func readPasswordExtpass(extpass []string) ([]byte, error) { var parts []string if len(extpass) == 1 { parts = strings.Split(extpass[0], " ") @@ -109,50 +113,47 @@ func readPasswordExtpass(extpass []string) []byte { cmd.Stderr = os.Stderr pipe, err := cmd.StdoutPipe() if err != nil { - tlog.Fatal.Printf("extpass pipe setup failed: %v", err) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("extpass pipe setup failed: %v", err) } err = cmd.Start() if err != nil { - tlog.Fatal.Printf("extpass cmd start failed: %v", err) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("extpass cmd start failed: %v", err) + } + p, err := readLineUnbuffered(pipe) + if err != nil { + return nil, err } - p := readLineUnbuffered(pipe) pipe.Close() err = cmd.Wait() if err != nil { - tlog.Fatal.Printf("extpass program returned an error: %v", err) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("extpass program returned an error: %v", err) } if len(p) == 0 { - tlog.Fatal.Println("extpass: password is empty") - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("extpass: password is empty") } - return p + return p, nil } // readLineUnbuffered reads single bytes from "r" util it gets "\n" or EOF. // The returned string does NOT contain the trailing "\n". -func readLineUnbuffered(r io.Reader) (l []byte) { +func readLineUnbuffered(r io.Reader) (l []byte, err error) { b := make([]byte, 1) for { if len(l) > maxPasswordLen { - tlog.Fatal.Printf("fatal: maximum password length of %d bytes exceeded", maxPasswordLen) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("fatal: maximum password length of %d bytes exceeded", maxPasswordLen) } n, err := r.Read(b) if err == io.EOF { - return l + return l, nil } if err != nil { - tlog.Fatal.Printf("readLineUnbuffered: %v", err) - os.Exit(exitcodes.ReadPassword) + return nil, fmt.Errorf("readLineUnbuffered: %v", err) } if n == 0 { continue } if b[0] == '\n' { - return l + return l, nil } l = append(l, b...) } diff --git a/internal/readpassword/stdin_test.go b/internal/readpassword/stdin_test.go index 01dd701..5d5d846 100644 --- a/internal/readpassword/stdin_test.go +++ b/internal/readpassword/stdin_test.go @@ -8,12 +8,20 @@ import ( ) // Provide password via stdin, terminated by "\n". +// +// The TEST_SLAVE magic is explained at +// https://talks.golang.org/2014/testing.slide#23 , mirror: +// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23 func TestStdin(t *testing.T) { p1 := "g55434t55wef" if os.Getenv("TEST_SLAVE") == "1" { - p2 := string(readPasswordStdin("foo")) - if p1 != p2 { - fmt.Fprintf(os.Stderr, "%q != %q", p1, p2) + p2, err := readPasswordStdin("foo") + if err != nil { + fmt.Fprint(os.Stderr, err) + os.Exit(1) + } + if p1 != string(p2) { + fmt.Fprintf(os.Stderr, "%q != %q", p1, string(p2)) os.Exit(1) } return @@ -41,12 +49,20 @@ func TestStdin(t *testing.T) { // Provide password via stdin, terminated by EOF (pipe close). This should not // hang. +// +// The TEST_SLAVE magic is explained at +// https://talks.golang.org/2014/testing.slide#23 , mirror: +// http://web.archive.org/web/20200426174352/https://talks.golang.org/2014/testing.slide#23 func TestStdinEof(t *testing.T) { p1 := "asd45as5f4a36" if os.Getenv("TEST_SLAVE") == "1" { - p2 := string(readPasswordStdin("foo")) - if p1 != p2 { - fmt.Fprintf(os.Stderr, "%q != %q", p1, p2) + p2, err := readPasswordStdin("foo") + if err != nil { + fmt.Fprint(os.Stderr, err) + os.Exit(1) + } + if p1 != string(p2) { + fmt.Fprintf(os.Stderr, "%q != %q", p1, string(p2)) os.Exit(1) } return @@ -76,7 +92,10 @@ func TestStdinEof(t *testing.T) { // Provide empty password via stdin func TestStdinEmpty(t *testing.T) { if os.Getenv("TEST_SLAVE") == "1" { - readPasswordStdin("foo") + _, err := readPasswordStdin("foo") + if err != nil { + os.Exit(1) + } } cmd := exec.Command(os.Args[0], "-test.run=TestStdinEmpty$") cmd.Env = append(os.Environ(), "TEST_SLAVE=1") diff --git a/main.go b/main.go index 0ac7423..d1bf0c3 100644 --- a/main.go +++ b/main.go @@ -55,11 +55,15 @@ func loadConfig(args *argContainer) (masterkey []byte, cf *configfile.ConfFile, if cf.IsFeatureFlagSet(configfile.FlagFIDO2) { if args.fido2 == "" { tlog.Fatal.Printf("Masterkey encrypted using FIDO2 token; need to use the --fido2 option.") - os.Exit(exitcodes.Usage) + return nil, nil, exitcodes.NewErr("", exitcodes.Usage) } pw = fido2.Secret(args.fido2, cf.FIDO2.CredentialID, cf.FIDO2.HMACSalt) } else { - pw = readpassword.Once([]string(args.extpass), []string(args.passfile), "") + pw, err = readpassword.Once([]string(args.extpass), []string(args.passfile), "") + if err != nil { + tlog.Fatal.Println(err) + return nil, nil, exitcodes.NewErr("", exitcodes.ReadPassword) + } } tlog.Info.Println("Decrypting master key") masterkey, err = cf.DecryptMasterKey(pw) @@ -93,7 +97,11 @@ func changePassword(args *argContainer) { os.Exit(exitcodes.Usage) } tlog.Info.Println("Please enter your new password.") - newPw := readpassword.Twice([]string(args.extpass), []string(args.passfile)) + newPw, err := readpassword.Twice([]string(args.extpass), []string(args.passfile)) + if err != nil { + tlog.Fatal.Println(err) + os.Exit(exitcodes.ReadPassword) + } logN := confFile.ScryptObject.LogN() if args._explicitScryptn { logN = args.scryptn diff --git a/masterkey.go b/masterkey.go index 10009cb..d488441 100644 --- a/masterkey.go +++ b/masterkey.go @@ -39,8 +39,12 @@ func unhexMasterKey(masterkey string, fromStdin bool) []byte { func handleArgsMasterkey(args *argContainer) (masterkey []byte) { // "-masterkey=stdin" if args.masterkey == "stdin" { - in := string(readpassword.Once(nil, nil, "Masterkey")) - return unhexMasterKey(in, true) + in, err := readpassword.Once(nil, nil, "Masterkey") + if err != nil { + tlog.Fatal.Println(err) + os.Exit(exitcodes.ReadPassword) + } + return unhexMasterKey(string(in), true) } // "-masterkey=941a6029-3adc6a1c-..." if args.masterkey != "" {