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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nwtgck
Copy link

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
Copy link
Contributor

bcmills commented Aug 26, 2019

CC @FiloSottile

@bcmills bcmills added Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 26, 2019
@bcmills bcmills added this to the Go1.14 milestone Aug 26, 2019
@gopherbot
Copy link

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

@FiloSottile FiloSottile added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 26, 2019
@FiloSottile FiloSottile modified the milestones: Go1.14, Go1.13 Aug 26, 2019
@nwtgck
Copy link
Author

nwtgck commented Aug 26, 2019

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

@FiloSottile
Copy link
Contributor

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
Copy link

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

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
Copy link
Author

nwtgck commented Aug 27, 2019

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

@FiloSottile
Copy link
Contributor

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

@nwtgck
Copy link
Author

nwtgck commented Aug 27, 2019

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

tomocy pushed 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 pushed 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>
@golang golang locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants