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: Can't read final bytes off of TLS socket before close #7414

Closed
bnham opened this issue Feb 25, 2014 · 4 comments
Closed

crypto/tls: Can't read final bytes off of TLS socket before close #7414

bnham opened this issue Feb 25, 2014 · 4 comments
Milestone

Comments

@bnham
Copy link

@bnham bnham commented Feb 25, 2014

What steps will reproduce the problem?

Suppose a server emits some data on a TLS socket and then immediately closes that
socket. In the current implementation of tls.Conn, the client side of that socket may or
may not be able to read those final bytes off the socket if the client is also
concurrently streaming writes to the server.

I've attached a sample client.go and server.go that demonstrate the issue. The client
essentially does this:

 - Writes 1, 2, 3, ... in a loop until a write error occurs
 - Concurrently, does a blocking read for another integer from the server

The server reads integers in a loop from the client until it reads 2048, then echoes
2048 back to the client and exits.

To test, fire up the server with some throwaway self-signed cert and key:

 $ go run server.go -port=10000 -cert=server.crt -key=server.key

Then run the client. Sometimes, the reader wins and is able to read the final message
from the server (2048) before the writer encounters an error:

 $ go run client.go -addr='secure.nhaminated.com:9999' -useTLS
2014/02/24 20:10:14.722902 read value:  2048
2014/02/24 20:10:14.723218 write error on iteration  6642 :  write tcp
162.243.46.153:9999: broken pipe

Other times, the writer wins and an encounters an error before the reader is able to
read the final bytes off the socket:

 $ go run client.go -addr='secure.nhaminated.com:9999' -useTLS
2014/02/24 20:10:23.934537 write error on iteration  6494 :  write tcp
162.243.46.153:9999: broken pipe
2014/02/24 20:10:23.934632 read error:  write tcp 162.243.46.153:9999: broken pipe

In both cases, the server actually wrote this final value of 2048, and the client's TCP
stack received and acked that value, before closing the socket in an orderly fashion
with a FIN. This can be verified by looking at the attached packet trace in wireshark,
using the result.key master key to decrypt the SSL connection.

This issue appears to be bacause tls.Conn has a single connErr field. Both
tls.Conn.Read() and tls.Conn.Write() update this field and return errors from that
field. The issue is that if the writer encounters an error first before the reader, then
the writer sets connErr and the reader can no longer read the final bytes off the
socket, even though those bytes could be read if go were to emit the read() syscall.

You can omit the -useTLS flag on the client and the -key/-cert flags on the server to
confirm that this issue does not occur when using raw sockets.

The type of protocol I've described is extremely widely used in production. For
instance, in Apple's APNs protocol, the client continuously streams push messages with
ids to the server. If the server encounters a malformed push message, it returns the id
of the malformed push message and then closes the socket. It is critical for the client
to be able to read this error message because the server expects the client to
re-connect and restart streaming from the push just after the failed id.

What is the expected output?

The reader should be able to read the last bytes off the socket; in the test program, it
should always be able to read the integer value 2048.

What do you see instead?

Sometimes the reader can read the last value, and sometimes it just returns an error.

Which operating system are you using?

OS X 10.9.2 and Ubuntu 13.10 (Linux 3.11.0-12-generic)

Which version are you using?  (run 'go version' or 'gccgo --version')

go version go1.2 darwin/amd64 and go version go1.2 linux/amd64

Attachments:

  1. server.go (2240 bytes)
  2. client.go (1206 bytes)
  3. result.key (1679 bytes)
  4. result.log (717 bytes)
@bnham

This comment has been minimized.

Copy link
Author

@bnham bnham commented Feb 25, 2014

Comment 1:

typo in the initial server command, it should be "go run server.go -port=9999
-cert=server.crt -key=server.key"
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 26, 2014

Comment 2:

Seems fixable. Thanks for the report.

Labels changed: added release-go1.3maybe, repo-main.

Status changed to Accepted.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Feb 26, 2014

Comment 3:

Thanks for the report! I've mailed out https://golang.org/cl/69090044/ to split
the connErr variable. Sadly this is a pretty subtle operation, but the race detector is
happy.
@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Mar 3, 2014

Comment 4:

This issue was closed by revision 3656c2d.

Status changed to Fixed.

@bnham bnham added fixed labels Mar 3, 2014
@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
Currently a write error will cause future reads to return that same error.
However, there may have been extra information from a peer pending on
the read direction that is now unavailable.

This change splits the single connErr into errors for the read, write and
handshake. (Splitting off the handshake error is needed because both read
and write paths check the handshake error.)

Fixes golang#7414.

LGTM=bradfitz, r
R=golang-codereviews, r, bradfitz
CC=golang-codereviews
https://golang.org/cl/69090044
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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