| Message ID | 20200124184221.322248-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: allow kernfs symlinks to inherit parent directory context | expand |
On 1/24/20 1:42 PM, Christian Göttsche wrote: > Currently symlinks on kernel filesystems, like sysfs, are labeled on > creation with the parent fs root sid. > > Allow symlinks to inherit the parent directory context, so fine-grained > kernfs labeling can be applied to symlinks too and checking contexts > doesn't complain about them. > > For backward-compatibility this behavior is contained in a new policy > capability: kernfs_sovereign_symlinks > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > security/selinux/hooks.c | 5 ++++- > security/selinux/include/security.h | 8 ++++++++ > security/selinux/ss/services.c | 3 ++- > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index d9e8b2131..1303bc8c4 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -1475,7 +1475,10 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent > /* Default to the fs superblock SID. */ > sid = sbsec->sid; > > - if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { > + if (((sbsec->flags & SE_SBGENFS) && > + (!S_ISLNK(inode->i_mode))) || > + (selinux_policycap_kernfs_sovereign_symlinks() && Not fond of the name. 1) kernfs is a kernel implementation detail, shouldn't be exposed to policy; genfs is the policy construct 2) sovereign doesn't seem to fit the meaning of this capability; seclabel would be more appropriate. > + (sbsec->flags & SE_SBGENFS_XATTR))) { Why limit this to SE_SBGENFS_XATTR filesystems? Why not just make the test: if ((sbsec->flags & SE_SBGENFS) && (!S_ISLNK(inode->i_mode) || selinux_policycap_genfs_symlinkseclabel())) or similar. > /* We must have a dentry to determine the label on > * procfs inodes */ > if (opt_dentry) { > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > index a39f9565d..cc8217848 100644 > --- a/security/selinux/include/security.h > +++ b/security/selinux/include/security.h > @@ -79,6 +79,7 @@ enum { > POLICYDB_CAPABILITY_ALWAYSNETWORK, > POLICYDB_CAPABILITY_CGROUPSECLABEL, > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION, > + POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS, > __POLICYDB_CAPABILITY_MAX > }; > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) > @@ -209,6 +210,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void) > return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION]; > } > > +static inline bool selinux_policycap_kernfs_sovereign_symlinks(void) > +{ > + struct selinux_state *state = &selinux_state; > + > + return state->policycap[POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS]; > +} > + > int security_mls_enabled(struct selinux_state *state); > int security_load_policy(struct selinux_state *state, > void *data, size_t len); > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 216ce602a..b70380947 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -73,7 +73,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { > "extended_socket_class", > "always_check_network", > "cgroup_seclabel", > - "nnp_nosuid_transition" > + "nnp_nosuid_transition", > + "kernfs_sovereign_symlinks" > }; > > static struct selinux_ss selinux_ss; >
Am Fr., 24. Jan. 2020 um 19:53 Uhr schrieb Stephen Smalley <sds@tycho.nsa.gov>: > > On 1/24/20 1:42 PM, Christian Göttsche wrote: > > Currently symlinks on kernel filesystems, like sysfs, are labeled on > > creation with the parent fs root sid. > > > > Allow symlinks to inherit the parent directory context, so fine-grained > > kernfs labeling can be applied to symlinks too and checking contexts > > doesn't complain about them. > > > > For backward-compatibility this behavior is contained in a new policy > > capability: kernfs_sovereign_symlinks > > > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > security/selinux/hooks.c | 5 ++++- > > security/selinux/include/security.h | 8 ++++++++ > > security/selinux/ss/services.c | 3 ++- > > 3 files changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index d9e8b2131..1303bc8c4 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -1475,7 +1475,10 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent > > /* Default to the fs superblock SID. */ > > sid = sbsec->sid; > > > > - if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { > > + if (((sbsec->flags & SE_SBGENFS) && > > + (!S_ISLNK(inode->i_mode))) || > > + (selinux_policycap_kernfs_sovereign_symlinks() && > > Not fond of the name. 1) kernfs is a kernel implementation detail, > shouldn't be exposed to policy; genfs is the policy construct 2) > sovereign doesn't seem to fit the meaning of this capability; seclabel > would be more appropriate. Something like genfs_seclabel_symlinks? > > + (sbsec->flags & SE_SBGENFS_XATTR))) { > > Why limit this to SE_SBGENFS_XATTR filesystems? Why not just make the test: > if ((sbsec->flags & SE_SBGENFS) && (!S_ISLNK(inode->i_mode) || > selinux_policycap_genfs_symlinkseclabel())) > or similar. I somehow thought that this functionality is limited to filesystems with SE_SBGENFS_XATTR; so I can expand the check to SE_SBGENFS. > > /* We must have a dentry to determine the label on > > * procfs inodes */ > > if (opt_dentry) { > > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h > > index a39f9565d..cc8217848 100644 > > --- a/security/selinux/include/security.h > > +++ b/security/selinux/include/security.h > > @@ -79,6 +79,7 @@ enum { > > POLICYDB_CAPABILITY_ALWAYSNETWORK, > > POLICYDB_CAPABILITY_CGROUPSECLABEL, > > POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION, > > + POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS, > > __POLICYDB_CAPABILITY_MAX > > }; > > #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) > > @@ -209,6 +210,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void) > > return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION]; > > } > > > > +static inline bool selinux_policycap_kernfs_sovereign_symlinks(void) > > +{ > > + struct selinux_state *state = &selinux_state; > > + > > + return state->policycap[POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS]; > > +} > > + > > int security_mls_enabled(struct selinux_state *state); > > int security_load_policy(struct selinux_state *state, > > void *data, size_t len); > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 216ce602a..b70380947 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -73,7 +73,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { > > "extended_socket_class", > > "always_check_network", > > "cgroup_seclabel", > > - "nnp_nosuid_transition" > > + "nnp_nosuid_transition", > > + "kernfs_sovereign_symlinks" > > }; > > > > static struct selinux_ss selinux_ss; > > >
On 1/24/20 2:08 PM, Christian Göttsche wrote: > Am Fr., 24. Jan. 2020 um 19:53 Uhr schrieb Stephen Smalley <sds@tycho.nsa.gov>: >> >> On 1/24/20 1:42 PM, Christian Göttsche wrote: >>> Currently symlinks on kernel filesystems, like sysfs, are labeled on >>> creation with the parent fs root sid. >>> >>> Allow symlinks to inherit the parent directory context, so fine-grained >>> kernfs labeling can be applied to symlinks too and checking contexts >>> doesn't complain about them. >>> >>> For backward-compatibility this behavior is contained in a new policy >>> capability: kernfs_sovereign_symlinks >>> >>> Signed-off-by: Christian Göttsche <cgzones@googlemail.com> >>> --- >> Not fond of the name. 1) kernfs is a kernel implementation detail, >> shouldn't be exposed to policy; genfs is the policy construct 2) >> sovereign doesn't seem to fit the meaning of this capability; seclabel >> would be more appropriate. > > Something like genfs_seclabel_symlinks? Works for me. > >>> + (sbsec->flags & SE_SBGENFS_XATTR))) { >> >> Why limit this to SE_SBGENFS_XATTR filesystems? Why not just make the test: >> if ((sbsec->flags & SE_SBGENFS) && (!S_ISLNK(inode->i_mode) || >> selinux_policycap_genfs_symlinkseclabel())) >> or similar. > > I somehow thought that this functionality is limited to filesystems > with SE_SBGENFS_XATTR; > so I can expand the check to SE_SBGENFS. I could be wrong but I don't see why it would need to be limited in that way.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d9e8b2131..1303bc8c4 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1475,7 +1475,10 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent /* Default to the fs superblock SID. */ sid = sbsec->sid; - if ((sbsec->flags & SE_SBGENFS) && !S_ISLNK(inode->i_mode)) { + if (((sbsec->flags & SE_SBGENFS) && + (!S_ISLNK(inode->i_mode))) || + (selinux_policycap_kernfs_sovereign_symlinks() && + (sbsec->flags & SE_SBGENFS_XATTR))) { /* We must have a dentry to determine the label on * procfs inodes */ if (opt_dentry) { diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index a39f9565d..cc8217848 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -79,6 +79,7 @@ enum { POLICYDB_CAPABILITY_ALWAYSNETWORK, POLICYDB_CAPABILITY_CGROUPSECLABEL, POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION, + POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS, __POLICYDB_CAPABILITY_MAX }; #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) @@ -209,6 +210,13 @@ static inline bool selinux_policycap_nnp_nosuid_transition(void) return state->policycap[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION]; } +static inline bool selinux_policycap_kernfs_sovereign_symlinks(void) +{ + struct selinux_state *state = &selinux_state; + + return state->policycap[POLICYDB_CAPABILITY_KERNFS_SOVEREIGN_SYMLINKS]; +} + int security_mls_enabled(struct selinux_state *state); int security_load_policy(struct selinux_state *state, void *data, size_t len); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 216ce602a..b70380947 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -73,7 +73,8 @@ const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { "extended_socket_class", "always_check_network", "cgroup_seclabel", - "nnp_nosuid_transition" + "nnp_nosuid_transition", + "kernfs_sovereign_symlinks" }; static struct selinux_ss selinux_ss;
Currently symlinks on kernel filesystems, like sysfs, are labeled on creation with the parent fs root sid. Allow symlinks to inherit the parent directory context, so fine-grained kernfs labeling can be applied to symlinks too and checking contexts doesn't complain about them. For backward-compatibility this behavior is contained in a new policy capability: kernfs_sovereign_symlinks Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/hooks.c | 5 ++++- security/selinux/include/security.h | 8 ++++++++ security/selinux/ss/services.c | 3 ++- 3 files changed, 14 insertions(+), 2 deletions(-)