Improve password handling #978

New issue

Open

opened 2025-12-28 17:14:49 +00:00 by sami · 3 comments

sami commented

2025-12-28 17:14:49 +00:00

Owner

Originally created by @roman-khimov on GitHub (Sep 1, 2022).

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?

Originally created by @roman-khimov on GitHub (Sep 1, 2022). 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 ```patch --- 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?

sami added the

security

enhancement

labels

2025-12-28 17:14:49 +00:00

sami commented

2025-12-28 17:14:49 +00:00

Author

Owner

@alexvanin commented on GitHub (Sep 8, 2022):

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.

@alexvanin commented on GitHub (Sep 8, 2022): 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.

sami commented

2025-12-28 17:14:49 +00:00

Author

Owner

@roman-khimov commented on GitHub (Sep 8, 2022):

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.

@roman-khimov commented on GitHub (Sep 8, 2022): 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.

sami commented

2025-12-28 17:14:49 +00:00

Author

Owner

@AnnaShaleva commented on GitHub (Jan 26, 2023):

The https://github.com/nspcc-dev/neo-go/pull/2887#discussion_r1086394998 should also be fixed within the scope of this issue.

@AnnaShaleva commented on GitHub (Jan 26, 2023): The https://github.com/nspcc-dev/neo-go/pull/2887#discussion_r1086394998 should also be fixed within the scope of this issue.