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: nil pointer dereference in transport.go #39397

Open
ghost opened this issue Jun 4, 2020 · 12 comments
Open

x/crypto/ssh: nil pointer dereference in transport.go #39397

ghost opened this issue Jun 4, 2020 · 12 comments
Labels
NeedsFix
Milestone

Comments

@ghost
Copy link

@ghost ghost commented Jun 4, 2020

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

$ go version
go version go1.14.2 windows/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
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\janos\AppData\Local\go-build
set GOENV=C:\Users\janos\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\janos\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\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\janos\go\src\containerssh\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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\janos\AppData\Local\Temp\go-build066225482=/tmp/go-build -gno-record-gcc-switches

What did you do?

Implement an SSH server with a limited set of MACs supported as follows. (Batteries included.) Then connect from a command line SSH client in Windows or Ubuntu 18.04 (not PuTTY). Tested on the following SSH versions:

  • OpenSSH_7.6p1 Ubuntu-4ubuntu0.3, OpenSSL 1.0.2n 7 Dec 2017
  • OpenSSH_for_Windows_7.7p1, LibreSSL 2.6.5
package main

import (
	"fmt"
	"golang.org/x/crypto/ssh"
	"log"
	"net"
	"strings"
)

func main() {
	config := &ssh.ServerConfig{}
	config.MACs = strings.Split("hmac-sha2-512-etm@openssh.com,hmac-sha2-256-etm@openssh.com,umac-128-etm@openssh.com,hmac-sha2-512,hmac-sha2-256,umac-128@openssh.com", ",")
	config.NoClientAuth = true
	hostKey := "-----BEGIN RSA PRIVATE KEY-----\nMIIEpAIBAAKCAQEAxXITaWGnEce/7c6G40Gr2krd8eIXbYK2xeeW7WxL73whktYh\nl5adRKFqxBmbaJr8AtkRe4EfOi4JNw0wha7jd6aGpXqOoETLKSRfKMf2Rl/OpJBB\nvArqiODVjsjjQ+p3n0X1uq3NhHoGyw9sCKW0Ex/FtVV3vGoUM3hXgqq6cRhnhJkb\n4tKNZGozESsYykO0PKoHtgz9+Fu7tXcm11BCxLAF0lNIN7zascHjOLUE0K8hD0TN\n9DFBPkxozcpAcVK30BkmbSzymDn7iH2kM1M/Bie8POFPivCnb47bdhK9d95geyRE\nM4h4Mip8ajanSr0QtIIE1WvuhPyNBPYPrE/tWwIDAQABAoIBAQCCh53ppxNKH+H0\nlvp5tuqjz6bWx/9w+F9Yingu0c+JrWiMqF9g1cN2obfxmHQzPXKpd1OlcIFydwjE\nikWvuyTJ7x9IurMbpR8iPddjIH7hm2kDRhMO+7B9eRX15TzJo3Idq6R+G3SoxftD\ntxhWxyU7F2AfNzeHKiHcvQwtaC0FolCPBRrVnk6d91DDT8UvkW5di3s9op/AMASl\n1DLWDyh+A+w5bTAVC2XsLfwVeIyFuzLW3TNCZZcFjcorKU2d7f1mwZRnKDieAQjw\nOkClH3LqFsv8sol45JOs11V5AiUzEiBiZvuQkbi72YuG0hz4qMGJeap4+EWdVEXz\n356Ufa+RAoGBAPHXvdzan3yx81jOSjS3UwfTEM1NPuJWPVfDWDMHLxM3OP5Csb2L\nWHwVH86YO1t/TGJdmrCO2u5cN/K+Gs1Z2RbGR3e73cd9+Xf9wZjAiPVNTJATTGnO\na6JHZhH3iAeFe8vLzILBy5AwU1uK/3h+iTAVWYu3bLvVd/IO3XYgY0k/AoGBANEA\n/4nL3W9gW4PBGMjBj92pXbYPViQ4P4V8A1HFgX6CacrrB7e7UgwCxXXT+8VV1Kk+\nvX6erpRVNj+sC1ZDBkWOOiEguEkL5VxcHqfX5jBIMN8p0JwR+Ige3Wn47MgviD77\nbUZ5qOdP/7aGbwVrofGB5CIsRJMMA2j8FwyNfpjlAoGAE6Zw8dn9rXcUC7ovQ1tF\n7tPSgKvvGRrivxfLOdIynAAXrGkk4f7JgMOCoxmxcPzF6xQp9oCU/1sU3K9fpCHR\nxszzj6H/Ii91Lq+6pDu4pR3Tw70dr1crXbMpcvpG3j2VUnjLtDAk5yFWFJEVsuet\nI/AIJ5cOybBNn6hfjDKTfqcCgYEAsnslvejoeppQzFzz60zQrLxbmIPUTi3yoO1c\ncFI32W9JJM480vwWfsdHFO4oTUaUyssXS6/66hUytIEZVVr7Wh6xKWUlust73LmW\nPEM1AfpEMe8lhIIcOTISZtL6caGVuiNAGDUAtjgs6RQ4buqRawo/ZadkECbsKpVM\noZ2bhLkCgYAWVJSomInW9rwx1tmMk88WqPX0eINYX+NODMDn2k2XSW79uPlAIj3q\nJAtuvlvkebm85yBPyeHFbJFSvS6o2HvkyXegN1YMbc/PfhYJfcP/ceI4d8dEfwMY\nRp423gj4VlvuZsnOLhIXKeiZUBzw1y4STtyavNMbxe6ZvLK3FxlgMw==\n-----END RSA PRIVATE KEY-----\n"
	private, err := ssh.ParsePrivateKey([]byte(hostKey))
	if err != nil {
		log.Fatal("Failed to parse private key")
	}
	config.AddHostKey(private)

	listener, err := net.Listen("tcp", "127.0.0.1:22")
	if err != nil {
		log.Fatalf("Failed to listen (%s)", err)
	}
	for {
		tcpConn, err := listener.Accept()
		if err != nil {
			log.Printf("Failed to accept incoming connection (%s)", err)
			continue
		}
		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())
		go ssh.DiscardRequests(reqs)
		go handleChannels(chans)
	}
}

func handleChannels(chans <-chan ssh.NewChannel) {
	for newChannel := range chans {
		go handleChannel(newChannel)
	}
}

func handleChannel(newChannel ssh.NewChannel) {
	newChannel.Reject(ssh.UnknownChannelType, fmt.Sprintf("unknown channel type"))
}

What did you expect to see?

The connection with an unknown MAC being rejected.

What did you see instead?

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x57b5fe]

goroutine 20 [running]:
golang.org/x/crypto/ssh.newPacketCipher(0x716165, 0x1, 0x1, 0x716166, 0x1, 0x1, 0x716167, 0x1, 0x1, 0xc00009e5e0, ...)
	C:/Users/janos/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200602180216-279210d13fed/ssh/transport.go:245 +0x14e
golang.org/x/crypto/ssh.(*transport).prepareKeyChange(0xc0000d6360, 0xc0000f6280, 0xc0000f6300, 0xc0000f6280, 0xc0000ea540)
	C:/Users/janos/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200602180216-279210d13fed/ssh/transport.go:80 +0x131
golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange(0xc0000fa000, 0xc000104580, 0x51a, 0x51a, 0x1, 0x0)
	C:/Users/janos/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200602180216-279210d13fed/ssh/handshake.go:599 +0x3df
golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop(0xc0000fa000)
	C:/Users/janos/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200602180216-279210d13fed/ssh/handshake.go:301 +0x1d5
created by golang.org/x/crypto/ssh.newServerTransport
	C:/Users/janos/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200602180216-279210d13fed/ssh/handshake.go:143 +0x10c

The list of MACs comes from the Mozilla list

Root cause

transport.go does not check if the MAC requested is present in the list of MACs supported and therefore crashes when the lookup yields a nil. (In this case umac-128-etm@openssh.com)

@gopherbot gopherbot added this to the Unreleased milestone Jun 4, 2020
@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 4, 2020

@janoszen thanks for raising this issue. Do you want to send a fix?

@ghost
Copy link
Author

@ghost ghost commented Jun 4, 2020

@janoszen thanks for raising this issue. Do you want to send a fix?

Yes, I would like to, but I don't know which repo is the correct one.

@ghost
Copy link
Author

@ghost ghost commented Jun 4, 2020

My first attempt at a patch would be this:

diff --git a/ssh/server.go b/ssh/server.go
index 7d42a8c..97a0a11 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -179,6 +179,26 @@ type ServerConn struct {
 	Permissions *Permissions
 }
 
+// Helper function to find unsupported items such as key exchanges, MACs and ciphers
+func findUnsupported(name string, requestedList []string, supportedList []string, forbiddenList map[string]struct{}) error {
+	for _, requestedItem := range requestedList {
+		if _, ok := forbiddenList[requestedItem]; ok {
+			return fmt.Errorf("ssh: unsupported %s %s for server", name, requestedItem)
+		}
+		found := false
+		for _, supportedItem := range supportedList {
+			if supportedItem == requestedItem {
+				found = true
+				break
+			}
+		}
+		if !found {
+			return fmt.Errorf("ssh: unsupported %s %s for server", name, requestedItem)
+		}
+	}
+}
+
+
 // NewServerConn starts a new SSH server with c as the underlying
 // transport.  It starts with a handshake and, if the handshake is
 // unsuccessful, it closes the connection and returns an error.  The
@@ -193,11 +213,23 @@ func NewServerConn(c net.Conn, config *ServerConfig) (*ServerConn, <-chan NewCha
 	if fullConf.MaxAuthTries == 0 {
 		fullConf.MaxAuthTries = 6
 	}
+
 	// Check if the config contains any unsupported key exchanges
-	for _, kex := range fullConf.KeyExchanges {
-		if _, ok := serverForbiddenKexAlgos[kex]; ok {
-			return nil, nil, nil, fmt.Errorf("ssh: unsupported key exchange %s for server", kex)
-		}
+	err := findUnsupported("key exchange", fullConf.KeyExchanges, supportedKexAlgos, serverForbiddenKexAlgos)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	// Check if the config contains any unsupported MACs
+	err = findUnsupported("MAC", fullConf.MACs, supportedMACs, map[string]struct{}{})
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	// Check if the config contains any unsupported Ciphers
+	err = findUnsupported("cipher", fullConf.Ciphers, supportedCiphers, map[string]struct{}{})
+	if err != nil {
+		return nil, nil, nil, err
 	}
 
 	s := &connection{

@davecheney
Copy link
Contributor

@davecheney davecheney commented Jun 4, 2020

Thank you, please follow the contribution process, https://golang.org/doc/contribute.html

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 4, 2020

Change https://golang.org/cl/236517 mentions this issue: ssh: Fixes golang/go#39397 nil pointer dereference on unsupported MAC

@dmitshur dmitshur changed the title x/crypto/ssh nil pointer dereference in transport.go x/crypto/ssh: nil pointer dereference in transport.go Jun 4, 2020
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Jun 4, 2020

/cc @hanwen @FiloSottile

Also /cc @stamblerre since CL 234899 is relevant here.

@dmitshur dmitshur added the NeedsFix label Jun 4, 2020
@ghost
Copy link
Author

@ghost ghost commented Jun 4, 2020

To keep the issue updated here too I've added the proposed patch and tests for it.

However, there seems to be a bit of an inconsistency with how I've implemented it. It seems the original code checks if the supplied cipher list is actually supported and removes the ciphers configured but not supported silently. My implementation for MACs and Kex' currently throws an error. I see three options:

  1. Throw errors for ciphers too.
  2. Change MAC and Kex checks to silently drop unsupported items.
  3. Change MAC and Kex checks to drop unsupported items and add a log warning to all three.

@ghost
Copy link
Author

@ghost ghost commented Jun 14, 2020

@davecheney @dmitshur is there anything else I can do to move this forward?

@ghost
Copy link
Author

@ghost ghost commented Jul 9, 2020

Hey folks, I don't mean to be pushy but I pushed (pun intended) my last patchset almost a month ago. Can I do something to get this moving?

@ghost
Copy link
Author

@ghost ghost commented Nov 8, 2020

Hey folks, any way to get this moving again?

@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Nov 8, 2020

@janoszen A problem here is that the ssh package doesn't have an explicit owner, so your efforts to fix this issue are currently bottlenecked on finding a reviewer for CL 236517. There are many open issues and CLs for this package, and there are many other packages, but few reviewers with available bandwidth.

I'm not sure what can be done other than to continue to wait until someone is able to review your CL. See also #17244 for past discussion on this topic. If this bug is causing a problem for your code, consider relying on your own fork of the package in the meantime, or looking for another package.

@ghost
Copy link
Author

@ghost ghost commented Nov 8, 2020

Hey @dmitshur thank you very much for your honesty.

I am using the SSH package heavily in my project ContainerSSH. I have built a number of improvements mostly in the validation department that would make sense upstream.

So my question is: what can I do to help? I'd like to see this library be maintained in the future and not be lost due to the lack of maintenance.

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

No branches or pull requests

3 participants