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: known_host comments with spaces break on marker lines #139

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skemper
Copy link

@skemper skemper commented May 14, 2020

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

$ go version
go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/skemper/Library/Caches/go-build"
GOENV="/Users/skemper/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/skemper/Code/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/skemper/Code/go/src/github.com/golang/crypto/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/6c/yshyrn911ln2jxvm4l1_vznwslv63v/T/go-build928041994=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

When calling ssh.ParseKnownHosts(), I gave it a known_hosts line which contained both an @marker directive, as well as a comment which included spaces.

@cert-authority * ssh-rsa AAAABunchOfStuff Test Comment

What did you expect to see?

I expected ssh.ParseKnownHosts() to return a parsed version of the line, with the comment "Test Comment".

What did you see instead?

err was filled in, with "ssh: invalid entry in known_hosts data." This happens because the function checks if there are more than 5 fields in the line, which is an incorrect check in this scenario. Since we're still parsing the entire line, we can't put an upper limit on the number of spaces as comments may contain spaces. This fact is already properly respected by ParseAuthorizedKey, which is called later on.

It appears this wasn't caught by the existing tests because they either use comments with just a single space, or don't test the combination of markers and comments-with-spaces. I've added a few new tests to compensate.

…multi-space comments in a variety of scenarios; add tests to trigger 'invalid entry' condition
@gopherbot
Copy link
Contributor

This PR (HEAD: 60a347e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/crypto/+/234018 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/234018.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Go Bot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/234018.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants