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

fix(http): use insecure cipher suites #4753

Conversation

gnuletik
Copy link

Proposed changes

Go 1.22 disabled the RSA key exchange cipher suites:
golang/go#63413

In order to be able to scan servers only supporting these cipher suites, the http client needs to explicitly support these.

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@geeknik
Copy link
Contributor

geeknik commented Feb 12, 2024

Shouldn't MinVersion also be set to VersionSSL30?

@tarunKoyalwar
Copy link
Member

As usual, the new GODEBUG would only be set in go 1.22 programs. Setting GODEBUG=tlsrsakex=1 would re-enable these suites.

we can try using this env , doing this (in retryablehttp) will reflect this change across other nuclei projects as well

@olearycrew
Copy link
Contributor

Thanks for this contribution @gnuletik !

@gnuletik
Copy link
Author

gnuletik commented Feb 12, 2024

@geeknik It seems tat VersionSSL30 is not supported anymore: https://pkg.go.dev/crypto/tls#pkg-constants

// Deprecated: SSLv3 is cryptographically broken, and is no longer
// supported by this package. See golang.org/issue/32716.

@tarunKoyalwar It seems that the GODEBUG variable is used at runtime, which seems tricky to use and not compatible with library usage.
I made a similar PR on retryablehttp package: projectdiscovery/retryablehttp-go#215
Should I also do it in httpx ?

@geeknik
Copy link
Contributor

geeknik commented Feb 12, 2024

@geeknik It seems tat VersionSSL30 is not supported anymore: pkg.go.dev/crypto/tls#pkg-constants

// Deprecated: SSLv3 is cryptographically broken, and is no longer
// supported by this package. See golang.org/issue/32716.

Ah well, that's too bad.

@gnuletik
Copy link
Author

Hi, is there anything blocking a merge here? Thanks!

@tarunKoyalwar
Copy link
Member

tarunKoyalwar commented Feb 17, 2024

@gnuletik , thanks for the pr but looking back at fastdialer it looks like this is implicitly covered. all pd tools internally use fastdialer which has auto ztls fallback (zcrypto project) and whenever a client hello or handshake fails , it reattempts connection with insecure ciphers and other options (see: https://github.com/projectdiscovery/fastdialer/blob/132fe30bd4812559e45ca164565f52089cb0d345/fastdialer/dialer.go#L384-L406) . and ztls supports ~350 ciphers https://github.com/projectdiscovery/tlsx/blob/f60f2bac3f2fd90c4d34ead0eea45758b520a47f/pkg/tlsx/ztls/utils.go#L81-L426 . so i think we can say this is already convered

and about 1.22 cipher disabling , if i am not wrong this will not effect nuclei and other pd projects now, even if you compile it using go 1.22 because of go directive

The go directive affects use of new language features:

For packages within the module, the compiler rejects use of language features introduced after the version specified by the go directive. For example, if a module has the directive go 1.12, its packages may not use numeric literals like 1_000_000, which were introduced in Go 1.13.
If an older Go version builds one of the module’s packages and encounters a compile error, the error notes that the module was written for a newer Go version. For example, suppose a module has go 1.13 and a package uses the numeric literal 1_000_000. If that package is built with Go 1.12, the compiler notes that the code is written for Go 1.13.

from https://go.dev/ref/mod#go-mod-file-go

this is because language features like deprecation of certain ciphers etc only affect if we explicitly bump go version in go.mod file and we have not done that yet and there's isn't any good reason to bump it unless we decide to use any 1.22 specific language features .

i'm closing this PR , if you think otherwise feel free to reopen this / submit new one

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.

None yet

4 participants