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: nonstandard const maxAgentResponseBytes should be configurable #64939

Open
cweagans opened this issue Jan 3, 2024 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@cweagans
Copy link

cweagans commented Jan 3, 2024

Go version

go version go1.21.1 darwin/arm64

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

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/cweagans/Library/Caches/go-build'
GOENV='/Users/cweagans/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/cweagans/Developer/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/cweagans/Developer/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.21.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.21.1/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.21.1'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/cweagans/Developer/github.com/swirldslabs/txauth/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/xh/xytgk13j0v33_8zj9qdtpyh80000gp/T/go-build3042814605=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

We have an agent protocol extension that can sometimes send a fair amount of data over the agent protocol - more than the 16kb limit imposed by https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.17.0:ssh/agent/client.go;l=156. Since there isn't a limit specified by the agent protocol, I didn't expect that the go library would impose one and ended up spending way too much time narrowing down the problem.

What did you expect to see?

I expected to be able to send any amount of arbitrary data over the connection.

What did you see instead?

I could not do that and got this error in my logs: agent 11: agent: client error: response too large.

@gopherbot gopherbot added this to the Unreleased milestone Jan 3, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 9, 2024
@dmitshur
Copy link
Contributor

dmitshur commented Jan 9, 2024

CC @drakkan.

@drakkan
Copy link
Member

drakkan commented Jan 9, 2024

@cweagans thank you for reporting this issue. Can you please share a reproducer and confirm it works with the OpenSSH agent implementation?

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 9, 2024
@cweagans
Copy link
Author

cweagans commented Jan 9, 2024

Sure, here ya go!

OpenSSH doesn't appear to use any agent protocol extensions beyond session-bind@openssh.com (which is used for setting key restrictions), so I'm not sure how to verify this against any of the OpenSSH implementations. None of the widely used implementations will really care about a custom protocol extension anyway.

My particular use case is related to a protocol extension (which by nature is specific to my project), but I don't see anywhere in the spec that a maximum agent reply size is specified. The comment on maxAgentResponseBytes agrees with that as well. It looks like client.List() relies on this value to see if there are too many keys in the agent reply + client.CallRaw uses it the way that you'd expect from the name. I'm not 100% sure that client.List() is doing the right thing with that variable.

Just brainstorming here: is there any reason to not make maxAgentResponseBytes a public variable that developers can simply change? The default value can stay what it is currently -- in most cases, that's a pretty reasonable limit. Alternatively, we could add an additional interface (ConfigurableAgent that has a method GetMaxAgentResponseBytes() int or something along those lines) that can override that limit on a per-agent basis (so those internal methods would either use the value of maxAgentResponseBytes or whatever GetMaxAgentResponseBytes() returns.

I've worked around this in my project temporarily by simply applying a patch to the agent package to change the value to 16 << 24 instead.


`testagent.go` (expand for source)
package main

import (
	"crypto/rand"
	"errors"
	"flag"
	"io"
	"log"
	"net"
	"os"
	"os/signal"

	"golang.org/x/crypto/ssh"
	"golang.org/x/crypto/ssh/agent"
)

var socketPath = "/tmp/response-limit-tester-agent.sock"

var serveAgent bool

func main() {
	flag.BoolVar(&serveAgent, "serve", false, "run the testing agent (otherwise, will run the client)")
	flag.Parse()

	if serveAgent {
		runAgent()
	} else {
		runClient()
	}
}

func runClient() {
	c, err := net.Dial("unix", socketPath)
	if err != nil {
		log.Fatalf("failed to dial testing agent: %v", err)
	}

	client := agent.NewClient(c)

	res, err := client.Extension("responselimittester@golang.org", []byte{})
	if err != nil {
		log.Fatalf("failed to get response from testing agent: %v", err)
	}

	log.Printf("got response from testing agent: %d bytes", len(res))
}

func runAgent() {
	ln, err := net.Listen("unix", socketPath)
	if err != nil {
		log.Fatal(err)
	}
	defer ln.Close()
	log.Printf("listening on %s", socketPath)

	// Handle interrupts.
	exit := make(chan os.Signal, 1)
	signal.Notify(exit, os.Interrupt)
	go func() {
		<-exit
		log.Println("received SIGINT; shutting down")
		ln.Close()
		os.Exit(0)
	}()

	log.Println("ready")

	for {
		c, err := ln.Accept()
		if errors.Is(err, net.ErrClosed) {
			return
		}
		if err != nil {
			log.Println("error accepting connection:", err)
		}

		go handleConnection(c)
	}
}

