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

Jump to bottom

x/crypto/ssh: fix documention for ssh.NewServerConn to avoid potential DoS attack #43521

Open
ncw opened this issue Jan 5, 2021 · 1 comment
Open

x/crypto/ssh: fix documention for ssh.NewServerConn to avoid potential DoS attack #43521

ncw opened this issue Jan 5, 2021 · 1 comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@ncw
Copy link
Contributor

ncw commented Jan 5, 2021

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

$ go version
go version go1.15.2 linux/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="/home/ncw/.cache/go-build"
GOENV="/home/ncw/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/ncw/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/ncw/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go/go1.15"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go1.15/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build874025211=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I followed the example here for building an ssh server: https://gist.github.com/jpillora/b480fde82bff51a06238

This contains the code

	for {
		tcpConn, err := listener.Accept()
		if err != nil {
			log.Printf("Failed to accept incoming connection (%s)", err)
			continue
		}
		// Before use, a handshake must be performed on the incoming net.Conn.
		sshConn, chans, reqs, err := ssh.NewServerConn(tcpConn, config)
		if err != nil {
			log.Printf("Failed to handshake (%s)", err)
			continue
		}

		log.Printf("New SSH connection from %s (%s)", sshConn.RemoteAddr(), sshConn.ClientVersion())
		// Discard all global out-of-band Requests
		go ssh.DiscardRequests(reqs)
		// Accept all channels
		go handleChannels(chans)
	}

The problem with this code is that calling ssh.NewServerConn can take an arbitrary length of time as it might have to ask the user for a password. In fact calling it like this is setting up a potential DoS attack - one user can take as long as desired to do the authentication, blocking any other users from connecting.

The documentation for ssh.NewServerConn should be changed to say that it must be called in a new go routine and should not be called in in the same go routine as the Listen loop.

It might be a good idea if the example for ssh.NewServerConn was changed to show a loop around the Listen call with the ssh.NewServerConn being called in a go routine.

This was originally discovered in rclone/rclone#4882 - thanks to @FiloSottile for working out the cause of the problem and suggesting making this issue.
@gopherbot gopherbot added this to the Unreleased milestone Jan 5, 2021
ncw added a commit to rclone/rclone that referenced this issue Jan 5, 2021
@ncw
sftp: fix authentication on one connection blocking others - fixes #4882
9b14eb6 


Before this change, if one connection was authenticating this would
block any others from authenticating.

This was due to ssh.NewServerConn not being called in a go routine
after the Accept call.

This is fixed by running the ssh authentication in a go routine.

Thanks to @FiloSottile for advice on how to fix this.

See: golang/go#43521
@ncw ncw mentioned this issue Jan 5, 2021
Closed
@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 5, 2021
ncw added a commit to rclone/rclone that referenced this issue Jan 6, 2021
@ncw
serve sftp: fix authentication on one connection blocking others - fixes
c553ad5 
 #4882

Before this change, if one connection was authenticating this would
block any others from authenticating.

This was due to ssh.NewServerConn not being called in a go routine
after the Accept call.

This is fixed by running the ssh authentication in a go routine.

Thanks to @FiloSottile for advice on how to fix this.

See: golang/go#43521
ncw added a commit to rclone/rclone that referenced this issue Jan 13, 2021
@ncw
serve sftp: fix authentication on one connection blocking others - fixes
9d9999d 
 #4882

Before this change, if one connection was authenticating this would
block any others from authenticating.

This was due to ssh.NewServerConn not being called in a go routine
after the Accept call.

This is fixed by running the ssh authentication in a go routine.

Thanks to @FiloSottile for advice on how to fix this.

See: golang/go#43521
negative0 pushed a commit to negative0/rclone that referenced this issue Aug 13, 2021
@ncw @negative0
serve sftp: fix authentication on one connection blocking others - fixes
28af7b6 
 rclone#4882

Before this change, if one connection was authenticating this would
block any others from authenticating.

This was due to ssh.NewServerConn not being called in a go routine
after the Accept call.

This is fixed by running the ssh authentication in a go routine.

Thanks to @FiloSottile for advice on how to fix this.

See: golang/go#43521
@abbbi abbbi mentioned this issue Mar 1, 2023
Merged
This was referenced Mar 19, 2023
Merged
Merged
@gopherbot
Copy link

Change https://go.dev/cl/570975 mentions this issue: ssh: improve doc for NewServerConn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@toothrot @ncw @gopherbot