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: client did not fail when lack of a certificate and server requires one #59012

Closed
codemaker219 opened this issue Mar 13, 2023 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@codemaker219
Copy link

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

$ go version
go version go1.20.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="0"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2979227275=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I was playing around with mtls in golang. During testing error cases I am not sure if golang behaves correctly.
If I configure the server to force client authentication with RequireAndVerifyClientCert the client seems not to get the info, that the tls handshake was not successful.
Try this:

package main

import (
	"crypto/tls"
	"crypto/x509"
	"fmt"
	"strconv"
	"sync"
)

const CRT = "-----BEGIN CERTIFICATE-----\nMIIDHzCCAgegAwIBAgIGAYbbjzNfMA0GCSqGSIb3DQEBCwUAMBQxEjAQBgNVBAMM\nCTEyNy4wLjAuMTAeFw0yMzAzMTMxNTIwNDBaFw0yNDAzMTMxNTIwNDBaMBQxEjAQ\nBgNVBAMMCTEyNy4wLjAuMTCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB\nAIlfhPzb31DD3ntWPLU2HSqb0z0dkns5hNQt3EN907wT2H+wnhPu9W21nOrQNk6D\nRPfs+2qzpuzMzbC1AAEz/YFrgctUgc7IDlMWOhmKusROb/VA4596jtIQpu35gUnO\niZ16V4ufUEGMn2ZNOB1FTlp+vTOjDvcEgXyEvwTrHjBAKf7/h8Ab8yQrEMZJLjNy\nUrC8IBtUKGr0qcYSBnbT35Y6weT0prZUi21trBQNtQQtegKnN3AssZnkUar3oc1M\nPSfEO385QbTj5P0RV6lYCdNNzlzZiyf89qdK1GwqjWIC5NMEfhgCWBmdwo4SHUUo\nPELmFekxNF1CC1fqARHke/ECAwEAAaN3MHUwDwYDVR0TAQH/BAUwAwEB/zAfBgNV\nHSMEGDAWgBQFgC5rEruxtIhgyck1mfJWlgnYVzAdBgNVHQ4EFgQUBYAuaxK7sbSI\nYMnJNZnyVpYJ2FcwDgYDVR0PAQH/BAQDAgGGMBIGA1UdEQEB/wQIMAaHBH8AAAEw\nDQYJKoZIhvcNAQELBQADggEBAHPFf7OJxMHuc7NpIR/qY9gWVyICwsDVN/V92ZDm\ng4IVhiQSyTYmtjQwbN848WRtihw3aR4FX1nvaDggHoHgdH2ug1pcdUUV4CjGTqaH\nCH5IRkg7SrlC2GimPS1LKGlB9a+nruFMevPez1Cn5OyFJ87mjcVsg+iHdhOXJD6M\ng3aN+UuslneYeLhmnupjJw4c7hZtQM8TfiuZ5ck+nGMupDXFZo5iBCS+fx9l8e3b\ntJUsmov2OmEDC+7lqkds4nBAj7GCvaD7vJr2bmwj3tNY1VURugQSIhuzWTAxohry\nM+GhLnsX+JNAU2e8E9cxCcceC8IaPAbX8y1teGCt9ukIxhE=\n-----END CERTIFICATE-----\n"
const KEY = "-----BEGIN RSA PRIVATE KEY-----\nMIIEogIBAAKCAQEAiV+E/NvfUMPee1Y8tTYdKpvTPR2SezmE1C3cQ33TvBPYf7Ce\nE+71bbWc6tA2ToNE9+z7arOm7MzNsLUAATP9gWuBy1SBzsgOUxY6GYq6xE5v9UDj\nn3qO0hCm7fmBSc6JnXpXi59QQYyfZk04HUVOWn69M6MO9wSBfIS/BOseMEAp/v+H\nwBvzJCsQxkkuM3JSsLwgG1QoavSpxhIGdtPfljrB5PSmtlSLbW2sFA21BC16Aqc3\ncCyxmeRRqvehzUw9J8Q7fzlBtOPk/RFXqVgJ003OXNmLJ/z2p0rUbCqNYgLk0wR+\nGAJYGZ3CjhIdRSg8QuYV6TE0XUILV+oBEeR78QIDAQABAoIBACYEJaD2VgwbUGAQ\ngvdhFNw8SE6S9v0b81rmoBybXzOeyFy561031Xq5dkXzPfwnTrhPwFoMgobovImI\n5YnvsdmVf1NePRgU/AXZUlXMMxhtXoVgIj35pDmU+yVDVZivzBylBUIx4ftp55jf\niMZs7hyUE1cNanBIsm7bq6M4T9/pEf41Ayf0XgkMOkjn2RpZCTdwvDASw2L0hzVv\nf9lZow1LecARdwD8dMvQ0oI7P4hSCzfTCi6JLioxLuu+8yw20Rn9/r2xV2UErPhe\nxhgxAUBLwVMlArb1c+ZVXFZpx+e1tmwmqTeAKptmFQNQkljwAEQB+6GEKhV01sQ9\nCG94SlMCgYEAwWMqdGX/KkXcCkjLbuu7qMspbI9zwoXdKc2eqp5sS6jex/gzS/Y6\nx0yMTq3wUIbHfDcVlRu9cmadW483I7DZCnRX7R0GFcW6YKA/Uog+8nykd5zA/aG8\na5tALJEd3JRNihHchxHwYpQEdklzfksN/i20CxRTehlyAQMs+jJBEz8CgYEAtdmi\ntR6YbwwxhRn7/kX/H39sEsb00sFRbSCTv8TR6nuBfmjISLY949PJ4TELj7PW7vgT\n1Rut6lsR7dSyqv6+p0J6pNfu90BUni+HRDtQyI60qiMhXqfHxpEUofkK6/er0yxN\no+Wp0NQa0788rGhNR7deTGrvQAwAl+tX2MX7FM8CgYBhrMGLqtyXMFO0ChJeAsh0\nt7BDea0BKfWLoKQsDvopuLBVFeJq0oHbmakgMDA5q+ljrrrC5hDokDWYQhyadT8a\nTar/QvKI2qaJGUcCW3hXp2a2V0EOmbr+KpreJ6mKeIk1XFIjEod0cshSKkUgG66E\nm/bcxbZn7CQtqLn14J4HpwKBgDIqQn9SXFyt8W81VtWzO4jV3ttjNSB6odrH1Npf\nEkVsIrHbd/zPAU27HALaQ8U1qEIt/1KXmyd+Tfjc5xjSw4diiHC2/L4Kag1lMRx6\nfHOHIoGYxVjWUf8OALIaAJBNt4G+xABFl8365ReqtrMc5sy04feUvEFfzj4adxZe\nrz/zAoGAU502B+/HvgdodBzdw2iQJseEG1eNaUoA1co2OxHUHlaPukt5RjOwuXrP\nobVdqbtBb9aa4ZbRl9Y1yvllJMTSzrWvWra1eV3czQOVS34FwaJ06AzaUCR7ht7D\nVOObYItn69ougXDjKwHR9t/q02fLVMmUAYBKEqaVWErMzJnssrQ=\n-----END RSA PRIVATE KEY-----\n"

