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 reads a maximum of 254 chars on Windows #36071

Closed
ncw opened this issue Dec 10, 2019 · 9 comments
Closed

x/crypto/ssh/terminal: ReadPassword reads a maximum of 254 chars on Windows #36071

ncw opened this issue Dec 10, 2019 · 9 comments

Comments

@ncw
Copy link
Contributor

@ncw ncw commented Dec 10, 2019

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

$ go version
go version go1.13.5 windows/amd64

Does this issue reproduce with the latest release?

Yes (not tried tip)

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

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Administrator\AppData\Local\go-build
set GOENV=C:\Users\Administrator\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\Administrator\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=C:\Users\Administrator\go\src\github.com\rclone\rclone\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\ADMINI~1\AppData\Local\Temp\2\go-build381157862=/tmp/go-build -gno-record-gcc-switches

What did you do?

I ran this program and typed > 254 characters

package main

import (
       "os"
       "fmt"
       "golang.org/x/crypto/ssh/terminal"
)

func main() {
        val, err := terminal.ReadPassword(int(os.Stdin.Fd()))
        fmt.Println(err)
        fmt.Printf("len=%d: %q\n", len(val), val)
}

What did you expect to see?

I expected to see all the characters I typed or an error message

What did you see instead?

My input string silently truncated to the first 254 characters.

go run x.go
<nil>
len=254: "alskdjflaskdjfaslkdfjlskdjflaksdjflskdjfalsdkfjlsakdfjlasdkfjlaskdfjalskdfjlasdkfjlasdkfjlsadkfjlasdkfjlsadkfjadslkfjlasdkfjadslkfjlasdkfjlakdsfjldsakfjlasdkfjlsadkfjlasdkfjlskdfjlaskdfjlakdsfjladksfjlaskdfjladksfjlasdkfjalsdkfjlasdkfjladskfjasldkfjalsdf"
@toothrot
Copy link
Contributor

@toothrot toothrot commented Dec 10, 2019

/cc @FiloSottile @katiehockman @alexbrainman

As you mentioned, this does look to be windows specific. I was not able to reproduce this on Linux.

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Dec 21, 2019

@ncw I can reproduce your problem.

I briefly looked at your code, and I cannot see why returned result is limited to 254 chars.

Maybe @mattn know.

Alex

@silviucm
Copy link

@silviucm silviucm commented Dec 21, 2019

@ncw @alexbrainman
Not exactly sure if applicable, so apologies in advance, but here's a .NET doc page that mentions the 254 character limitation, due to a default internal buffer, and suggests an alternative

https://docs.microsoft.com/en-us/dotnet/api/system.console.readline?view=netframework-4.8

By default, the method reads input from a 256-character input buffer. Because this includes the Environment.NewLine character(s), the method can read lines that contain up to 254 characters. To read longer lines, call the OpenStandardInput(Int32) method.

From what I am seeing here, it's pointing to that:
https://github.com/golang/go/blob/master/src/internal/poll/fd_windows.go#L540

In case the same API is used, that may explain what's happening

@mattn
Copy link
Member

@mattn mattn commented Dec 21, 2019

It is limit of ENABLE_LINE_INPUT. When using ReadConsole with ENABLE_LINE_INPUT, the input buffer always be 254 (256 - \r - \n). To solve this issue, ReadPassword stop to use ENABLE_LINE_INPUT.

diff --git a/ssh/terminal/terminal.go b/ssh/terminal/terminal.go
index 2f04ee5..ea2cd57 100644
--- a/ssh/terminal/terminal.go
+++ b/ssh/terminal/terminal.go
@@ -946,11 +946,17 @@ func readPasswordLine(reader io.Reader) ([]byte, error) {
 	for {
 		n, err := reader.Read(buf[:])
 		if n > 0 {
-			switch buf[0] {
+			switch byte(buf[0]) {
+			case '\b':
+				if len(ret) > 0 {
+					ret = ret[:len(ret)-1]
+				}
 			case '\n':
 				return ret, nil
 			case '\r':
-				// remove \r from passwords on Windows
+				// On Windows, \n never read when ENABLE_LINE_INPUT is
+				// disabled.
+				return ret, nil
 			default:
 				ret = append(ret, buf[0])
 			}
diff --git a/ssh/terminal/util_windows.go b/ssh/terminal/util_windows.go
index 5cfdf8f..c2cdf24 100644
--- a/ssh/terminal/util_windows.go
+++ b/ssh/terminal/util_windows.go
@@ -85,8 +85,8 @@ func ReadPassword(fd int) ([]byte, error) {
 	}
 	old := st
 
-	st &^= (windows.ENABLE_ECHO_INPUT)
-	st |= (windows.ENABLE_PROCESSED_INPUT | windows.ENABLE_LINE_INPUT | windows.ENABLE_PROCESSED_OUTPUT)
+	st &^= (windows.ENABLE_ECHO_INPUT | windows.ENABLE_LINE_INPUT | windows.ENABLE_PROCESSED_INPUT)
+	st |= windows.ENABLE_PROCESSED_OUTPUT
 	if err := windows.SetConsoleMode(windows.Handle(fd), st); err != nil {
 		return nil, err
 	}

image

@mattn
Copy link
Member

@mattn mattn commented Dec 21, 2019

Sorry, the code above is wrong. we must stop to use ENABLE_PROCESSED_INPUT too.

@mattn
Copy link
Member

@mattn mattn commented Dec 21, 2019

For the note: dotnet/corefx@f374612

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 21, 2019

Change https://golang.org/cl/212377 mentions this issue: ssh/terminal: Stop using ENABLE_LINE_INPUT

@ncw
Copy link
Contributor Author

@ncw ncw commented Jan 20, 2020

I merged this fix into rclone however @Ajaja reported in rclone/rclone#3888 that the Enter key no longer works.

I can confirm this, the little test program above fails on WIndows 10 - you can't press Enter to end the input.

Sorry, I missed the new issue about this #36609

@alexbrainman
Copy link
Member

@alexbrainman alexbrainman commented Jan 23, 2020

I merged this fix into rclone however @Ajaja reported in rclone/rclone#3888 that the Enter key no longer works.

Sorry for submitting completely broken version.

Sorry, I missed the new issue about this #36609

Yes. The fix https://go-review.googlesource.com/c/crypto/+/215417 is out for review.

Alex

janisz added a commit to janisz/dcos-cli that referenced this issue Aug 17, 2020
Currently we are using simple Input to read token.
Due to some limitation adn echoing it will be better to read
it as password.

> ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Refs: golang/go#36071
janisz added a commit to janisz/dcos-cli that referenced this issue Aug 17, 2020
Currently we are using simple Input to read token.
Due to some limitation adn echoing it will be better to read
it as password.

> ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Refs: golang/go#36071
janisz added a commit to janisz/dcos-cli that referenced this issue Aug 17, 2020
Currently we are using simple Input to read token.
Due to some limitation adn echoing it will be better to read
it as password.

> ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Refs: golang/go#36071
janisz added a commit to janisz/dcos-cli that referenced this issue Aug 17, 2020
Currently we are using simple Input to read token.
Due to some limitation adn echoing it will be better to read
it as password.

> ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Refs: golang/go#36071
janisz added a commit to dcos/dcos-cli that referenced this issue Aug 19, 2020
Currently we are using simple Input to read token.
Due to some limitation adn echoing it will be better to read
it as password.

> ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Refs: golang/go#36071
gopherbot pushed a commit to golang/term that referenced this issue Oct 16, 2020
ReadConsole does not read more than 254 bytes when ENABLE_LINE_INPUT is
enabled.

Fixes golang/go#36071

Change-Id: If5c160404b855387a80f1d57638aac3f2db1a097
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/212377
Run-TryBot: Alex Brainman <alex.brainman@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.