| Message ID | 20200523195130.409607-1-jlebon@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: allow reading labels before policy is loaded | expand |
On Sat, May 23, 2020 at 9:56 PM Jonathan Lebon <jlebon@redhat.com> wrote: > This patch does for `getxattr` what 3e3e24b4204 did for `setxattr`: it > allows querying the current SELinux label on disk before the policy is > loaded. > > One of the motivations described in that commit message also drives this > patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be > able to move the root filesystem for example from xfs to ext4, on first > boot, at initrd time.[1] > > Because such an operation works at the filesystem level, we need to be > able to read the SELinux labels first from the original root, and apply > them to the files of the new root. Commit 3e3e24b4204 enabled the second > part of this process; this patch enables the first part. > > [1] https://github.com/coreos/fedora-coreos-tracker/issues/94 > > Signed-off-by: Jonathan Lebon <jlebon@redhat.com> > --- > security/selinux/hooks.c | 55 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 50 insertions(+), 5 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 0b4e32161b7..3bbb9966697 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c <snip> > @@ -3333,10 +3354,34 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void > int error; > char *context = NULL; > struct inode_security_struct *isec; > + struct superblock_security_struct *sbsec; > > if (strcmp(name, XATTR_SELINUX_SUFFIX)) > return -EOPNOTSUPP; I might be missing something, but couldn't you achieve the same by simply adding something like this in here: if (!selinux_initialized(&selinux_state)) return -EOPNOTSUPP; (Or by adding it to the condition above.) Then you should hit this condition here and be all set: https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 > > + isec = inode_security(inode); > + sbsec = inode->i_sb->s_security; > + > + /* Just return the raw context if the policy isn't even loaded since we > + * have no way to validate it anyway. This is symmetrical with allowing > + * setxattr without a policy. */ > + if (!selinux_state.initialized) { > + /* See similar code in inode_doinit_with_dentry; for xattrs, > + * some filesystems really want a connected inode. If we don't > + * find one, just let fallback in case it corresponds to one of > + * the default sids. */ > + struct dentry *dentry = d_find_alias(inode); > + if (!dentry) > + dentry = d_find_any_alias(inode); > + > + if (dentry) { > + error = get_inode_raw_xattr_context(inode, dentry, > + &context, &size); > + dput(dentry); > + goto out; > + } > + } > + > /* > * If the caller has CAP_MAC_ADMIN, then get the raw context > * value even if it is not defined by current policy; otherwise, > @@ -3346,7 +3391,6 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void > * and lack of permission just means that we fall back to the > * in-core context value, not a denial. > */ > - isec = inode_security(inode); > if (has_cap_mac_admin(false)) > error = security_sid_to_context_force(&selinux_state, > isec->sid, &context, > @@ -3354,6 +3398,7 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void > else > error = security_sid_to_context(&selinux_state, isec->sid, > &context, &size); > +out: > if (error) > return error; > error = size; > -- > 2.25.4 >
On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > I might be missing something, but couldn't you achieve the same by > simply adding something like this in here: > > if (!selinux_initialized(&selinux_state)) > return -EOPNOTSUPP; > > (Or by adding it to the condition above.) > > Then you should hit this condition here and be all set: > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 Hi Ondrej, Yes, that looks promising. Two questions with that approach: 1. Is there a concern here with transiently returning -EOPNOTSUPP even if the SELinux LSM does technically support the inode_getsecurity hook? I'm thinking of potential corner-cases down the road where somehow this knowledge is cached. 2. The selinux_inode_getsecurity hook today does somewhat handle the uninitialized case. It ends up here: https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322. Specifically, it has support for initial SIDs. The patch I wrote purposely tries to allow falling back to that logic. Is there a concern with short-circuiting this logic even if the inode SID somehow isn't SECINITSID_UNLABELED?
On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > I might be missing something, but couldn't you achieve the same by > simply adding something like this in here: > > if (!selinux_initialized(&selinux_state)) > return -EOPNOTSUPP; > > (Or by adding it to the condition above.) > > Then you should hit this condition here and be all set: > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 OK, I tried this now and it does work well at least for our use case. It's clearly a much simpler patch as well, which is nice. So assuming my questions from the previous email don't raise any concerns, I'm happy to switch to that instead. One thing worth mentioning is that both patches change the behaviour of `getxattr` for inodes on SE_SBGENFS superblocks. For example, before, `getxattr("/proc", "security.selinux")` would return `unlabeled_t`. Now it gets EOPNOTSUPP instead. Anyway, I think this is fine overall and consistent with the behaviour change proposed, but just wanted to flag it.
On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@redhat.com> wrote: > On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > I might be missing something, but couldn't you achieve the same by > > simply adding something like this in here: > > > > if (!selinux_initialized(&selinux_state)) > > return -EOPNOTSUPP; > > > > (Or by adding it to the condition above.) > > > > Then you should hit this condition here and be all set: > > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 > > Hi Ondrej, > > Yes, that looks promising. Two questions with that approach: > > 1. Is there a concern here with transiently returning -EOPNOTSUPP even > if the SELinux LSM does technically support the inode_getsecurity > hook? I'm thinking of potential corner-cases down the road where > somehow this knowledge is cached. I would hope not. I don't think it's likely this would be cached, since it would require a guarantee from all LSMs that they won't flip from -EOPNOTSUPP to something else... That would be error-prone IMHO. > > 2. The selinux_inode_getsecurity hook today does somewhat handle the > uninitialized case. It ends up here: > > https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322. > > Specifically, it has support for initial SIDs. The patch I wrote > purposely tries to allow falling back to that logic. Is there a > concern with short-circuiting this logic even if the inode SID somehow > isn't SECINITSID_UNLABELED? Oh, right, so that's what I missed :) I'll have to defer to Stephen on whether this is a concern... Obviously we lose the previous behavior of returning the initial SID strings via getxattr(), but I'm not sure if that's significant. Since those strings obviously aren't real contexts, it seems they only serve an informational purpose. Anyway, I looked at the original patch again and it generally looks sane. I don't like the fact that we need to call back to __vfs_getxattr() in yet another place, but it makes sense since security_inode_getsecurity() is basically overriding it. So I leave it on Stephen or Paul to decide which is better.
On Wed, May 27, 2020 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Tue, May 26, 2020 at 9:12 PM Jonathan Lebon <jlebon@redhat.com> wrote: > > On Mon, May 25, 2020 at 1:14 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > I might be missing something, but couldn't you achieve the same by > > > simply adding something like this in here: > > > > > > if (!selinux_initialized(&selinux_state)) > > > return -EOPNOTSUPP; > > > > > > (Or by adding it to the condition above.) > > > > > > Then you should hit this condition here and be all set: > > > https://elixir.bootlin.com/linux/v5.7-rc7/source/fs/xattr.c#L337 > > > > Hi Ondrej, > > > > Yes, that looks promising. Two questions with that approach: > > > > 1. Is there a concern here with transiently returning -EOPNOTSUPP even > > if the SELinux LSM does technically support the inode_getsecurity > > hook? I'm thinking of potential corner-cases down the road where > > somehow this knowledge is cached. > > I would hope not. I don't think it's likely this would be cached, > since it would require a guarantee from all LSMs that they won't flip > from -EOPNOTSUPP to something else... That would be error-prone IMHO. > > > > > 2. The selinux_inode_getsecurity hook today does somewhat handle the > > uninitialized case. It ends up here: > > > > https://elixir.bootlin.com/linux/v5.7-rc7/source/security/selinux/ss/services.c#L1322. > > > > Specifically, it has support for initial SIDs. The patch I wrote > > purposely tries to allow falling back to that logic. Is there a > > concern with short-circuiting this logic even if the inode SID somehow > > isn't SECINITSID_UNLABELED? > > Oh, right, so that's what I missed :) I'll have to defer to Stephen on > whether this is a concern... Obviously we lose the previous behavior > of returning the initial SID strings via getxattr(), but I'm not sure > if that's significant. Since those strings obviously aren't real > contexts, it seems they only serve an informational purpose. > > Anyway, I looked at the original patch again and it generally looks > sane. I don't like the fact that we need to call back to > __vfs_getxattr() in yet another place, but it makes sense since > security_inode_getsecurity() is basically overriding it. So I leave it > on Stephen or Paul to decide which is better. I think Ondrej's suggested approach is better. I don't think it is a concern.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 0b4e32161b7..3bbb9966697 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -1317,8 +1317,11 @@ static int selinux_genfs_get_sid(struct dentry *dentry, return rc; } -static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry, - u32 def_sid, u32 *sid) +/* Retrieves the raw context from the fs xattr. Returns 0 on success. */ +static int get_inode_raw_xattr_context(struct inode *inode, + struct dentry *dentry, + char **out_context, + u32 *out_len) { #define INITCONTEXTLEN 255 char *context; @@ -1354,13 +1357,31 @@ static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry, if (rc != -ENODATA) { pr_warn("SELinux: %s: getxattr returned %d for dev=%s ino=%ld\n", __func__, -rc, inode->i_sb->s_id, inode->i_ino); - return rc; } + return rc; + } + + *out_len = rc; + *out_context = context; + return 0; +} + +static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry, + u32 def_sid, u32 *sid) +{ + char *context; + u32 size; + int rc; + + rc = get_inode_raw_xattr_context(inode, dentry, &context, &size); + if (rc < 0) { + if (rc != -ENODATA) + return rc; *sid = def_sid; return 0; } - rc = security_context_to_sid_default(&selinux_state, context, rc, sid, + rc = security_context_to_sid_default(&selinux_state, context, size, sid, def_sid, GFP_NOFS); if (rc) { char *dev = inode->i_sb->s_id; @@ -3333,10 +3354,34 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void int error; char *context = NULL; struct inode_security_struct *isec; + struct superblock_security_struct *sbsec; if (strcmp(name, XATTR_SELINUX_SUFFIX)) return -EOPNOTSUPP; + isec = inode_security(inode); + sbsec = inode->i_sb->s_security; + + /* Just return the raw context if the policy isn't even loaded since we + * have no way to validate it anyway. This is symmetrical with allowing + * setxattr without a policy. */ + if (!selinux_state.initialized) { + /* See similar code in inode_doinit_with_dentry; for xattrs, + * some filesystems really want a connected inode. If we don't + * find one, just let fallback in case it corresponds to one of + * the default sids. */ + struct dentry *dentry = d_find_alias(inode); + if (!dentry) + dentry = d_find_any_alias(inode); + + if (dentry) { + error = get_inode_raw_xattr_context(inode, dentry, + &context, &size); + dput(dentry); + goto out; + } + } + /* * If the caller has CAP_MAC_ADMIN, then get the raw context * value even if it is not defined by current policy; otherwise, @@ -3346,7 +3391,6 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void * and lack of permission just means that we fall back to the * in-core context value, not a denial. */ - isec = inode_security(inode); if (has_cap_mac_admin(false)) error = security_sid_to_context_force(&selinux_state, isec->sid, &context, @@ -3354,6 +3398,7 @@ static int selinux_inode_getsecurity(struct inode *inode, const char *name, void else error = security_sid_to_context(&selinux_state, isec->sid, &context, &size); +out: if (error) return error; error = size;
This patch does for `getxattr` what 3e3e24b4204 did for `setxattr`: it allows querying the current SELinux label on disk before the policy is loaded. One of the motivations described in that commit message also drives this patch: for Fedora CoreOS (and eventually RHEL CoreOS), we want to be able to move the root filesystem for example from xfs to ext4, on first boot, at initrd time.[1] Because such an operation works at the filesystem level, we need to be able to read the SELinux labels first from the original root, and apply them to the files of the new root. Commit 3e3e24b4204 enabled the second part of this process; this patch enables the first part. [1] https://github.com/coreos/fedora-coreos-tracker/issues/94 Signed-off-by: Jonathan Lebon <jlebon@redhat.com> --- security/selinux/hooks.c | 55 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 50 insertions(+), 5 deletions(-)