tlshd: Fix netlink attribute handling - #160
Merged
Merged
Conversation
tlshd_genl_valid_handler() returns NL_STOP when it cannot parse an accept message. libnl reports NL_STOP to its caller as success. nl_recvmsgs_default() returns zero, and tlshd_genl_get_handshake_parms() hands back handshake parameters it never finished filling in. tlshd then services the socket with a sockfd it could not read a peer address from, or with ip_proto left at -1. Return a negative libnl error code instead. recvmsgs() propagates it to nl_recvmsgs_default(), and tlshd_genl_get_handshake_parms() already converts a negative return to EINVAL. tlshd_service_socket() skips the handshake and reports the failure to the kernel. tlshd_genl_event_handler() returns NL_SKIP and nothing else. Drop the NL_OK and NL_STOP retvals its documentation still lists. Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
HANDSHAKE_A_ACCEPT_PEERNAME is optional. The kernel emits it only for a consumer that set ta_peername. Both TLS 1.3 client handshake paths pass parms->peername to strlen() without checking. Only the SUNRPC client requests a client-side handshake today, and it always sets ta_peername, so the dereference is unreachable. Nothing in the protocol keeps it that way. Guarding the strlen() and continuing would trade the dereference for a weaker session. The peer name also reaches gnutls_session_set_verify_cert() and gnutls_certificate_verify_peers3(). Given a NULL hostname, GnuTLS checks the certificate chain but not who presented it. Nothing on the kernel side makes up the difference, because xs_tls_handshake_done() ignores the peerid. The QUIC client path guards its strlen() already, and completes exactly that unverified handshake. Reject the request instead when the peer name is missing. session_status is already EIO, so the early return fails the kernel's request rather than stranding the socket. The QUIC path's test for a peer name is then redundant and goes away. Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
tlshd_genl_valid_handler() fails the handshake request when getnameinfo() cannot map the peer address to a name. That lookup is a fallback for a request that carries no HANDSHAKE_A_ACCEPT_PEERNAME. Only the SUNRPC client sets ta_peername, so the fallback runs for the NFSD and NVMe handshakes, and none of them read the result. A resolver that cannot answer must not fail those handshakes. Leave the name unset and proceed. The client handshake paths are the only ones that need a peer name, and they already reject a request that arrives without one. Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
tlshd_probe_attr() sends a HANDSHAKE_CMD_DONE message carrying the attribute under test and a sockfd of -1, then reads nl_send_auto()'s return to decide whether the kernel accepts that attribute. That return is the count of bytes written to the socket. The kernel has not seen the message yet, so every attribute reads as supported. An administrator who configures session tags for a kernel whose HANDSHAKE_CMD_DONE policy has no HANDSHAKE_A_DONE_TAG entry gets a DONE message the kernel rejects. The handshake completion is lost. Reading the reply does not rescue the probe. The sockfd of -1 that makes the probe harmless is the one sockfd_lookup() rejects, so the reply carries an error whether or not the kernel knows the attribute. Ask the generic netlink control family instead. CTRL_CMD_GETPOLICY dumps the attribute policy for one command, and an attribute the policy rejects is left out of the dump. Per-command policy dumping arrived in v5.10 and the handshake family in v6.5, so every kernel that offers the family can answer. Give the dump its own socket rather than borrowing the notification socket, which by then has joined the tlshd multicast group and has the event handler installed. These control attributes come from the build host's UAPI headers. Nothing else in tlshd requires headers that recent, so a host with pre-v5.10 headers builds the daemon today. Test for them at configure time and record the requirement in README, so that host gets one clear diagnostic instead of a compile failure in netlink.c. Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two of tlshd's responses to a handshake request are inverted.
tlshd_genl_valid_handler() reports a parse failure as NL_STOP, which
libnl hands back to its caller as success, so tlshd services the socket
with parameters it never finished filling in. The same handler fails the
request when getnameinfo() cannot name the peer, and that lookup is a
fallback for the NFSD and NVMe handshakes, which never read its result.
Relaxing that fallback exposes a null dereference.
HANDSHAKE_A_ACCEPT_PEERNAME is optional, so leaving the name unset hands
NULL to the strlen() in both TLS 1.3 client paths. That is why the
client-side check for a missing peer name comes first. After the series a
client handshake request that carries no peer name is rejected instead of
completing a session GnuTLS cannot tie to a peer.
The capability probe never reaches the kernel. It reads nl_send_auto()'s
return, which counts the bytes written to the socket, so every attribute
under test reads as supported. Session tags configured against a kernel
whose HANDSHAKE_CMD_DONE policy has no tag attribute produce a DONE
message the kernel rejects, and the handshake completion is lost. Asking
the generic netlink control family instead pulls CTRL_ATTR_POLICY
definitions from the build host's UAPI headers. Per-command policy
dumping arrived in v5.10 and the handshake family in v6.5, so any kernel
that offers the family can answer, but nothing else in tlshd has needed
headers that recent. configure.ac tests for them and README records the
requirement, so an older build host gets one diagnostic instead of a
compile failure in netlink.c.