selinux: fix the default socket labeling in sock_graft()
PATCH 1/1 selinux: fix the default socket labeling in sock_graft()
The sock_graft() hook has special handling for AF_INET, AF_INET, and AF_UNIX sockets as those address families have special hooks which label the sock before it is attached its associated socket. Unfortunately, the sock_graft() hook was missing a default approach to labeling sockets which meant that any other address family which made use of connections or the accept() syscall would find the returned socket to be in an "unlabeled" state. This was recently demonstrated by the kcrypto/AF_ALG subsystem and the newly released cryptsetup package (cryptsetup v1.6.5 and later).
This patch preserves the special handling in selinux_sock_graft(), but adds a default behavior - setting the sock's label equal to the associated socket - which resolves the problem with AF_ALG and presumably any other address family which makes use of accept().
--- security/selinux/hooks.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
security/selinux/hooks.c +10 -2
@@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)struct inode_security_struct *isec =SOCK_INODE (parent)->i_security;struct sk_security_struct *sksec = sk->sk_security; -if (sk->sk_family ==PF_INET || sk->sk_family ==PF_INET6 || - sk->sk_family ==PF_UNIX ) +switch (sk->sk_family) { +case PF_INET : +case PF_INET6 : +case PF_UNIX : isec->sid = sksec->sid; +break ; +default : +/* by default there is no special labeling mechanism for the +* sock label so inherit the label from the parent socket */ + sksec->sid = isec->sid; + } sksec->sclass = isec->sclass; }
Quoted 33 lines · Paul Moore
The
sock_graft()hook has special handling forAF_INET,AF_INET, andAF_UNIXsockets as those address families have special hooks which label the sock before it is attached its associated socket. Unfortunately, thesock_graft()hook was missing a default approach to labeling sockets which meant that any other address family which made use of connections or theaccept()syscall would find the returned socket to be in an "unlabeled" state. This was recently demonstrated by the kcrypto/AF_ALG subsystem and the newly released cryptsetup package (cryptsetup v1.6.5 and later).This patch preserves the special handling in
selinux_sock_graft(), but adds a default behavior - setting the sock's label equal to the associated socket - which resolves the problem withAF_ALGand presumably any other address family which makes use ofaccept().Signed-off-by Paul Moore <[email protected]>Tested-by Milan Broz <[email protected]>--- security/selinux/hooks.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)security/selinux/hooks.c +1
-2@@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)struct inode_security_struct *isec =SOCK_INODE (parent)->i_security;struct sk_security_struct *sksec = sk->sk_security; -if (sk->sk_family ==PF_INET || sk->sk_family ==PF_INET6 || - sk->sk_family ==PF_UNIX ) +switch (sk->sk_family) {
security/selinux/hooks.c static void selinux_sock_graft(struct sock *sk, struct socket *parent) · line 4512
Quoted 10 lines
security/selinux/hooks.c +9
-0+case PF_INET : +case PF_INET6 : +case PF_UNIX : isec->sid = sksec->sid; +break ; +default : +/* by default there is no special labeling mechanism for the +* sock label so inherit the label from the parent socket */ + sksec->sid = isec->sid; + }
Wait...why would we assign isec->sid from sksec->sid in the former case but the reverse here? Shouldn't we be setting isec->sid in all cases? The hook documentation in include/linux/security.h unfortunately does not describe the actual abstract behavior but rather describes the implementation in the inet case.
Quoted 10 lines
security/selinux/hooks.c +0
-0sksec->sclass = isec->sclass; }_______________________________________________
Selinux mailing list
[email protected]
To unsubscribe, send email to [email protected].
To get help, send an email containing "help" to [email protected].
Quoted 42 lines · Stephen Smalley
Quoted 41 lines · Paul Moore
The
sock_graft()hook has special handling forAF_INET,AF_INET, andAF_UNIXsockets as those address families have special hooks which label the sock before it is attached its associated socket. Unfortunately, thesock_graft()hook was missing a default approach to labeling sockets which meant that any other address family which made use of connections or theaccept()syscall would find the returned socket to be in an "unlabeled" state. This was recently demonstrated by the kcrypto/AF_ALG subsystem and the newly released cryptsetup package (cryptsetup v1.6.5 and later).This patch preserves the special handling in
selinux_sock_graft(), but adds a default behavior - setting the sock's label equal to the associated socket - which resolves the problem withAF_ALGand presumably any other address family which makes use ofaccept().Signed-off-by Paul Moore <[email protected]>Tested-by Milan Broz <[email protected]>--- security/selinux/hooks.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)security/selinux/hooks.c +8
-2@@ -4499,9 +4499,17 @@ static void selinux_sock_graft(struct sock *sk, struct socket *parent)struct inode_security_struct *isec =SOCK_INODE (parent)->i_security;struct sk_security_struct *sksec = sk->sk_security; -if (sk->sk_family ==PF_INET || sk->sk_family ==PF_INET6 || - sk->sk_family ==PF_UNIX ) +switch (sk->sk_family) { +case PF_INET : +case PF_INET6 : +case PF_UNIX : isec->sid = sksec->sid; +break ; +default : +/* by default there is no special labeling mechanism for the +* sock label so inherit the label from the parent socket */
Quoted 8 lines
security/selinux/hooks.c static void selinux_sock_graft(struct sock *sk, struct socket *parent) · line 4512
Quoted 2 lines
security/selinux/hooks.c +2
-0+ sksec->sid = isec->sid; + }Wait...why would we assign
isec->sidfromsksec->sidin the former case but the reverse here? Shouldn't we be settingisec->sidin all cases? The hook documentation ininclude/linux/security.hunfortunately does not describe the actual abstract behavior but rather describes the implementation in the inet case.
Ok, I think I understand this now: for inet and unix, sksec->sid is set in other hooks upon connection establishment based on the peer label - primarily for multi-level servers - and we are propagating it upward to the parent socket inode. For others, sksec->sid is not set anywhere except initialized to unlabeled upon sock creation and so you are just pushing the parent socket inode label down to the sock in your patch.
It seems a bit fragile though and certainly doesn't align with the sock_graft hook documentation anymore. Wondering if we should assert that sksec->sid is not SECINITSID_UNLABELED in the inet/inet6/unix case (i.e. that sksec->sid has been set prior to copying it to isec->sid) and that sksec->sid is SECINITSID_UNLABELED (i.e. that it has not already been set by the protocol implementation) in the default case. We need to update the hook documentation too.
Quoted 6 lines · Stephen Smalley
Ok, I think I understand this now: for inet and unix,
sksec->sidis set in other hooks upon connection establishment based on the peer label - primarily for multi-level servers - and we are propagating it upward to the parent socket inode. For others,sksec->sidis not set anywhere except initialized to unlabeled upon sock creation and so you are just pushing the parent socket inode label down to the sock in your patch.
Yep.
Quoted 7 lines
It seems a bit fragile though and certainly doesn't align with the
sock_grafthook documentation anymore. Wondering if we should assert thatsksec->sidis notSECINITSID_UNLABELEDin the inet/inet6/unix case (i.e. thatsksec->sidhas been set prior to copying it toisec->sid) and thatsksec->sidisSECINITSID_UNLABELED(i.e. that it has not already been set by the protocol implementation) in the default case. We need to update the hook documentation too.
Since we can't return an error code, we would be stuck with a BUG_ON() which is okay, but doesn't help in the situation where the kernel has compiled out the BUG/BUG_ON macros. Regardless, there probably is some value in adding a BUG_ON(), if for no other reason than documentation.
I'll see about correcting the comment in, thanks for catching that.
Signature · 2 lines
paul moore security and virtualization @ redhat
Quoted 4 lines · Stephen Smalley
It seems a bit fragile though and certainly doesn't align with the
sock_grafthook documentation anymore. Wondering if we should assert thatsksec->sidis notSECINITSID_UNLABELEDin the inet/inet6/unix case (i.e. thatsksec->sidhas been set prior to copying it toisec->sid) ...
Now that I'm changing the code I'm wondering if we could ever arrive at a situation where sksec->sid would legitimately end up as SECINITSID_UNLABELED in selinux_sock_graft(). In the normal case of no labeled networking, the sksec->sid label should end up as SECSID_NULL so I'm not to worried about that, but if would you get the SECINITSID_UNLABELED SID if you fed "...:unlabeled_t:..." into security_secctx_to_secid()? I'm pretty sure the answer is "no", but I haven't looked at that code in a while.
Signature · 2 lines
paul moore security and virtualization @ redhat
Quoted 6 lines · Paul Moore
Quoted 4 lines · Stephen Smalley
It seems a bit fragile though and certainly doesn't align with the
sock_grafthook documentation anymore. Wondering if we should assert thatsksec->sidis notSECINITSID_UNLABELEDin the inet/inet6/unix case (i.e. thatsksec->sidhas been set prior to copying it toisec->sid) ...
Quoted 7 lines
Now that I'm changing the code I'm wondering if we could ever arrive at a situation where
sksec->sidwould legitimately end up asSECINITSID_UNLABELEDinselinux_sock_graft(). In the normal case of no labeled networking, thesksec->sidlabel should end up asSECSID_NULLso I'm not to worried about that, but if would you get theSECINITSID_UNLABELEDSID if you fed "...:unlabeled_t:..." intosecurity_secctx_to_secid()? I'm pretty sure the answer is "no", but I haven't looked at that code in a while.
Yes, you can get back an initial SID from security_secctx_to_secid() if the context string matches. In what scenario would sksec->sid be set to the result of security_secctx_to_secid() on a context string though, and from where would that context string originate? At most we only copy the MLS attributes of the peer label, right?
Quoted 16 lines · Stephen Smalley
Quoted 6 lines · Paul Moore
Quoted 4 lines · Stephen Smalley
It seems a bit fragile though and certainly doesn't align with the
sock_grafthook documentation anymore. Wondering if we should assert thatsksec->sidis notSECINITSID_UNLABELEDin the inet/inet6/unix case (i.e. thatsksec->sidhas been set prior to copying it toisec->sid) ...Quoted 8 lines
Now that I'm changing the code I'm wondering if we could ever arrive at a situation where
sksec->sidwould legitimately end up asSECINITSID_UNLABELEDinselinux_sock_graft(). In the normal case of no labeled networking, thesksec->sidlabel should end up asSECSID_NULLso I'm not to worried about that, but if would you get theSECINITSID_UNLABELEDSID if you fed "...:unlabeled_t:..." intosecurity_secctx_to_secid()? I'm pretty sure the answer is "no", but I haven't looked at that code in a while.
Quoted 5 lines
Yes, you can get back an initial SID from
security_secctx_to_secid()if the context string matches. In what scenario wouldsksec->sidbe set to the result ofsecurity_secctx_to_secid()on a context string though, and from where would that context string originate? At most we only copy the MLS attributes of the peer label, right?
Yes, my apologies, I wasn't think through the full process to the MLS attribute copy.
Regardless, I suppose I'm still a bit leery of adding the assertion in the AF_INET/AF_INET6/AF_UNIX case, especially since that code has been there for years now without problem (I know, famous last words). I don't have a problem adding an assertion in the other, default case.
Signature · 2 lines
paul moore security and virtualization @ redhat