-
Notifications
You must be signed in to change notification settings - Fork 1.2k
server: ignore more error kinds in incoming socket stream #1885
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
Conversation
|
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). |
I could see why Tokio wouldn't want to be so opinionated -- |
grembo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, here's my approval.
| e.kind(), | ||
| io::ErrorKind::ConnectionAborted | ||
| | io::ErrorKind::Interrupted | ||
| | io::ErrorKind::WouldBlock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | io::ErrorKind::WouldBlock | |
| | io::ErrorKind::WouldBlock | |
| | io::ErrorKind::InvalidData |
When we got a TLS handshake error, A InvalidData error will happen here. see: #1897
|
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. |
|
Might be nice to get this into 0.12.3. |
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. |
|
Its been published as a GHSA-4jwc-w2hc-78qv and is a low sev CVE. RustSec PR will follow. |
As discussed in #1882 (comment).
cc @grembo