6.14-stable review patch. If anyone has any objections, please let me know.
------------------
From: Konstantin Andreev andreev@swemel.ru
[ Upstream commit 6cce0cc3861337b3ad8d4ac131d6e47efa0954ec ]
Since inception [1], SMACK initializes ipv* child socket security for connection-oriented communications (tcp/sctp/dccp) during accept() syscall, in the security_sock_graft() hook:
| void smack_sock_graft(struct sock *sk, ...) | { | // only ipv4 and ipv6 are eligible here | // ... | ssp = sk->sk_security; // socket security | ssp->smk_in = skp; // process label: smk_of_current() | ssp->smk_out = skp; // process label: smk_of_current() | }
This approach is incorrect for two reasons:
A) initialization occurs too late for child socket security:
The child socket is created by the kernel once the handshake completes (e.g., for tcp: after receiving ack for syn+ack).
Data can legitimately start arriving to the child socket immediately, long before the application calls accept() on the socket.
Those data are (currently — were) processed by SMACK using incorrect child socket security attributes.
B) Incoming connection requests are handled using the listening socket's security, hence, the child socket must inherit the listening socket's security attributes.
smack_sock_graft() initilizes the child socket's security with a process label, as is done for a new socket()
But ... the process label is not necessarily the same as the listening socket label. A privileged application may legitimately set other in/out labels for a listening socket.
When this happens, SMACK processes incoming packets using incorrect socket security attributes.
In [2] Michael Lontke noticed (A) and fixed it in [3] by adding socket initialization into security_sk_clone_security() hook like
| void smack_sk_clone_security(struct sock *oldsk, struct sock *newsk) | { | *(struct socket_smack *)newsk->sk_security = | *(struct socket_smack *)oldsk->sk_security; | }
This initializes the child socket security with the parent (listening) socket security at the appropriate time.
I was forced to revisit this old story because
smack_sock_graft() was left in place by [3] and continues overwriting the child socket's labels with the process label, and there might be a reason for this, so I undertook a study.
If the process label differs from the listening socket's labels, the following occurs for ipv4:
assigning the smk_out is not accompanied by netlbl_sock_setattr, so the outgoing packet's cipso label does not change.
So, the only effect of this assignment for interhost communications is a divergence between the program-visible “out” socket label and the cipso network label. For intrahost communications this label, however, becomes visible via secmark netfilter marking, and is checked for access rights by the client, receiving side.
Assigning the smk_in affects both interhost and intrahost communications: the server begins to check access rights against an wrong label.
Access check against wrong label (smk_in or smk_out), unsurprisingly fails, breaking the connection.
The above affects protocols that calls security_sock_graft() during accept(), namely: {tcp,dccp,sctp}/{ipv4,ipv6} One extra security_sock_graft() caller, crypto/af_alg.c`af_alg_accept is not affected, because smack_sock_graft() does nothing for PF_ALG.
To reproduce, assign non-default in/out labels to a listening socket, setup rules between these labels and client label, attempt to connect and send some data.
Ipv6 specific: ipv6 packets do not convey SMACK labels. To reproduce the issue in interhost communications set opposite labels in /smack/ipv6host on both hosts. Ipv6 intrahost communications do not require tricking, because SMACK labels are conveyed via secmark netfilter marking.
So, currently smack_sock_graft() is not useful, but harmful, therefore, I have removed it.
This fixes the issue for {tcp,dccp}/{ipv4,ipv6}, but not sctp/{ipv4,ipv6}.
Although this change is necessary for sctp+smack to function correctly, it is not sufficient because: sctp/ipv4 does not call security_sk_clone() and sctp/ipv6 ignores SMACK completely.
These are separate issues, belong to other subsystem, and should be addressed separately.
[1] 2008-02-04, Fixes: e114e473771c ("Smack: Simplified Mandatory Access Control Kernel")
[2] Michael Lontke, 2022-08-31, SMACK LSM checks wrong object label during ingress network traffic Link: https://lore.kernel.org/linux-security-module/6324997ce4fc092c5020a4add07525...
[3] 2022-08-31, michael.lontke, commit 4ca165fc6c49 ("SMACK: Add sk_clone_security LSM hook")
Signed-off-by: Konstantin Andreev andreev@swemel.ru Signed-off-by: Casey Schaufler casey@schaufler-ca.com Signed-off-by: Sasha Levin sashal@kernel.org --- security/smack/smack_lsm.c | 24 ------------------------ 1 file changed, 24 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 85f3d4337ca19..e68c982e499eb 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4350,29 +4350,6 @@ static int smack_socket_getpeersec_dgram(struct socket *sock, return 0; }
-/** - * smack_sock_graft - Initialize a newly created socket with an existing sock - * @sk: child sock - * @parent: parent socket - * - * Set the smk_{in,out} state of an existing sock based on the process that - * is creating the new socket. - */ -static void smack_sock_graft(struct sock *sk, struct socket *parent) -{ - struct socket_smack *ssp; - struct smack_known *skp = smk_of_current(); - - if (sk == NULL || - (sk->sk_family != PF_INET && sk->sk_family != PF_INET6)) - return; - - ssp = smack_sock(sk); - ssp->smk_in = skp; - ssp->smk_out = skp; - /* cssp->smk_packet is already set in smack_inet_csk_clone() */ -} - /** * smack_inet_conn_request - Smack access check on connect * @sk: socket involved @@ -5187,7 +5164,6 @@ static struct security_hook_list smack_hooks[] __ro_after_init = { LSM_HOOK_INIT(sk_free_security, smack_sk_free_security), #endif LSM_HOOK_INIT(sk_clone_security, smack_sk_clone_security), - LSM_HOOK_INIT(sock_graft, smack_sock_graft), LSM_HOOK_INIT(inet_conn_request, smack_inet_conn_request), LSM_HOOK_INIT(inet_csk_clone, smack_inet_csk_clone),