func handleConnection(c net.Conn) {
	log.Println("client connected; serving testing agent")
	defer c.Close()
	a := &TestingAgent{}
	err := agent.ServeAgent(a, c)
	if err == io.EOF {
		err = nil
	}
	if err != nil {
		log.Fatalf("failed to serve agent: %v", err)
	}
	log.Println("connection closed")
}

var ErrNotImplemented = errors.New("not implemented")

// TestingAgent only implements the protocol extension and just passes random data back to the client.
type TestingAgent struct{}

func (a *TestingAgent) List() ([]*agent.Key, error) {
	return nil, ErrNotImplemented
}

func (a *TestingAgent) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) {
	return nil, ErrNotImplemented
}

func (a *TestingAgent) Add(key agent.AddedKey) error {
	return ErrNotImplemented
}

func (a *TestingAgent) Remove(key ssh.PublicKey) error {
	return ErrNotImplemented
}

func (a *TestingAgent) RemoveAll() error {
	return ErrNotImplemented
}

func (a *TestingAgent) Lock(passphrase []byte) error {
	return ErrNotImplemented
}

func (a *TestingAgent) Unlock(passphrase []byte) error {
	return ErrNotImplemented
}

func (a *TestingAgent) Signers() ([]ssh.Signer, error) {
	return nil, ErrNotImplemented
}

func (a *TestingAgent) SignWithFlags(key ssh.PublicKey, data []byte, flags agent.SignatureFlags) (*ssh.Signature, error) {
	return nil, ErrNotImplemented
}

func (a *TestingAgent) Extension(extensionType string, contents []byte) ([]byte, error) {
	log.Println("got extension request with type:", extensionType)

	if extensionType == "responselimittester@golang.org" {
		response := make([]byte, 16<<24)
		rand.Read(response)
		return response, nil
	}

	return nil, nil
}

Before changing maxAgentResponseBytes:

$ ./testagent -serve
2024/01/09 11:10:14 listening on /tmp/response-limit-tester-agent.sock
2024/01/09 11:10:14 ready
2024/01/09 11:10:16 client connected; serving testing agent
2024/01/09 11:10:16 got extension request with type: responselimittester@golang.org
2024/01/09 11:10:17 failed to serve agent: agent: reply too large: 268435456 bytes

# In another terminal:
$ ./testagent
2024/01/09 11:10:17 failed to get response from testing agent: agent: client error: EOF

After changing maxAgentResponseBytes:

./testagent -serve
2024/01/09 11:09:26 listening on /tmp/response-limit-tester-agent.sock
2024/01/09 11:09:26 ready
2024/01/09 11:09:31 client connected; serving testing agent
2024/01/09 11:09:31 got extension request with type: responselimittester@golang.org
2024/01/09 11:09:32 connection closed
^C2024/01/09 11:10:02 received SIGINT; shutting down

# In another terminal:

$ ./testagent
2024/01/09 11:09:32 got response from testing agent: 268435456 bytes

@drakkan
Copy link
Member

drakkan commented Jan 9, 2024

Thanks for the reply, the reason I asked is because OpenSSH also seems to have a similar limitation.

Your request makes sense, we need to propose the new API and go through the change proposal process

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 9, 2024
@cweagans
Copy link
Author

cweagans commented Jan 9, 2024

Okay. How can I help? Should I write that proposal? Do you have some idea of which direction would make sense? (Public var vs a new interface vs just change the default)

@drakkan
Copy link
Member

drakkan commented Jan 27, 2024

The limit was introduced in this commit, so it is here since the initial implementation. It wasn't added to fix any particular issue, so it just seems like a sanity check to avoid consuming too much memory or such.

Maybe we can just increase the limit. What would be an appropriate value for your use case? For example a 1MB limit should be appropriate for any use case and at the same time we keep the existing sanity check in place.

I don't like a public var is good choice. If changing the default is not acceptable we should add an interface extension or an option (for example func NewClient(rw io.ReadWriter, options ...AgentOption) ExtendedAgent).

I would like the opinion of other maintainers

cc @golang/security

@cweagans
Copy link
Author

I don't think we should just change the default. That might fix it for me personally, but the ideal value for the limit really depends on what you're doing with the library.

IMO, it should really be configurable so that devs can set the right value for their specific application.

@drakkan
Copy link
Member

drakkan commented Jan 30, 2024

Uhmm, I can't think of a scenario where the agent can be untrusted, maybe we can just remove this limit instead of making it configurable.

@cweagans
Copy link
Author

That seems reasonable to me.

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

No branches or pull requests

5 participants