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: set timeout so that Close does not block indefinitely #31224

Open
NWilson opened this issue Apr 3, 2019 · 21 comments
Open

crypto/tls: set timeout so that Close does not block indefinitely #31224

NWilson opened this issue Apr 3, 2019 · 21 comments
Assignees
Milestone

Comments

@NWilson
Copy link

@NWilson NWilson commented Apr 3, 2019

What did you do?

Reading the documentation for tls.Conn (https://golang.org/pkg/crypto/tls/) the deadline behaviour is not clearly explained.

  • It's not obvious that Read calls may block forever unless the write deadline is set to a non-zero time. (This is because handshaking is done internally, which writes.) Since this is different to net.TLSConn, it should be called out.
  • It's reasonable to understand that Handshake calls need to have both read and write deadline set... but could be called out explicitly.
  • But the killer: calling Close on a tls.Conn risks blocking forever, unless the write deadline has been set! (It writes out a close alert.)

This last one is the worst. It would be very easy to have a defer'ed call to Close, and assume that it will always return promptly, when in fact it may be possible for a malicious client to fill up the socket buffers with handshaking data, so that writing the close alert blocks (on an OS with tiny socket buffers?).

I have (or had) some code that does the following:

for {
  conn, _ := listener.Accept() // error handling omitted for clarity
  if tooManyConns() {
    conn.Close() // oops, may block!
    continue
  }
  go handleConn(conn)
}

I have now carefully gone and wrapped all calls to Close with a safeClose wrapper method that calls SetWriteDeadline first!

Perhaps Close should actually call SetWriteDeadline on the underlying connection with a sane value (eg time.Now().Add(100*time.Millisecond)) to ensure that no-one else is caught out.

Thankfully, http.conn.serve does call SetWriteDeadline as soon as possible for TLS connections, and never sets a zero write deadline, so that code at least is safe, and can't call Close with a zero deadline.

What did you expect to see?

More mention of what deadlines need to set, to prevent each method blocking.

@bradfitz bradfitz changed the title tls.Conn.Close documentation should explain blocking crypto/tls: Conn.Close documentation should explain blocking Apr 3, 2019
@andybons andybons added this to the Unplanned milestone Apr 3, 2019
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Apr 3, 2019

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Apr 5, 2019

Assuming the above points are all correct, a lot of folks use the tls API incorrectly. I have a TLS+TCP server that calls SetReadDeadline() and Close(). I don't recall mention of SetWriteDeadline() in any example code I read.

Maybe this calls for additional APIs, e.g. SetTLSReadDeadline() & CloseTLS()? Or a go vet item?

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jun 18, 2019

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jun 26, 2019

@andybons, could this be milestoned for 1.13 and made release-blocker?

Correct use of TLS is a security concern, and one of the suggested fixes is to change .Close() behavior.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jun 26, 2019

I agree this needs addressing, but it does not warrant changing subtle behavior while in deep feature freeze. The only fix for Go 1.13 will be probably in documentation, and we can discuss in 1.14 with more flexibility.

@FiloSottile FiloSottile modified the milestones: Unplanned, Go1.13 Jun 26, 2019
@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jul 3, 2019

Thanks, happy to give feedback on the CL when ready...

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jul 16, 2019

@FiloSottile, you tagged this for 1.13. It's now been re-tagged for 1.14; was that a mistake?

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Sep 11, 2019

@FiloSottile we might want to tackle this one early in 1.14 cycle...

@gopherbot add release-blocker

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 1, 2019

I'm thinking of introducing a default deadline on the close notify send, and document in Read and Write, where they say they call Handshake, that that means they need both read and write deadlines.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 1, 2019

Re setting a default deadline, is there any conflict with http.Server/Client? See #21133 (comment)

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 1, 2019

Reusing a net.Conn after a tls.Close is weird but I suppose not impossible? Sure, we can just reset it before leaving Close.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 1, 2019

Just wondering whether the implicit deadlines that http uses might conflict with an implicit deadline in tls.Close?

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 1, 2019

Ah, well by the time tls.Close is called it doesn't really matter, does it? We are tearing down, so we can just override them.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 1, 2019

@bradfitz any thoughts?

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 2, 2019

I haven't looked into this issue, but from a quick skim of this thread, it does indeed seem like things are too complicated/surprising for users.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 2, 2019

@bradfitz any comment re a new implicit timer in tls.Close? Could that conflict with implicit timers in http?

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 2, 2019

A new timer in tls.Close sounds good. If crypto/tls doesn't and net/http doesn't already(?), then it seems that net/http should work around any such blocking and close the underlying net.Conn directly. But it might be already. I'd have to go refresh my memory of the code.

In any case, crypto/tls doing it (regardless of whether a higher layer is also doing it) sounds good.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 5, 2019

Looks like no change was made for 1.14, so retargeting for 1.15. Also looks like the current thinking is to change the code, so dropping the documentation label.

@ianlancetaylor ianlancetaylor changed the title crypto/tls: Conn.Close documentation should explain blocking crypto/tls: set timeout so that Close does not block indefinitely Dec 5, 2019
@ianlancetaylor ianlancetaylor removed this from the Go1.14 milestone Dec 5, 2019
@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 5, 2019

This issue entails both:
a docs fix re write deadlines (which should be made ASAP, hence the release-blocker), and
a change to .Close()

Could you put back 1.14 and release-blocker, and we'll change that after the docs fix lands?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 6, 2019

If we change the Close method then we may not need a doc change.

But if anyone wants to send a doc change, go ahead. We can always take doc changes even if the issue is targeted to a later release.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 6, 2019

I don't think a timer in tls.Close() prevents .Read() blocking indefinitely if there's no write deadline...

@gopherbot add "help wanted"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.