server: ignore more error kinds in incoming socket stream

[djc]

As discussed in #1882 (comment).

cc @grembo

[grembo]

Looks good to me. In theory, WouldBlock is already filtered in TcpStream, but having all the transient ones checked in the same place is better 👍

I wonder if it would be a good idea to add/move these checks to tokio/net in the long run (around https://github.com/tokio-rs/tokio/blob/master/tokio/src/net/tcp/listener.rs#L186, where it already handled WouldBlock).

[djc]

I wonder if it would be a good idea to add/move these checks to tokio/net in the long run (around https://github.com/tokio-rs/tokio/blob/master/tokio/src/net/tcp/listener.rs#L186, where it already handled WouldBlock).

I could see why Tokio wouldn't want to be so opinionated -- WouldBlock is more clear-cut because that is part of the whole async stack.

[grembo]

FWIW, here's my approval.

[zier-one]

Suggested change

	| io::ErrorKind::WouldBlock
	| io::ErrorKind::WouldBlock
	| io::ErrorKind::InvalidData

When we got a TLS handshake error, A InvalidData error will happen here. see: #1897

[zier-one]

Should we maybe add more tests about TLS? It is really a critical bug that the server process exits when a TLS handshake error happened. Clients can easily perform denial-of-service attacks.

[djc]

Might be nice to get this into 0.12.3.

[lap1nou]

Should we maybe add more tests about TLS? It is really a critical bug that the server process exits when a TLS handshake error happened. Clients can easily perform denial-of-service attacks.

Hello,

Sorry to intervene in that merged PR, but shouldn't that bug have it's own CVE number ? I encountered that bug while developping my application and indeed this seems like a nice primitive to crash any gRPC TLS server running Tonic 0.12.2 (or less ?).

Regards.

[LucioFranco]

Its been published as a GHSA-4jwc-w2hc-78qv and is a low sev CVE. RustSec PR will follow.