Improve password handling
In many cases we get passwords from configuration files and they're all strings somewhere in memory, this can't really be improved. But we also get passwords from the CLI in most of regular commands and they can be cleaned after use, except our input.ReadPassword returns a string that can't be changed. We can do some
--- a/cli/input/input.go
+++ b/cli/input/input.go
@@ -47,10 +47,14 @@ func readLine(trm *term.Terminal, prompt string) (string, error) {
}
// ReadPassword reads the user's password with prompt.
-func ReadPassword(prompt string) (string, error) {
+func ReadPassword(prompt string) ([]byte, error) {
trm := Terminal
- if trm != nil {
- return trm.ReadPassword(prompt)
+ if trm != nil { // Tests only.
+ s, err := trm.ReadPassword(prompt)
+ if err != nil {
+ return nil, err
+ }
+ return []byte(s), nil
}
return readSecurePassword(prompt)
}
--- a/cli/input/readpass_unix.go
+++ b/cli/input/readpass_unix.go
@@ -10,20 +10,20 @@ import (
)
// readSecurePassword reads the user's password with prompt directly from /dev/tty.
-func readSecurePassword(prompt string) (string, error) {
+func readSecurePassword(prompt string) ([]byte, error) {
f, err := os.OpenFile("/dev/tty", os.O_RDWR, 0)
if err != nil {
- return "", err
+ return nil, err
}
defer f.Close()
_, err = f.WriteString(prompt)
if err != nil {
- return "", err
+ return nil, err
}
pass, err := term.ReadPassword(int(f.Fd()))
if err != nil {
- return "", fmt.Errorf("failed to read password: %w", err)
+ return nil, fmt.Errorf("failed to read password: %w", err)
}
_, err = f.WriteString("\n")
- return string(pass), err
+ return pass, err
}
--- a/cli/input/readpass_windows.go
+++ b/cli/input/readpass_windows.go
@@ -4,18 +4,20 @@ package input
import (
"os"
- "syscall"
"golang.org/x/term"
)
// readSecurePassword reads the user's password with prompt.
-func readSecurePassword(prompt string) (string, error) {
- s, err := term.MakeRaw(int(syscall.Stdin))
+func readSecurePassword(prompt string) ([]byte, error) {
+ _, err := os.Stdout.WriteString(prompt)
if err != nil {
- return "", err
+ return nil, err
}
- defer func() { _ = term.Restore(int(syscall.Stdin), s) }()
- trm := term.NewTerminal(ReadWriter{os.Stdin, os.Stdout}, prompt)
- return trm.ReadPassword(prompt)
+ p, err := term.ReadPassword(os.Stdin.Fd())
+ if err != nil {
+ return nil, err
+ }
+ _, err = os.Stdout.WriteString("\n")
+ return p, err
}
But it's not enough, we then have an (*Account).Decrypt that also wants a string, keys.NEP2Decrypt that accepts strings (and converts them immediately to []byte), so we have a huge set of changes before long. And then we have external code like neofs-node or neofs-http-gw that happily calls (*Account).Decrypt with a string and would be surprised (hi, @fyrchik, @alexvanin) if it to be changed to accept []byte.
The question mostly is, are we paranoid enough to do this at all?
External code of NeoFS services also reads passwords from config in many cases.
Personally, I won't be upset with s/passwd/[]byte(passwd)/g if it reduces security risks.
It's not that it's the end of world problem, but removing sensitive data from memory when it's no longer needed is a (very) good practice.
The https://github.com/nspcc-dev/neo-go/pull/2887#discussion_r1086394998 should also be fixed within the scope of this issue.