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

Implement OpenSSH strict key exchange extension #1366

Merged
merged 19 commits into from
Apr 24, 2024

Conversation

scott-xu
Copy link
Collaborator

@scott-xu scott-xu commented Mar 30, 2024

The PR implements the algorithm described in section 1.10 of https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.

All integration tests passed, especially *-etm@openssh.com HMAC variants.

Resolves #1285

Terrapin Scanner report without this PR:

Terrapin_Scanner_Windows_amd64.exe --listen 2222
Listening for incoming client connection on 127.0.0.1:2222
================================================================================
==================================== Report ====================================
================================================================================

Remote Banner: SSH-2.0-Renci.SshNet.SshClient.0.0.1

ChaCha20-Poly1305 support:   false
CBC-EtM support:             true

Strict key exchange support: false

The scanned peer is VULNERABLE to Terrapin.

Note: This tool is provided as is, with no warranty whatsoever. It determines
      the vulnerability of a peer by checking the supported algorithms and
      support for strict key exchange. It may falsely claim a peer to be
      vulnerable if the vendor supports countermeasures other than strict key
      exchange.

For more details visit our website available at https://terrapin-attack.com

Terrapin Scanner report with this PR:

Terrapin_Scanner_Windows_amd64.exe --listen 2222
Listening for incoming client connection on 127.0.0.1:2222
================================================================================
==================================== Report ====================================
================================================================================

Remote Banner: SSH-2.0-Renci.SshNet.SshClient.0.0.1

ChaCha20-Poly1305 support:   false
CBC-EtM support:             true

Strict key exchange support: true

The scanned peer supports Terrapin mitigations and can establish
connections that are NOT VULNERABLE to Terrapin. Glad to see this.
For strict key exchange to take effect, both peers must support it.

Note: This tool is provided as is, with no warranty whatsoever. It determines
      the vulnerability of a peer by checking the supported algorithms and
      support for strict key exchange. It may falsely claim a peer to be
      vulnerable if the vendor supports countermeasures other than strict key
      exchange.

For more details visit our website available at https://terrapin-attack.com

@scott-xu scott-xu marked this pull request as ready for review March 30, 2024 14:13
@scott-xu scott-xu requested a review from Rob-Hague March 30, 2024 14:18
@scott-xu scott-xu marked this pull request as draft April 1, 2024 14:20
is only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored
if they are present in subsequent SSH2_MSG_KEXINIT packets.
@scott-xu scott-xu marked this pull request as ready for review April 2, 2024 12:46
@scott-xu scott-xu mentioned this pull request Apr 4, 2024
@scott-xu scott-xu self-assigned this Apr 4, 2024
Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good so far. I know that the integration tests are testing this in the positive case (i.e. they are using strict KEX), but it would make sense to add some tests in the negative cases assuming a bad actor, e.g.

  • When we have strict KEX established, and we receive SSH_MSG_IGNORE (or other) during the key exchange. We should check that the connection fails.
  • Conversely, when we don't have strict KEX established, we should not fail when receiving SSH_MSG_IGNORE (or other) during the key exchange
  • When we have strict KEX established, and the peer does not reset the sequence number. We should check that the connection fails.
  • Maybe some other cases?

test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs is probably a good starting point for simulating connections (either deriving from that or copying stuff out)

src/Renci.SshNet/ConnectionInfo.cs Outdated Show resolved Hide resolved
src/Renci.SshNet/Session.cs Show resolved Hide resolved
src/Renci.SshNet/Session.cs Outdated Show resolved Hide resolved
@scott-xu scott-xu marked this pull request as draft April 9, 2024 13:39
@scott-xu scott-xu marked this pull request as ready for review April 13, 2024 14:15
@scott-xu scott-xu requested a review from Rob-Hague April 13, 2024 14:22
Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the tests. I will run this in production for a week or two

@Rob-Hague Rob-Hague merged commit 94397d4 into sshnet:develop Apr 24, 2024
1 check was pending
@Rob-Hague
Copy link
Collaborator

Thanks

@scott-xu scott-xu deleted the strict-kex branch April 24, 2024 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terrapin attack and strict KEX
2 participants