Skip to content

x/crypto/ssh: diffie-hellman-group16/18-sha512 #61381

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.

Sign up for GitHub

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

Jump to bottom
Closed
drakkan opened this issue Jul 16, 2023 · 1 comment
Closed

x/crypto/ssh: diffie-hellman-group16/18-sha512 #61381

drakkan opened this issue Jul 16, 2023 · 1 comment
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@drakkan
Copy link
Member

drakkan commented Jul 16, 2023 •
edited
Loading

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

$ go version
go version go1.20.6 linux/amd64

I added support for diffie-hellman-group16/18-sha512 in CL 506839.

The CL includes a benchmark, here are the results

name                                           time/op
Kexes/diffie-hellman-group-exchange-sha256-12  22.6ms ± 9%
Kexes/diffie-hellman-group18-sha512-12          1.15s ±11%
Kexes/ecdh-sha2-nistp384-12                    3.91ms ± 6%
Kexes/ecdh-sha2-nistp256-12                     304µs ± 5%
Kexes/curve25519-sha256@libssh.org-12           413µs ± 7%
Kexes/ecdh-sha2-nistp521-12                    11.6ms ±13%
Kexes/curve25519-sha256-12                      361µs ± 5%
Kexes/diffie-hellman-group-exchange-sha1-12    22.9ms ± 9%
Kexes/diffie-hellman-group1-sha1-12            3.59ms ± 6%
Kexes/diffie-hellman-group14-sha1-12           22.1ms ±11%
Kexes/diffie-hellman-group14-sha256-12         21.6ms ± 8%
Kexes/diffie-hellman-group16-sha512-12          138ms ± 9%

name                                           alloc/op
Kexes/diffie-hellman-group-exchange-sha256-12  67.8kB ± 1%
Kexes/diffie-hellman-group18-sha512-12          243kB ± 9%
Kexes/ecdh-sha2-nistp384-12                    13.9kB ± 0%
Kexes/ecdh-sha2-nistp256-12                    12.1kB ± 0%
Kexes/curve25519-sha256@libssh.org-12          8.22kB ± 0%
Kexes/ecdh-sha2-nistp521-12                    16.5kB ± 0%
Kexes/curve25519-sha256-12                     8.22kB ± 0%
Kexes/diffie-hellman-group-exchange-sha1-12    67.5kB ± 0%
Kexes/diffie-hellman-group1-sha1-12            34.9kB ± 0%
Kexes/diffie-hellman-group14-sha1-12           61.9kB ± 0%
Kexes/diffie-hellman-group14-sha256-12         62.0kB ± 0%
Kexes/diffie-hellman-group16-sha512-12          117kB ± 0%

name                                           allocs/op
Kexes/diffie-hellman-group-exchange-sha256-12     314 ± 0%
Kexes/diffie-hellman-group18-sha512-12            271 ± 4%
Kexes/ecdh-sha2-nistp384-12                       243 ± 0%
Kexes/ecdh-sha2-nistp256-12                       213 ± 0%
Kexes/curve25519-sha256@libssh.org-12             168 ± 0%
Kexes/ecdh-sha2-nistp521-12                       245 ± 0%
Kexes/curve25519-sha256-12                        168 ± 0%
Kexes/diffie-hellman-group-exchange-sha1-12       314 ± 0%
Kexes/diffie-hellman-group1-sha1-12               255 ± 0%
Kexes/diffie-hellman-group14-sha1-12              255 ± 0%
Kexes/diffie-hellman-group14-sha256-12            255 ± 0%
Kexes/diffie-hellman-group16-sha512-12            256 ± 0%

and here is the profiler output for diffie-hellman-group18-sha512

Showing top 10 nodes out of 116
      flat  flat%   sum%        cum   cum%
    72.13s 44.55% 44.55%     72.13s 44.55%  math/big.addMulVVW
    16.03s  9.90% 54.45%     90.60s 55.96%  math/big.nat.montgomery
    10.10s  6.24% 60.69%     10.43s  6.44%  crypto/internal/nistec/fiat.p384Mul
     8.25s  5.10% 65.78%      8.25s  5.10%  crypto/internal/edwards25519/field.feMul
     7.91s  4.89% 70.67%      8.03s  4.96%  crypto/internal/nistec/fiat.p521Mul
     6.62s  4.09% 74.76%      6.62s  4.09%  crypto/internal/edwards25519/field.feSquare
     4.71s  2.91% 77.67%      4.71s  2.91%  p256MulInternal
     2.46s  1.52% 79.19%      2.52s  1.56%  crypto/internal/nistec/fiat.p384Square
     2.34s  1.45% 80.63%      2.34s  1.45%  crypto/internal/edwards25519/field.(*Element).carryPropagateGeneric
     2.21s  1.36% 82.00%      2.28s  1.41%  crypto/internal/nistec/fiat.p521Square

So these kexes are really slow in Go and could easily cause a DoS.

Considering there are a number of more modern kexes based on elliptic curves, which are faster to process and as secure and that supporting diffie-hellman-group14-sha256 should be enough for interoperability, I think 506839 should not be merged, at least for now. The CL is still useful because we can run the benchmark with newer Go versions and see if the performance improves in the future.

profile.zip

cc @golang/security
@gopherbot gopherbot added this to the Unreleased milestone Jul 16, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 17, 2023
@drakkan
Copy link
Member Author

drakkan commented Nov 15, 2023

CL 506839 was merged, we added diffie-hellman-group16-sha512

@drakkan drakkan closed this as completed Nov 15, 2023
@golang golang locked and limited conversation to collaborators Nov 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@drakkan @gopherbot @seankhliao