const PORT = 1555

var wg sync.WaitGroup

func main() {
	startServer()

	config := &tls.Config{
		RootCAs: x509.NewCertPool(),
	}
	config.RootCAs.AppendCertsFromPEM([]byte(CRT))

	con, err := tls.Dial("tcp", "127.0.0.1:"+strconv.Itoa(1555), config)
	if err != nil {
		fmt.Println("I do not have a client cert, I should reach here")
		panic(err)
	}
	defer con.Close()

	n, err := con.Write([]byte("msg"))
	if err != nil {
		fmt.Println("At least here should be a error")
		panic(err)
	}
	fmt.Printf("Wrote %d bytes\n", n)
	wg.Wait()

}

func startServer() {
	wg.Add(1)
	cert, _ := tls.X509KeyPair([]byte(CRT), []byte(KEY))
	config := &tls.Config{
		Certificates: []tls.Certificate{cert},
		ClientAuth:   tls.RequireAndVerifyClientCert,
		ClientCAs:    x509.NewCertPool(),
	}
	config.ClientCAs.AppendCertsFromPEM([]byte(CRT))

	socket, _ := tls.Listen("tcp", ":"+strconv.Itoa(PORT), config)
	buffer := make([]byte, 1024)

	go func() {
		defer wg.Done()
		defer socket.Close()
		conn, _ := socket.Accept()

		n, _ := conn.Read(buffer)
		fmt.Println(string(buffer[:n]))
		fmt.Printf("Read %d bytes\n", n)
		conn.Close()

	}()
}



