Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/ssh/terminal: ReadPassword doesn't work on redirected stdin giving inappropriate ioctl for device #19909

Open
ncw opened this issue Apr 10, 2017 · 22 comments

Comments

Projects
None yet
10 participants
@ncw
Copy link
Contributor

commented Apr 10, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.8 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ncw/go"
GORACE=""
GOROOT="/opt/go/go1.8"
GOTOOLDIR="/opt/go/go1.8/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build011975399=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Use ReadPassword with redirected stdin - it gives error "inappropriate ioctl for device"

Save this code as readpass.go

$ go build readpass.go
$ ./readpass 
2017/04/10 16:18:22 Read "hello"
$ echo "hello" | ./readpass
2017/04/10 16:18:34 inappropriate ioctl for device
$ 

What did you expect to see?

2017/04/10 16:18:22 Read "hello"

I would expect ReadPass to figure out that it is not reading from a terminal before issuing ioctls that are terminal specific.

What did you see instead?

2017/04/10 16:18:34 inappropriate ioctl for device

Originally reported in: ncw/rclone#1308

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2017

was this working before in a previous go release?

@ncw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

@jessfraz I don't think this has ever worked - I did a quick bit of testing with old go versions and old x/crypto versions and couldn't find a working version.

@jessfraz

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

hmmm odd will play around

@bradfitz bradfitz changed the title x/crypto/ssh/terminal ReadPassword() doesn't work on redirected stdin giving inappropriate ioctl for device x/crypto/ssh/terminal: ReadPassword doesn't work on redirected stdin giving inappropriate ioctl for device Jun 7, 2017

@tmc tmc referenced this issue Nov 10, 2017

Closed

support stdin #67

@sevagh

This comment has been minimized.

Copy link

commented Jun 21, 2018

Are there any updates on this issue?
I wrote something that uses terminal.ReadPassword to have a login prompt, and trying to script some unit tests:

yes $(PASSWORD) | my-program login

Gives me the same error: Password (hidden): ⨯ Error when typing password: inappropriate ioctl for device

@sevagh

This comment has been minimized.

Copy link

commented Jun 21, 2018

You can try some istty handling like:

	if (stat.Mode() & os.ModeCharDevice) == 0 {
		reader := bufio.NewReader(os.Stdin)
		pass, err = reader.ReadString('\n')
		if err != nil {
			log.Fatalf("Error when typing password: %s", err.Error())
		}
	} else {
                //old prompt code
		...
@pam4

This comment has been minimized.

Copy link

commented Jun 22, 2018

When I use standard input for a password, I always check if it is a terminal and handle the two cases separately, like this:

fd := int(os.Stdin.Fd())
if terminal.IsTerminal(fd) {
    pw, err = terminal.ReadPassword(fd)
} else {
    // handle non-terminal
}

ReadPassword reads a line of input from a terminal without local echo.

Even if it did work with non-terminal FDs, I wouldn't rely on undocumented behavior. This looks like a feature request.
If it is going to be implemented, it should be clearly documented and it should read non-terminal FDs one byte at a time to avoid consuming more input than necessary.

@maraino

This comment has been minimized.

Copy link

commented Jun 27, 2018

@ncw the only solution I know is to open the tty and read from it, something like:

func readPassword(prompt string) ([]byte, error) {
    fmt.Fprint(os.Stderr, prompt)
    var fd int
    if terminal.IsTerminal(syscall.Stdin) {
        fd = syscall.Stdin
    } else {
        tty, err := os.Open("/dev/tty")
        if err != nil {
            return nil, errors.Wrap(err, "error allocating terminal")
        }
        defer tty.Close()
        fd = int(tty.Fd())
    }

    pass, err := terminal.ReadPassword(fd)
    fmt.Fprintln(os.Stderr)
    return pass, err
}
@pam4

This comment has been minimized.

Copy link

commented Jun 28, 2018

I can think of two common needs:

  1. read a password from the controlling terminal, regardless of standard input:
$ cat input-file | myprogram  # read password from terminal, not pipe
Password: _
  1. read a password from the standard input, regardless if it is a terminal, but if it is a terminal, turn off echo and use a prompt:
$ cat input-file | myprogram  # read password from pipe
$

$ myprogram  # read password from terminal
Password: _

In case #1 @maraino solution is fine, except I wouldn't even bother to check stdin and just open /dev/tty right away.

In case #2 you need to check if stdin is a terminal; if true use terminal.ReadPassword, otherwise use normal reads (directly or via bufio, but the only way to ensure that exactly one line is consumed is to read one byte at a time).

@ncw from the original post I would understand you are trying to achieve #2, correct me if I'm wrong.
I think the error you get by using ReadPassword with a non-terminal FD is to be expected, but if you are proposing an enhancement, please make your proposal clear.

@ncw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2018

@pam4 - I guess I was expecting ReadPassword to act like 2) so read the password from the pipe and only turn off echo if it was a terminal.

If it worked like option 1) that would be fine too.

However just returning inappropriate ioctl for device is unexpected.

Perhaps a patch to the documentation and to the the error returned might be all that is needed:

@@ -93,11 +95,12 @@ func (r passwordReader) Read(buf []byte) (int, error) {
 
 // ReadPassword reads a line of input from a terminal without local echo.  This
 // is commonly used for inputting passwords and other sensitive data. The slice
-// returned does not include the \n.
+// returned does not include the \n. Note that ReadPassword will only work on
+// an fd where IsTerminal(fd) returns true.
 func ReadPassword(fd int) ([]byte, error) {
 	termios, err := unix.IoctlGetTermios(fd, ioctlReadTermios)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("input must be a terminal: %v", err)
 	}
 
 	newState := *termios
@maraino

This comment has been minimized.

Copy link

commented Jun 28, 2018

@pam4 @ncw: my example was for case #1. On a program that reads on STDIN and then decides if a password is required or not.

It's not a common case, and probably not a good practice to get the user password from STDIN. For example, you can't do that using ssh (echo "tr0ub4dor&3" | ssh foo.bar.zar).

If you need to automatize something a most common case would be to use environment variables, a named file from an argument or an external secrets management tool API.

@sevagh

This comment has been minimized.

Copy link

commented Jun 28, 2018

If you need to automatize something a most common case would be to use environment variables, a named file from an argument or an external secrets management tool API.

You can still use all of these with the command line, without needing to make your Go code support them. E.g.:

$ yes $(vault read secret/mysecret) | my-tool
$ echo ${PASSWORD}" | my-tool
@pam4

This comment has been minimized.

Copy link

commented Jun 28, 2018

@maraino

It's not a common case, and probably not a good practice to get the user password from STDIN.

I don't think it is so uncommon. For example the ecryptfs utils read passphrases from stdin even if it's not a terminal. GPG and OpenVPN read passphrases from the terminal by default, but both have a way to use stdin (e.g. gpg --passphrase-fd 0).

Reading a password from stdin is not intrinsically less secure than reading it from a file or an environment variable; the problem is how the password is fed to stdin.

Of course it can be done wrong, for example if the password itself end up as a process argument, it would be very insecure (this is the case of echo PASSWORD | myprogram, unless echo is a shell builtin).

But I think any method to exchange secrets between processes can be used in an insecure way, unless it is done with care by someone who knows all the subtleties involved.

Going into details about this is probably off topic, but the point is that it is not always handled the same way, even among well known tools.

@ncw
The docs already say "from a terminal", so I don't really see much difference, but I don't see any harm either in stressing it. It would be nice to hear more opinions.
About wrapping the error, I'm not sure it is safe to assume that any error at that point is caused by the FD not being a terminal. If I wanted to change ReadPassword like that, I would just add an IsTerminal check with its own error at the top.

@ncw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

@pam4

About wrapping the error, I'm not sure it is safe to assume that any error at that point is caused by the FD not being a terminal. If I wanted to change ReadPassword like that, I would just add an IsTerminal check with its own error at the top.

If you look at the implementation of IsTerminal it does exactly the same thing as that call and returns err == nil

https://github.com/golang/crypto/blob/a49355c7e3f8fe157a85be2f77e6e269a0f89602/ssh/terminal/util.go#L29-L32

So I don't see the point of calling IsTerminal again!

@pam4

This comment has been minimized.

Copy link

commented Jun 29, 2018

@ncw you are right, I should have checked, I assumed IsTerminal did something more specific like checking if the error is ENOTTY.

@truongnmt

This comment has been minimized.

Copy link

commented Feb 7, 2019

I end up to this situation too. My specific case is exactly like #2. Here is what I came up with:

func readPassword(prompt string) ([]byte, error) {
	fmt.Fprint(os.Stderr, prompt)
	var fd int
	var pass []byte
	if terminal.IsTerminal(syscall.Stdin) {
		fd = syscall.Stdin
		inputPass, err := terminal.ReadPassword(fd)
		if err != nil {
			return nil, err
		}
		pass = inputPass
	} else {
		reader := bufio.NewReader(os.Stdin)
		s, err := reader.ReadString('\n')
		if err != nil {
			return nil, err
		}
		pass = []byte(s)
	}

	return pass, nil
}
@pam4

This comment has been minimized.

Copy link

commented Feb 10, 2019

@truongnmt your solution may be fine for your use case, but be aware that the buffering will generally prevent further use of standard input.

For example suppose that you need to read two passwords. Calling your readPassword function twice won't work, because the bufio.Reader of the first call will probably buffer more than the first password, and then such buffer is discarded and never seen by the second call.
If it's not a second password, it may be some other kind of input.

Here's a variation that doesn't have the buffering problem:

func readPassword(prompt string) (pw []byte, err error) {
	fd := int(os.Stdin.Fd())
	if terminal.IsTerminal(fd) {
		fmt.Fprint(os.Stderr, prompt)
		pw, err = terminal.ReadPassword(fd)
		fmt.Fprintln(os.Stderr)
		return
	}

	var b [1]byte
	for {
		n, err := os.Stdin.Read(b[:])
		// terminal.ReadPassword discards any '\r', so we do the same
		if n > 0 && b[0] != '\r' {
			if b[0] == '\n' {
				return pw, nil
			}
			pw = append(pw, b[0])
			// limit size, so that a wrong input won't fill up the memory
			if len(pw) > 1024 {
				err = errors.New("password too long")
			}
		}
		if err != nil {
			// terminal.ReadPassword accepts EOF-terminated passwords
			// if non-empty, so we do the same
			if err == io.EOF && len(pw) > 0 {
				err = nil
			}
			return pw, err
		}
	}
}

@truongnmt you also forgot to trim the final \n from the reader.ReadString output (notice that terminal.ReadPassword does not include the \n in its output).

@truongnmt

This comment has been minimized.

Copy link

commented Feb 12, 2019

Thanks for such a detail reply. Hmm I also use the custom readPassword when I need to enter password twice. Running fine tho, lemme have a careful look.

@pam4

This comment has been minimized.

Copy link

commented Feb 12, 2019

@truongnmt The bufio.Reader tries to fill its own buffer but doesn't wait for it to be full, so if it works for you it is probably because the timing is right (no more than the first password is available at the time buffering takes place).

For example:

$ echo -en 'password1\npassword2\n' | yourprogram # probably fails
$ cat passwordfile
password1
password2
$ cat passwordfile | yourprogram # probably fails
$ { echo password1; sleep 1; echo password2; } | yourprogram # probably works

Here is a playground demonstration:
https://play.golang.org/p/uxSMCw7anP4

Notice that terminal.ReadPassword itself also avoids buffering. In fact its implementation is similar to mine, see here.

@pam4

This comment has been minimized.

Copy link

commented Feb 23, 2019

@ncw, at first I didn't like the idea of changing terminal.ReadPassword, because users have different needs and I like the standard function to be as simple as possible, but now I realize that the code to handle use case #2 is already here, and the change would be trivial.

Ok, I would expect a function called terminal.ReadPassword to only work on a terminal, but the bad thing is that currently, use case #2 requires users to essentially duplicate the readPasswordLine function (possibly in an unsafe way with bufio).

The code could be changed from this:

termios, err := unix.IoctlGetTermios(fd, ioctlReadTermios)
if err != nil {
	return nil, err
}
// set up the terminal
// do the read

to this:

termios, err := unix.IoctlGetTermios(fd, ioctlReadTermios)
if err == nil {
	// set up the terminal
}
// do the read

Users that only want a terminal could still use the IsTerminal check or open /dev/tty directly.

Possible problems:

  • the continue here at line 957 can cause the loss of an error (Read can return both n > 0 and err != nil);
  • it would be nice to have a (generous) limit on password length, to avoid filling up the memory by accident when reading from non-terminals;
  • it may be safer to distinguish between the ENOTTY error and other errors (in the latter case it should fail, to avoid the risk of accidentally displaying the password).
@lfaoro

This comment has been minimized.

Copy link

commented May 17, 2019

fmt.Print("Encryption-key: ")
pwd, err := terminal.ReadPassword(int(os.Stdin.Fd()))
if err != nil {
	return err
}
fmt.Print("\n")
@agnivade

This comment has been minimized.

Copy link
Member

commented May 18, 2019

@hanwen

This comment has been minimized.

Copy link
Contributor

commented May 18, 2019

I am not involved in the terminal package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.