x/crypto/ssh: msgNewKeys interpreted too soon #18850

Closed
hanwen opened this Issue Jan 30, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@hanwen
Contributor

hanwen commented Jan 30, 2017

"We've also run into a different issue during the kex/handshake.

panic: ssh: no key material for msgNewKeys
goroutine 31868301 [running]:
panic(0x7ce020, 0xc42414c260)
/usr/local/go/src/runtime/panic.go:500 +0x1a1
bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh.(*connectionState).writePacket(0xc422774068, 0xc4218a8480, 0xa90080, 0xc42000c2d0, 0xc420010790, 0x9, 0x9, 0x0, 0x0)
/go/src/bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh/transport.go:163 +0x226
bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh.(*transport).writePacket(0xc422774000, 0xc420010790, 0x9, 0x9, 0x0, 0x0)
/go/src/bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh/transport.go:144 +0x77
bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh.(*handshakeTransport).pushPacket(0xc422326140, 0xc420010790, 0x9, 0x9, 0x0, 0x0)
/go/src/bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh/handshake.go:211 +0x51
bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop(0xc422326140)
/go/src/bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh/handshake.go:291 +0x303
created by bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh.newServerTransport
/go/src/bitbucket.org/bitbucket/conker/vendor/golang.org/x/crypto/ssh/handshake.go:126 +0x227
This is on a server running on golang/crypto@41d678d

We were previously running golang/crypto@ca7e7f1"

@belak

This comment has been minimized.

Show comment
Hide comment
@belak

belak Jan 30, 2017

Ah, thanks for opening this. I wasn't sure if I should open a new issue because it also seemed related to the kex rewrite. Let me know if there's any additional information you need to help debugging this.

belak commented Jan 30, 2017

Ah, thanks for opening this. I wasn't sure if I should open a new issue because it also seemed related to the kex rewrite. Let me know if there's any additional information you need to help debugging this.

@bradfitz bradfitz changed the title from msgNewKeys interpreted too soon. to x/crypto/ssh: msgNewKeys interpreted too soon Jan 30, 2017

@bradfitz bradfitz added this to the Unreleased milestone Jan 30, 2017

@hanwen

This comment has been minimized.

Show comment
Hide comment
@hanwen

hanwen Jan 30, 2017

Contributor

I took a look at this , but it's puzzling.

  • In handshake.go line 291, we are writing a packet coming from pendingPackets with a msgNewKeys.

  • pendingPackets is only populated from handshakeTransport.writePacket, and we check that the packet type is not msgNewKeys at the top of the function.

Can you confirm that your code runs cleanly under the race detector?

Contributor

hanwen commented Jan 30, 2017

I took a look at this , but it's puzzling.

  • In handshake.go line 291, we are writing a packet coming from pendingPackets with a msgNewKeys.

  • pendingPackets is only populated from handshakeTransport.writePacket, and we check that the packet type is not msgNewKeys at the top of the function.

Can you confirm that your code runs cleanly under the race detector?

@belak

This comment has been minimized.

Show comment
Hide comment
@belak

belak Jan 30, 2017

We run all our tests with the race detector and haven't had any complaints from that lately.

What would happen if a client sends a kex with FirstKexFollows set? I can try to test that (using dropbear), but it's just a guess at this point.

EDIT:
After looking through the code, I think the most likely reason is that the error from prepareKeyChange is ignored which would mean that the key change data wouldn't be set. However, that looks like old code, so I'm not certain that's the case... probably a red herring.

belak commented Jan 30, 2017

We run all our tests with the race detector and haven't had any complaints from that lately.

What would happen if a client sends a kex with FirstKexFollows set? I can try to test that (using dropbear), but it's just a guess at this point.

EDIT:
After looking through the code, I think the most likely reason is that the error from prepareKeyChange is ignored which would mean that the key change data wouldn't be set. However, that looks like old code, so I'm not certain that's the case... probably a red herring.

@hanwen

This comment has been minimized.

Show comment
Hide comment
@hanwen

hanwen Jan 31, 2017

Contributor

good catch, yes the error check is certainly to blame for the panic, but then what error does it print?

Contributor

hanwen commented Jan 31, 2017

good catch, yes the error check is certainly to blame for the panic, but then what error does it print?

@hanwen

This comment has been minimized.

Show comment
Hide comment
@hanwen

hanwen Jan 31, 2017

Contributor

ok, so

	t.pendingPackets = t.pendingPackets[0:]

looks plausible in codereview, but is actually a pretty dumb thing to do. I wonder if we should find this automatically with go vet.

Contributor

hanwen commented Jan 31, 2017

ok, so

	t.pendingPackets = t.pendingPackets[0:]

looks plausible in codereview, but is actually a pretty dumb thing to do. I wonder if we should find this automatically with go vet.

@hanwen

This comment has been minimized.

Show comment
Hide comment

@hanwen hanwen closed this Feb 6, 2017

@golang golang locked and limited conversation to collaborators Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.