What did you expect to see?

On tls.Dial or at least on n, err := con.Write([]byte("msg")) an error should be returned since the client was not able to authenticate (Dint send any client certificate). It seems like the client thinks that everything is going well...

What did you see instead?

You will see Wrote 3 bytes produced by the client part, but the server part didnt received the message msg

@cherrymui cherrymui changed the title affected/package: crypto/tls: client did not fail when lack of a certificate and server requires one Mar 13, 2023
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 13, 2023
@cherrymui cherrymui added this to the Backlog milestone Mar 13, 2023
@cherrymui
Copy link
Member

cc @golang/security

@rittneje
Copy link

rittneje commented Mar 14, 2023

Because of how TLS 1.3 works, the client won't notice the issue until the first call to Read within the client.

In TLS 1.3 the client is the last one to speak in the handshake, so if it causes an error to occur on the server, it will be returned on the client by the first Read, not by Handshake.

Currently this information is only documented in the 1.12 release notes. It would be good to add it to crypto/tls.

@codemaker219
Copy link
Author

Thanks for the investigation.
From my point of view, this seems to be a little problematic in some usecases. For example in IoT or syslog etc.., the server would not send any data. If I try now to call Read with an empty slice, Read will return 0 and also no error.
So with this behavior, a can call Read with an non empty slice and get the error (yeah). But if everything is fine, we are stuck now in the Read method since the server will not send any data.
I hope I could explain my concerns in a understandable way.

I stumbled over this, since I want to write a very simple syslog implementation, that sends logs to other syslog sinks (like rsyslog etc.)

@zerkms
Copy link

zerkms commented Mar 14, 2023

@codemaker219 checking prior to your first real interaction does not really add much benefit because of TOCTOU

@codemaker219
Copy link
Author

@zerkms Write is an interaction, or?

@codemaker219
Copy link
Author

codemaker219 commented Mar 15, 2023

Now I thought a little bit more about it. If the Write would now also return the error, there are still usecases where this is not suitable. Imagine a tool thats purpose is to check tls configurations. You give this tool a ca, crt and key. And this tool should says if it can establish a tls connection. Since it should be working for all kinds services, its a bad idea to assume that we will receive data or what happens when we send data.
So in conclusion: The error should be return, if the user (programmer) calls maybe the Handshake function because he would expect that the tls connection is established. Or there should be a new function FinialzeTlsHandshake or whatever, that can than return the error.

@rittneje
Copy link

rittneje commented Mar 15, 2023

@codemaker219 What you want is simply incompatible with how TLS 1.3 works. You have to read from the connection to tell if the server rejected you. But if it didn't reject you, then reading from the connection will either block indefinitely, or irreversibly consume some actual data.

I will also point out that any application that only writes data to a remote TCP connection is fundamentally broken in the event of a half-closed connection. In that case, you need to use application-level pings/acks, at which point your TLS 1.3 issue is resolved.

@codemaker219
Copy link
Author

@rittneje Thanks for the reply. I did also some research and it seems that this behavior is indeed not a bug. So I will close that issue even I think that this could be improved.

@golang golang locked and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants