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

crypto/tls: TLS 1.0 is not min version by default in HTTP server #33837

Closed
nwtgck opened this issue Aug 26, 2019 · 8 comments
Closed

crypto/tls: TLS 1.0 is not min version by default in HTTP server #33837

nwtgck opened this issue Aug 26, 2019 · 8 comments
Labels
Milestone

Comments

@nwtgck
Copy link

@nwtgck nwtgck commented Aug 26, 2019

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

$ go version
go version go1.12.9 darwin/amd64

Does this issue reproduce with the latest release?

Yes. 1.12.9 is the latest release now.

What did you do? - Test code

Here is very simple HTTPS server code. You can choose &tls.Config{} or &tls.Config{MinVersion: tls.VersionTLS10} by the comment.

// main.go
package main

import (
	"crypto/tls"
	"fmt"
	"log"
	"net/http"
)

func main() {
	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		_, _ = fmt.Fprintln(w, "<h1>hello, world</h1>")
	})

	// Start HTTP server
	fmt.Println("Listening...")

	// TLS config
	// NOTE: Switch by comment
	tlsConfig := &tls.Config{}
	//tlsConfig := &tls.Config{MinVersion: tls.VersionTLS10}

	server := &http.Server{Addr: ":8443", Handler: handler, TLSConfig: tlsConfig}
	// Start HTTPS server
	if err := server.ListenAndServeTLS("./ssl_certs/server.crt", "./ssl_certs/server.key"); err != nil {
		log.Fatal(err.Error())
	}
}

Run

go run main.go

What did you expect to see? & What did you see instead?

Testing

I use the following command to confirm whether SSLv3 is supported or not.

openssl s_client -connect localhost:8443 -ssl3

When using &tls.Config{}, SSLv3 is NOT rejected.
When using &tls.Config{MinVersion: tls.VersionTLS10}, SSLv3 is rejected and the server emits "http: TLS handshake error from 127.0.0.1:65528: tls: client offered only unsupported versions: [300]", which is my expectation.

I expect both &tls.Config{} and &tls.Config{MinVersion: tls.VersionTLS10} cases reject SSLv3, but that was not true. The official document says "If zero, then TLS 1.0 is taken as the minimum" like the following.

Document

// MinVersion contains the minimum SSL/TLS version that is acceptable.
// If zero, then TLS 1.0 is taken as the minimum.
MinVersion uint16

from: tls - The Go Programming Language

In the document, MinVersion should be TLS 1.0, so I think it rejects SSLv3. However, the server allows SSLv3.

@bcmills bcmills changed the title TLS 1.0 is not min version by default in HTTP server crypto/tls: TLS 1.0 is not min version by default in HTTP server Aug 26, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Aug 26, 2019

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 26, 2019

Change https://golang.org/cl/191877 mentions this issue: crypto/tls: make SSLv3 again disabled by default

@nwtgck

This comment has been minimized.

Copy link
Author

@nwtgck nwtgck commented Aug 26, 2019

Is this vulnerable? In default, HTTP server in Go 1.12 has a possibility of downgrade attack?

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Aug 27, 2019

No, SSLv3 is broken, but not so broken that TLS connections can be downgraded to it arbitrarily by an attacker. Virtually all clients don't do insecure fallbacks anymore (and haven't for years), and in any case we support TLS_FALLBACK_SCSV.

This does not pose a threat to clients that support TLS, but it will allow clients that only support SSLv3 to connect insecurely, which should not be the case.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 27, 2019

Change https://golang.org/cl/191998 mentions this issue: [release-branch.go1.13] crypto/tls: make SSLv3 again disabled by default

@gopherbot gopherbot closed this in 2ebc3d8 Aug 27, 2019
gopherbot pushed a commit that referenced this issue Aug 27, 2019
It was mistakenly re-enabled in CL 146217.

Updates #33837

Change-Id: I8c0e1787114c6232df5888e51e355906622295bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/191877
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
(cherry picked from commit 2ebc3d8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/191998
@nwtgck

This comment has been minimized.

Copy link
Author

@nwtgck nwtgck commented Aug 27, 2019

@FiloSottile Thank you very much for your explanation and fix!

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Aug 27, 2019

Thank you for catching this just in time for Go 1.13!

@nwtgck

This comment has been minimized.

Copy link
Author

@nwtgck nwtgck commented Aug 27, 2019

@FiloSottile I'm looking forward to Go 1.13 release!

tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
It was mistakenly re-enabled in CL 146217.

Fixes golang#33837

Change-Id: I8c0e1787114c6232df5888e51e355906622295bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/191877
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
It was mistakenly re-enabled in CL 146217.

Fixes golang#33837

Change-Id: I8c0e1787114c6232df5888e51e355906622295bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/191877
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dtomcej dtomcej mentioned this issue Sep 10, 2019
1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.