Patching PublicKeyCallback for CVE-2024-45337

[alecb-stripe]

From [security] Vulnerability in golang.org/x/crypto:

For example, an attacker may send public keys A and B, and then authenticate with A. PublicKeyCallback would be called only twice, first with A and then with B. A vulnerable application may then make authorization decisions based on key B for which the attacker does not actually control the private key.

golang.org/x/[email protected] enforces the property that, when successfully authenticating via public key, the last key passed to ServerConfig.PublicKeyCallback will be the key used to authenticate the connection.

I'm worried that this fix isn't sufficient given this package's use of PublicKeyCallback, because applyConnMetadata only ever sets context values on its first call:

ssh/context.go

Lines 114 to 116 in adec695

	if ctx.Value(ContextKeySessionID) != nil {
	return
	}

I.e., the fact that v0.31.0 makes additional calls to PublicKeyCallback won't actually help here, because only the values from the first call get stored in the ctx.

Does that seem correct? If so, should applyConnMetadata be patched so that we used the values from the last call? Or would some other patch be appropriate?

[belak]

I'm still looking at this - I hope to have a closer look tonight.

I think you're right, that we should store the public key from the last call to PublicKeyCallback, but I want to make sure before pushing out a pre-mature fix.

EDIT: this is not correct, see below.

[smlx]

I don't know if applyConnMetadata is the problem here. None of the value it sets will be change between calls to PublicKeyCallback with different keys AFAIK?

However, storing the public key value in the connection context inside PublicKeyCallback surely is problematic because the last call to this callback will set the final value of the public key in the context, which is not necessarily the key used to authenticate? i.e. this seem vulnerable:

ssh/server.go

Line 163 in adec695

ctx.SetValue(ContextKeyPublicKey, key)

But I guess with the updated behaviour in 0.31.0 it will no longer be a problem.

[belak]

Yeah, it's been a while since I've looked at this code, but I believe updating to 0.31.0 will be sufficient. It seems like the alternative is to store the public key in the Permissions.Extensions, but that would require marshaling and unmarshaling the public key because it's a map[string]string rather than a map[string]any.

The simplest solution is to update x/crypto to 0.31.0, which I will do later tonight.

[belak]

I've pushed commit a8ecd3e, but I don't have a great way to verify the change. Because this is a standard dependency bump, I don't have a problem tagging a new version, but I'd like to have a way to verify the behavior is correct before closing this.

[belak]

I've tagged the previously mentioned commit as v0.3.8, to allow people to easily update to a version of x/crypto which should resolve the issue.

However, we're still leaking Permissions data between auth callbacks, which could result in a similar issue to the x/crypto CVE if used incorrectly.

Because of this I've opened #243 which aims to make Permissions harder to misuse, and avoids attaching public keys users don't actually have the corresponding PrivateKey for to the Context.

[viceice]

We're now using a fork and made the connection permission public. So we can use the key from auth handler

https://code.forgejo.org/forgejo/ssh/commit/5fc306ca0616d1b05ce8f81c480ea6ee21ebb2c0