| Message ID | 20230518204549.3139044-1-enlightened@chromium.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2] lsm: adds process attribute getter for Landlock | expand |
On 5/18/2023 1:45 PM, Shervin Oloumi wrote: > Adds a new getprocattr hook function to the Landlock LSM, which tracks > the landlocked state of the process. This is invoked when user-space > reads /proc/[pid]/attr/domain Please don't add a Landlock specific entry directly in the attr/ directory. Add it only to attr/landlock. Also be aware that the LSM maintainer (Paul Moore) wants to move away from the /proc/.../attr interfaces in favor of a new system call, which is in review. > to determine whether a given process is > sand-boxed using Landlock. When the target process is not sand-boxed, > the result is "none", otherwise the result is empty, as we still need to > decide what kind of domain information is best to provide in "domain". Unless it's too late, you should consider using a term other than "domain". Domain is used in many contexts already, and your use could be confused with any number of those. > > The hook function also performs an access check. The request is rejected > if the tracing process is the same as the target process, or if the > tracing process domain is not an ancestor to the target process domain. > > Adds a new directory for landlock under the process attribute > filesystem, and defines "domain" as a read-only process attribute entry > for landlock. > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > --- > fs/proc/base.c | 11 +++++++++++ > security/landlock/fs.c | 38 ++++++++++++++++++++++++++++++++++++++ > security/landlock/fs.h | 1 + > security/landlock/ptrace.c | 4 ++-- > security/landlock/ptrace.h | 3 +++ > 5 files changed, 55 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 9e479d7d202b..b257ea704666 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2851,6 +2851,13 @@ static const struct pid_entry apparmor_attr_dir_stuff[] = { > LSM_DIR_OPS(apparmor); > #endif > > +#ifdef CONFIG_SECURITY_LANDLOCK > +static const struct pid_entry landlock_attr_dir_stuff[] = { > + ATTR("landlock", "domain", 0444), > +}; > +LSM_DIR_OPS(landlock); > +#endif > + > static const struct pid_entry attr_dir_stuff[] = { > ATTR(NULL, "current", 0666), > ATTR(NULL, "prev", 0444), > @@ -2866,6 +2873,10 @@ static const struct pid_entry attr_dir_stuff[] = { > DIR("apparmor", 0555, > proc_apparmor_attr_dir_inode_ops, proc_apparmor_attr_dir_ops), > #endif > +#ifdef CONFIG_SECURITY_LANDLOCK > + DIR("landlock", 0555, > + proc_landlock_attr_dir_inode_ops, proc_landlock_attr_dir_ops), > +#endif > }; > > static int proc_attr_dir_readdir(struct file *file, struct dir_context *ctx) > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index adcea0fe7e68..2f8b0837a0fd 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -1280,6 +1280,42 @@ static int hook_file_truncate(struct file *const file) > return -EACCES; > } > > +/* process attribute interfaces */ > + > +/** > + * landlock_getprocattr - Landlock process attribute getter > + * @task: the object task > + * @name: the name of the attribute in /proc/.../attr > + * @value: where to put the result > + * > + * Performs access checks and writes any applicable results to value > + * > + * Returns the length of the result inside value or an error code > + */ > +static int landlock_getprocattr(struct task_struct *task, const char *name, > + char **value) > +{ > + char *val = ""; > + int slen; > + > + // If the tracing process is landlocked, ensure its domain is an > + // ancestor to the target process domain. Please read the kernel style documentation. "//" comments are not used in the kernel. > + if (landlocked(current)) > + if (current == task || !task_is_scoped(current, task)) > + return -EACCES; > + > + // The only supported attribute is "domain". > + if (strcmp(name, "domain") != 0) > + return -EINVAL; > + > + if (!landlocked(task)) > + val = "none"; > + > + slen = strlen(val); > + *value = val; > + return slen; > +} > + > static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), > > @@ -1302,6 +1338,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security), > LSM_HOOK_INIT(file_open, hook_file_open), > LSM_HOOK_INIT(file_truncate, hook_file_truncate), > + > + LSM_HOOK_INIT(getprocattr, landlock_getprocattr), > }; > > __init void landlock_add_fs_hooks(void) > diff --git a/security/landlock/fs.h b/security/landlock/fs.h > index 488e4813680a..64145e8b5537 100644 > --- a/security/landlock/fs.h > +++ b/security/landlock/fs.h > @@ -13,6 +13,7 @@ > #include <linux/init.h> > #include <linux/rcupdate.h> > > +#include "ptrace.h" > #include "ruleset.h" > #include "setup.h" > > diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c > index 4c5b9cd71286..de943f0f3899 100644 > --- a/security/landlock/ptrace.c > +++ b/security/landlock/ptrace.c > @@ -47,8 +47,8 @@ static bool domain_scope_le(const struct landlock_ruleset *const parent, > return false; > } > > -static bool task_is_scoped(const struct task_struct *const parent, > - const struct task_struct *const child) > +const bool task_is_scoped(const struct task_struct *const parent, > + const struct task_struct *const child) > { > bool is_scoped; > const struct landlock_ruleset *dom_parent, *dom_child; > diff --git a/security/landlock/ptrace.h b/security/landlock/ptrace.h > index 265b220ae3bf..c6eb08951fc1 100644 > --- a/security/landlock/ptrace.h > +++ b/security/landlock/ptrace.h > @@ -11,4 +11,7 @@ > > __init void landlock_add_ptrace_hooks(void); > > +const bool task_is_scoped(const struct task_struct *const parent, > + const struct task_struct *const child); > + > #endif /* _SECURITY_LANDLOCK_PTRACE_H */
On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 5/18/2023 1:45 PM, Shervin Oloumi wrote: > > Adds a new getprocattr hook function to the Landlock LSM, which tracks > > the landlocked state of the process. This is invoked when user-space > > reads /proc/[pid]/attr/domain > > Please don't add a Landlock specific entry directly in the attr/ > directory. Add it only to attr/landlock. > > Also be aware that the LSM maintainer (Paul Moore) wants to move > away from the /proc/.../attr interfaces in favor of a new system call, > which is in review. What Casey said above. There is still some uncertainty around timing, and if we're perfectly honest, acceptance of the new syscalls at the Linus level, but yes, I would very much like to see the LSM infrastructure move away from procfs and towards a syscall API. Part of the reasoning is that the current procfs API is ill-suited to handle the multiple, stacked LSMs and the other part being the complexity of procfs in a namespaced system. If the syscall API is ultimately rejected, we will need to revisit the idea of a procfs API, but even then I think we'll need to make some changes to the current approach. As I believe we are in the latter stages of review for the syscall API, perhaps you could take a look and ensure that the current proposed API works for what you are envisioning with Landlock?
On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 5/18/2023 1:45 PM, Shervin Oloumi wrote: > > > Adds a new getprocattr hook function to the Landlock LSM, which tracks > > > the landlocked state of the process. This is invoked when user-space > > > reads /proc/[pid]/attr/domain > > > > Please don't add a Landlock specific entry directly in the attr/ > > directory. Add it only to attr/landlock. > > > > Also be aware that the LSM maintainer (Paul Moore) wants to move > > away from the /proc/.../attr interfaces in favor of a new system call, > > which is in review. > > What Casey said above. > > There is still some uncertainty around timing, and if we're perfectly > honest, acceptance of the new syscalls at the Linus level, but yes, I > would very much like to see the LSM infrastructure move away from > procfs and towards a syscall API. Part of the reasoning is that the > current procfs API is ill-suited to handle the multiple, stacked LSMs > and the other part being the complexity of procfs in a namespaced > system. If the syscall API is ultimately rejected, we will need to > revisit the idea of a procfs API, but even then I think we'll need to > make some changes to the current approach. > > As I believe we are in the latter stages of review for the syscall > API, perhaps you could take a look and ensure that the current > proposed API works for what you are envisioning with Landlock? > Which review/patch to look for the proposed API ? I guess ChromeOS will need to backport to 5.10 when the proposal is accepted. Thanks -Jeff > -- > paul-moore.com
On 5/22/2023 11:13 PM, Jeff Xu wrote: > On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> wrote: >> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>> Adds a new getprocattr hook function to the Landlock LSM, which tracks >>>> the landlocked state of the process. This is invoked when user-space >>>> reads /proc/[pid]/attr/domain >>> Please don't add a Landlock specific entry directly in the attr/ >>> directory. Add it only to attr/landlock. >>> >>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>> away from the /proc/.../attr interfaces in favor of a new system call, >>> which is in review. >> What Casey said above. >> >> There is still some uncertainty around timing, and if we're perfectly >> honest, acceptance of the new syscalls at the Linus level, but yes, I >> would very much like to see the LSM infrastructure move away from >> procfs and towards a syscall API. Part of the reasoning is that the >> current procfs API is ill-suited to handle the multiple, stacked LSMs >> and the other part being the complexity of procfs in a namespaced >> system. If the syscall API is ultimately rejected, we will need to >> revisit the idea of a procfs API, but even then I think we'll need to >> make some changes to the current approach. >> >> As I believe we are in the latter stages of review for the syscall >> API, perhaps you could take a look and ensure that the current >> proposed API works for what you are envisioning with Landlock? >> > Which review/patch to look for the proposed API ? https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ > I guess ChromeOS will need to backport to 5.10 when the proposal is accepted. > > Thanks > -Jeff > > >> -- >> paul-moore.com
On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: > On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > > On 5/18/2023 1:45 PM, Shervin Oloumi wrote: > > > > Adds a new getprocattr hook function to the Landlock LSM, which tracks > > > > the landlocked state of the process. This is invoked when user-space > > > > reads /proc/[pid]/attr/domain > > > > > > Please don't add a Landlock specific entry directly in the attr/ > > > directory. Add it only to attr/landlock. > > > > > > Also be aware that the LSM maintainer (Paul Moore) wants to move > > > away from the /proc/.../attr interfaces in favor of a new system call, > > > which is in review. > > > > What Casey said above. > > > > There is still some uncertainty around timing, and if we're perfectly > > honest, acceptance of the new syscalls at the Linus level, but yes, I > > would very much like to see the LSM infrastructure move away from > > procfs and towards a syscall API. Part of the reasoning is that the > > current procfs API is ill-suited to handle the multiple, stacked LSMs > > and the other part being the complexity of procfs in a namespaced > > system. If the syscall API is ultimately rejected, we will need to > > revisit the idea of a procfs API, but even then I think we'll need to > > make some changes to the current approach. > > > > As I believe we are in the latter stages of review for the syscall > > API, perhaps you could take a look and ensure that the current > > proposed API works for what you are envisioning with Landlock? > > > Which review/patch to look for the proposed API ? See Casey's reply if you haven't already. You can also find the LSM list archived on lore.kernel.org; that is probably the best way to track LSM development if you don't want to subscribe to the list. * https://lore.kernel.org/linux-security-module > I guess ChromeOS will need to backport to 5.10 when the proposal is accepted. Maybe? Distro specific backports aren't generally on-topic for the upstream Linux mailing lists, especially large commercial distros with plenty of developers to take care of things like that.
On 23/05/2023 23:12, Paul Moore wrote: > On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: >> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> wrote: >>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>>> Adds a new getprocattr hook function to the Landlock LSM, which tracks >>>>> the landlocked state of the process. This is invoked when user-space >>>>> reads /proc/[pid]/attr/domain >>>> >>>> Please don't add a Landlock specific entry directly in the attr/ >>>> directory. Add it only to attr/landlock. >>>> >>>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>>> away from the /proc/.../attr interfaces in favor of a new system call, >>>> which is in review. >>> >>> What Casey said above. >>> >>> There is still some uncertainty around timing, and if we're perfectly >>> honest, acceptance of the new syscalls at the Linus level, but yes, I >>> would very much like to see the LSM infrastructure move away from >>> procfs and towards a syscall API. Part of the reasoning is that the >>> current procfs API is ill-suited to handle the multiple, stacked LSMs >>> and the other part being the complexity of procfs in a namespaced >>> system. If the syscall API is ultimately rejected, we will need to >>> revisit the idea of a procfs API, but even then I think we'll need to >>> make some changes to the current approach. >>> >>> As I believe we are in the latter stages of review for the syscall >>> API, perhaps you could take a look and ensure that the current >>> proposed API works for what you are envisioning with Landlock? I agree, and since the LSM syscalls are almost ready that should not change much the timing. In fact, extending these syscalls might be easier than tweaking the current procfs/attr API for Landlock specific requirements (e.g. scoped visibility). We should ensure that these syscalls would be a good fit to return file descriptors, but in the short term we only need to know if a process is landlocked or not, so a raw return value (0 or -errno) will be enough. Mentioning in the LSM syscalls patch series that they may deal with (and return) file descriptors could help API reviewers though. >>> >> Which review/patch to look for the proposed API ? > > See Casey's reply if you haven't already. You can also find the LSM > list archived on lore.kernel.org; that is probably the best way to > track LSM development if you don't want to subscribe to the list. > > * https://lore.kernel.org/linux-security-module > >> I guess ChromeOS will need to backport to 5.10 when the proposal is accepted. > > Maybe? Distro specific backports aren't generally on-topic for the > upstream Linux mailing lists, especially large commercial distros with > plenty of developers to take care of things like that. > Backporting the LSM syscall patch series will create conflicts but they should be manageable and the series should be quite standalone. You'll need to understand the changes to get a clean backport, so reviewing the current proposal is a good opportunity to be ready and to catch potential future issues.
On 24/05/2023 17:38, Mickaël Salaün wrote: > > On 23/05/2023 23:12, Paul Moore wrote: >> On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: >>> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> wrote: >>>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>>>> Adds a new getprocattr hook function to the Landlock LSM, which tracks >>>>>> the landlocked state of the process. This is invoked when user-space >>>>>> reads /proc/[pid]/attr/domain >>>>> >>>>> Please don't add a Landlock specific entry directly in the attr/ >>>>> directory. Add it only to attr/landlock. >>>>> >>>>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>>>> away from the /proc/.../attr interfaces in favor of a new system call, >>>>> which is in review. >>>> >>>> What Casey said above. >>>> >>>> There is still some uncertainty around timing, and if we're perfectly >>>> honest, acceptance of the new syscalls at the Linus level, but yes, I >>>> would very much like to see the LSM infrastructure move away from >>>> procfs and towards a syscall API. Part of the reasoning is that the >>>> current procfs API is ill-suited to handle the multiple, stacked LSMs >>>> and the other part being the complexity of procfs in a namespaced >>>> system. If the syscall API is ultimately rejected, we will need to >>>> revisit the idea of a procfs API, but even then I think we'll need to >>>> make some changes to the current approach. >>>> >>>> As I believe we are in the latter stages of review for the syscall >>>> API, perhaps you could take a look and ensure that the current >>>> proposed API works for what you are envisioning with Landlock? > > I agree, and since the LSM syscalls are almost ready that should not > change much the timing. In fact, extending these syscalls might be > easier than tweaking the current procfs/attr API for Landlock specific > requirements (e.g. scoped visibility). We should ensure that these > syscalls would be a good fit to return file descriptors, but in the > short term we only need to know if a process is landlocked or not, so a > raw return value (0 or -errno) will be enough. > > Mentioning in the LSM syscalls patch series that they may deal with (and > return) file descriptors could help API reviewers though. It should be kept in mind that the current LSM syscalls only deal with the calling task, whereas the goal of this Landlock patch series is to inspect other tasks. A new LSM syscall would need to be created to handle pidfd e.g., named lsm_get_proc_attr() or lsm_get_pid_attr(). I'm not sure if this should be a generic LSM syscall or a Landlock syscall though. I have plan to handle processes other than the caller (e.g. to restrict an existing process hierarchy), so thinking about a Landlock-specific syscall could make sense. To summarize, creating a new LSM syscall to deal with pidfd and to get LSM process "status/attr" looks OK. However, Landlock-specific syscalls to deal with Landlock specificities (e.g. ruleset or domain file descriptor) make more sense. Having one LSM-generic syscall to get minimal Landlock attributes (i.e. mainly to know if a process is sandboxed), and another Landlock-specific syscall to do more things (e.g. get the domain file descriptor, restrict a task) seems reasonable. The second one would overlap with the first one though. What do you think?
On 18/05/2023 23:26, Casey Schaufler wrote: > On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >> Adds a new getprocattr hook function to the Landlock LSM, which tracks >> the landlocked state of the process. This is invoked when user-space >> reads /proc/[pid]/attr/domain > > Please don't add a Landlock specific entry directly in the attr/ > directory. Add it only to attr/landlock. The commit message doesn't match the code, which creates attr/landlock.
On 18/05/2023 22:45, Shervin Oloumi wrote: > Adds a new getprocattr hook function to the Landlock LSM, which tracks > the landlocked state of the process. This is invoked when user-space > reads /proc/[pid]/attr/domain to determine whether a given process is > sand-boxed using Landlock. When the target process is not sand-boxed, > the result is "none", otherwise the result is empty, as we still need to > decide what kind of domain information is best to provide in "domain". > > The hook function also performs an access check. The request is rejected > if the tracing process is the same as the target process, or if the > tracing process domain is not an ancestor to the target process domain. > > Adds a new directory for landlock under the process attribute > filesystem, and defines "domain" as a read-only process attribute entry > for landlock. > > Signed-off-by: Shervin Oloumi <enlightened@chromium.org> > --- > fs/proc/base.c | 11 +++++++++++ > security/landlock/fs.c | 38 ++++++++++++++++++++++++++++++++++++++ > security/landlock/fs.h | 1 + > security/landlock/ptrace.c | 4 ++-- > security/landlock/ptrace.h | 3 +++ > 5 files changed, 55 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 9e479d7d202b..b257ea704666 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2851,6 +2851,13 @@ static const struct pid_entry apparmor_attr_dir_stuff[] = { > LSM_DIR_OPS(apparmor); > #endif > > +#ifdef CONFIG_SECURITY_LANDLOCK > +static const struct pid_entry landlock_attr_dir_stuff[] = { > + ATTR("landlock", "domain", 0444), > +}; > +LSM_DIR_OPS(landlock); > +#endif > + > static const struct pid_entry attr_dir_stuff[] = { > ATTR(NULL, "current", 0666), > ATTR(NULL, "prev", 0444), > @@ -2866,6 +2873,10 @@ static const struct pid_entry attr_dir_stuff[] = { > DIR("apparmor", 0555, > proc_apparmor_attr_dir_inode_ops, proc_apparmor_attr_dir_ops), > #endif > +#ifdef CONFIG_SECURITY_LANDLOCK > + DIR("landlock", 0555, > + proc_landlock_attr_dir_inode_ops, proc_landlock_attr_dir_ops), > +#endif > }; > > static int proc_attr_dir_readdir(struct file *file, struct dir_context *ctx) > diff --git a/security/landlock/fs.c b/security/landlock/fs.c > index adcea0fe7e68..2f8b0837a0fd 100644 > --- a/security/landlock/fs.c > +++ b/security/landlock/fs.c > @@ -1280,6 +1280,42 @@ static int hook_file_truncate(struct file *const file) > return -EACCES; > } > > +/* process attribute interfaces */ > + > +/** > + * landlock_getprocattr - Landlock process attribute getter > + * @task: the object task > + * @name: the name of the attribute in /proc/.../attr > + * @value: where to put the result > + * > + * Performs access checks and writes any applicable results to value > + * > + * Returns the length of the result inside value or an error code > + */ > +static int landlock_getprocattr(struct task_struct *task, const char *name, > + char **value) > +{ > + char *val = ""; > + int slen; > + > + // If the tracing process is landlocked, ensure its domain is an > + // ancestor to the target process domain. > + if (landlocked(current)) > + if (current == task || !task_is_scoped(current, task)) ptrace_may_access() checks more things than task_is_scoped(), but we should also make sure that that the current domain is taken into account (with a simple domain comparison). Tests should check these cases. > + return -EACCES; > + > + // The only supported attribute is "domain". > + if (strcmp(name, "domain") != 0) > + return -EINVAL; > + > + if (!landlocked(task)) > + val = "none"; I think the return values, for a dedicated syscall, would be "unknown", "unrestricted", "restricted". This could just be a returned enum. > + > + slen = strlen(val); > + *value = val; > + return slen; > +} This should be part of the ptrace.c file, which would also avoid exporting functions. > + > static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), > > @@ -1302,6 +1338,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security), > LSM_HOOK_INIT(file_open, hook_file_open), > LSM_HOOK_INIT(file_truncate, hook_file_truncate), > + > + LSM_HOOK_INIT(getprocattr, landlock_getprocattr), > }; > > __init void landlock_add_fs_hooks(void) > diff --git a/security/landlock/fs.h b/security/landlock/fs.h > index 488e4813680a..64145e8b5537 100644 > --- a/security/landlock/fs.h > +++ b/security/landlock/fs.h > @@ -13,6 +13,7 @@ > #include <linux/init.h> > #include <linux/rcupdate.h> > > +#include "ptrace.h" > #include "ruleset.h" > #include "setup.h" > > diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c > index 4c5b9cd71286..de943f0f3899 100644 > --- a/security/landlock/ptrace.c > +++ b/security/landlock/ptrace.c > @@ -47,8 +47,8 @@ static bool domain_scope_le(const struct landlock_ruleset *const parent, > return false; > } > > -static bool task_is_scoped(const struct task_struct *const parent, > - const struct task_struct *const child) > +const bool task_is_scoped(const struct task_struct *const parent, > + const struct task_struct *const child) > { > bool is_scoped; > const struct landlock_ruleset *dom_parent, *dom_child; > diff --git a/security/landlock/ptrace.h b/security/landlock/ptrace.h > index 265b220ae3bf..c6eb08951fc1 100644 > --- a/security/landlock/ptrace.h > +++ b/security/landlock/ptrace.h > @@ -11,4 +11,7 @@ > > __init void landlock_add_ptrace_hooks(void); > > +const bool task_is_scoped(const struct task_struct *const parent, > + const struct task_struct *const child); > + > #endif /* _SECURITY_LANDLOCK_PTRACE_H */
On 5/24/2023 9:02 AM, Mickaël Salaün wrote: > > On 24/05/2023 17:38, Mickaël Salaün wrote: >> >> On 23/05/2023 23:12, Paul Moore wrote: >>> On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: >>>> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> >>>> wrote: >>>>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler >>>>> <casey@schaufler-ca.com> wrote: >>>>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>>>>> Adds a new getprocattr hook function to the Landlock LSM, which >>>>>>> tracks >>>>>>> the landlocked state of the process. This is invoked when >>>>>>> user-space >>>>>>> reads /proc/[pid]/attr/domain >>>>>> >>>>>> Please don't add a Landlock specific entry directly in the attr/ >>>>>> directory. Add it only to attr/landlock. >>>>>> >>>>>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>>>>> away from the /proc/.../attr interfaces in favor of a new system >>>>>> call, >>>>>> which is in review. >>>>> >>>>> What Casey said above. >>>>> >>>>> There is still some uncertainty around timing, and if we're perfectly >>>>> honest, acceptance of the new syscalls at the Linus level, but yes, I >>>>> would very much like to see the LSM infrastructure move away from >>>>> procfs and towards a syscall API. Part of the reasoning is that the >>>>> current procfs API is ill-suited to handle the multiple, stacked LSMs >>>>> and the other part being the complexity of procfs in a namespaced >>>>> system. If the syscall API is ultimately rejected, we will need to >>>>> revisit the idea of a procfs API, but even then I think we'll need to >>>>> make some changes to the current approach. >>>>> >>>>> As I believe we are in the latter stages of review for the syscall >>>>> API, perhaps you could take a look and ensure that the current >>>>> proposed API works for what you are envisioning with Landlock? >> >> I agree, and since the LSM syscalls are almost ready that should not >> change much the timing. In fact, extending these syscalls might be >> easier than tweaking the current procfs/attr API for Landlock specific >> requirements (e.g. scoped visibility). We should ensure that these >> syscalls would be a good fit to return file descriptors, but in the >> short term we only need to know if a process is landlocked or not, so a >> raw return value (0 or -errno) will be enough. >> >> Mentioning in the LSM syscalls patch series that they may deal with (and >> return) file descriptors could help API reviewers though. > > It should be kept in mind that the current LSM syscalls only deal with > the calling task, whereas the goal of this Landlock patch series is to > inspect other tasks. A new LSM syscall would need to be created to > handle pidfd e.g., named lsm_get_proc_attr() or lsm_get_pid_attr(). I think it would be lsm_get_pid_attr(). Yes, it's the obvious next step. > > I'm not sure if this should be a generic LSM syscall or a Landlock > syscall though. I have plan to handle processes other than the caller > (e.g. to restrict an existing process hierarchy), so thinking about a > Landlock-specific syscall could make sense. > > To summarize, creating a new LSM syscall to deal with pidfd and to get > LSM process "status/attr" looks OK. However, Landlock-specific > syscalls to deal with Landlock specificities (e.g. ruleset or domain > file descriptor) make more sense. > > Having one LSM-generic syscall to get minimal Landlock attributes > (i.e. mainly to know if a process is sandboxed), and another > Landlock-specific syscall to do more things (e.g. get the domain file > descriptor, restrict a task) seems reasonable. The second one would > overlap with the first one though. What do you think? I find it difficult to think of a file descriptor as an attribute of a process. To my (somewhat unorthodox) thinking a file descriptor is a name for an object, not an attribute of the object. You can't access an object by its attributes, but you can by its name. An attribute is a description of the object. I'm perfectly happy with lsm_get_pid_attr() returning an attribute that is a file descriptor if it describes the process in some way, but not as a substitute for opening /proc/42.
> >> > >> As I believe we are in the latter stages of review for the syscall > >> API, perhaps you could take a look and ensure that the current > >> proposed API works for what you are envisioning with Landlock? > >> > > Which review/patch to look for the proposed API ? > > https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ > > How easy is it to add a customized LSM with new APIs? I'm asking because there are some hard-coded constant/macro, i.e. +#define LSM_ID_LANDLOCK 111 (Do IDs need to be sequential ?) + define LSM_CONFIG_COUNT Today, only security/Kconfig change is needed to add a new LSM, I think ?
On Thu, May 25, 2023 at 9:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 5/24/2023 9:02 AM, Mickaël Salaün wrote: > > > > On 24/05/2023 17:38, Mickaël Salaün wrote: > >> > >> On 23/05/2023 23:12, Paul Moore wrote: > >>> On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: > >>>> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> > >>>> wrote: > >>>>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler > >>>>> <casey@schaufler-ca.com> wrote: > >>>>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: > >>>>>>> Adds a new getprocattr hook function to the Landlock LSM, which > >>>>>>> tracks > >>>>>>> the landlocked state of the process. This is invoked when > >>>>>>> user-space > >>>>>>> reads /proc/[pid]/attr/domain > >>>>>> > >>>>>> Please don't add a Landlock specific entry directly in the attr/ > >>>>>> directory. Add it only to attr/landlock. > >>>>>> > >>>>>> Also be aware that the LSM maintainer (Paul Moore) wants to move > >>>>>> away from the /proc/.../attr interfaces in favor of a new system > >>>>>> call, > >>>>>> which is in review. > >>>>> > >>>>> What Casey said above. > >>>>> > >>>>> There is still some uncertainty around timing, and if we're perfectly > >>>>> honest, acceptance of the new syscalls at the Linus level, but yes, I > >>>>> would very much like to see the LSM infrastructure move away from > >>>>> procfs and towards a syscall API. Part of the reasoning is that the > >>>>> current procfs API is ill-suited to handle the multiple, stacked LSMs > >>>>> and the other part being the complexity of procfs in a namespaced > >>>>> system. If the syscall API is ultimately rejected, we will need to > >>>>> revisit the idea of a procfs API, but even then I think we'll need to > >>>>> make some changes to the current approach. > >>>>> > >>>>> As I believe we are in the latter stages of review for the syscall > >>>>> API, perhaps you could take a look and ensure that the current > >>>>> proposed API works for what you are envisioning with Landlock? > >> > >> I agree, and since the LSM syscalls are almost ready that should not > >> change much the timing. In fact, extending these syscalls might be > >> easier than tweaking the current procfs/attr API for Landlock specific > >> requirements (e.g. scoped visibility). We should ensure that these > >> syscalls would be a good fit to return file descriptors, but in the > >> short term we only need to know if a process is landlocked or not, so a > >> raw return value (0 or -errno) will be enough. > >> > >> Mentioning in the LSM syscalls patch series that they may deal with (and > >> return) file descriptors could help API reviewers though. > > > > It should be kept in mind that the current LSM syscalls only deal with > > the calling task, whereas the goal of this Landlock patch series is to > > inspect other tasks. A new LSM syscall would need to be created to > > handle pidfd e.g., named lsm_get_proc_attr() or lsm_get_pid_attr(). > > I think it would be lsm_get_pid_attr(). Yes, it's the obvious next step. > > > > > I'm not sure if this should be a generic LSM syscall or a Landlock > > syscall though. I have plan to handle processes other than the caller > > (e.g. to restrict an existing process hierarchy), so thinking about a > > Landlock-specific syscall could make sense. > > > > To summarize, creating a new LSM syscall to deal with pidfd and to get > > LSM process "status/attr" looks OK. However, Landlock-specific > > syscalls to deal with Landlock specificities (e.g. ruleset or domain > > file descriptor) make more sense. > > > > Having one LSM-generic syscall to get minimal Landlock attributes > > (i.e. mainly to know if a process is sandboxed), and another > > Landlock-specific syscall to do more things (e.g. get the domain file > > descriptor, restrict a task) seems reasonable. The second one would > > overlap with the first one though. What do you think? > > I find it difficult to think of a file descriptor as an attribute of > a process. To my (somewhat unorthodox) thinking a file descriptor is > a name for an object, not an attribute of the object. You can't access > an object by its attributes, but you can by its name. An attribute is > a description of the object. I'm perfectly happy with lsm_get_pid_attr() > returning an attribute that is a file descriptor if it describes the > process in some way, but not as a substitute for opening /proc/42. > > If I understand correctly: 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the process's landlock sandbox status: true/false. Is this a right fit for SELinux to also return the process's enforcing mode ? such as enforcing/permissive. 2> Landlock will have its own specific syscall to deal with Landlock specificities (e.g. ruleset or domain file descriptor).
On 5/30/2023 11:02 AM, Jeff Xu wrote: >>>> As I believe we are in the latter stages of review for the syscall >>>> API, perhaps you could take a look and ensure that the current >>>> proposed API works for what you are envisioning with Landlock? >>>> >>> Which review/patch to look for the proposed API ? >> https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ >> >> > How easy is it to add a customized LSM with new APIs? I haven't found it difficult, but that was in the pre-syscall era. Look at Landlock for an example of LSM specific syscalls, if you want to go that route. > I'm asking because there are some hard-coded constant/macro, i.e. > > +#define LSM_ID_LANDLOCK 111 > (Do IDs need to be sequential ?) No, but I would want a good reason for doing otherwise. > + define LSM_CONFIG_COUNT > > Today, only security/Kconfig change is needed to add a new LSM, I think ? That's correct. The syscall patches make it a trifle more difficult, requiring they be acknowledged in security.c. We could probably work around that, but it's really a small price to pay to get a constant value.
On 5/30/2023 11:05 AM, Jeff Xu wrote: > On Thu, May 25, 2023 at 9:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 5/24/2023 9:02 AM, Mickaël Salaün wrote: >>> On 24/05/2023 17:38, Mickaël Salaün wrote: >>>> On 23/05/2023 23:12, Paul Moore wrote: >>>>> On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: >>>>>> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> >>>>>> wrote: >>>>>>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler >>>>>>> <casey@schaufler-ca.com> wrote: >>>>>>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>>>>>>> Adds a new getprocattr hook function to the Landlock LSM, which >>>>>>>>> tracks >>>>>>>>> the landlocked state of the process. This is invoked when >>>>>>>>> user-space >>>>>>>>> reads /proc/[pid]/attr/domain >>>>>>>> Please don't add a Landlock specific entry directly in the attr/ >>>>>>>> directory. Add it only to attr/landlock. >>>>>>>> >>>>>>>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>>>>>>> away from the /proc/.../attr interfaces in favor of a new system >>>>>>>> call, >>>>>>>> which is in review. >>>>>>> What Casey said above. >>>>>>> >>>>>>> There is still some uncertainty around timing, and if we're perfectly >>>>>>> honest, acceptance of the new syscalls at the Linus level, but yes, I >>>>>>> would very much like to see the LSM infrastructure move away from >>>>>>> procfs and towards a syscall API. Part of the reasoning is that the >>>>>>> current procfs API is ill-suited to handle the multiple, stacked LSMs >>>>>>> and the other part being the complexity of procfs in a namespaced >>>>>>> system. If the syscall API is ultimately rejected, we will need to >>>>>>> revisit the idea of a procfs API, but even then I think we'll need to >>>>>>> make some changes to the current approach. >>>>>>> >>>>>>> As I believe we are in the latter stages of review for the syscall >>>>>>> API, perhaps you could take a look and ensure that the current >>>>>>> proposed API works for what you are envisioning with Landlock? >>>> I agree, and since the LSM syscalls are almost ready that should not >>>> change much the timing. In fact, extending these syscalls might be >>>> easier than tweaking the current procfs/attr API for Landlock specific >>>> requirements (e.g. scoped visibility). We should ensure that these >>>> syscalls would be a good fit to return file descriptors, but in the >>>> short term we only need to know if a process is landlocked or not, so a >>>> raw return value (0 or -errno) will be enough. >>>> >>>> Mentioning in the LSM syscalls patch series that they may deal with (and >>>> return) file descriptors could help API reviewers though. >>> It should be kept in mind that the current LSM syscalls only deal with >>> the calling task, whereas the goal of this Landlock patch series is to >>> inspect other tasks. A new LSM syscall would need to be created to >>> handle pidfd e.g., named lsm_get_proc_attr() or lsm_get_pid_attr(). >> I think it would be lsm_get_pid_attr(). Yes, it's the obvious next step. >> >>> I'm not sure if this should be a generic LSM syscall or a Landlock >>> syscall though. I have plan to handle processes other than the caller >>> (e.g. to restrict an existing process hierarchy), so thinking about a >>> Landlock-specific syscall could make sense. >>> >>> To summarize, creating a new LSM syscall to deal with pidfd and to get >>> LSM process "status/attr" looks OK. However, Landlock-specific >>> syscalls to deal with Landlock specificities (e.g. ruleset or domain >>> file descriptor) make more sense. >>> >>> Having one LSM-generic syscall to get minimal Landlock attributes >>> (i.e. mainly to know if a process is sandboxed), and another >>> Landlock-specific syscall to do more things (e.g. get the domain file >>> descriptor, restrict a task) seems reasonable. The second one would >>> overlap with the first one though. What do you think? >> I find it difficult to think of a file descriptor as an attribute of >> a process. To my (somewhat unorthodox) thinking a file descriptor is >> a name for an object, not an attribute of the object. You can't access >> an object by its attributes, but you can by its name. An attribute is >> a description of the object. I'm perfectly happy with lsm_get_pid_attr() >> returning an attribute that is a file descriptor if it describes the >> process in some way, but not as a substitute for opening /proc/42. >> >> > If I understand correctly: > 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the > process's landlock sandbox status: true/false. There would have to be a new LSM_ATTR_ENFORCMENT to query. Each LSM could then report what, if any, value it choose to. I can't say whether SELinux would take advantage of this. I don't see that Smack would report this attribute. > > Is this a right fit for SELinux to also return the process's enforcing > mode ? such as enforcing/permissive. > > 2> Landlock will have its own specific syscall to deal with Landlock > specificities (e.g. ruleset or domain file descriptor). I don't see how a syscall to load arbitrary LSM policy (e.g. landlock ruleset, Smack rules) would behave, so each LSM is on it's own regarding that. I doubt that the VFS crowd would be especially keen on an LSM creating file descriptors, but stranger things have happened.
On 30/05/2023 20:02, Jeff Xu wrote: >>>> >>>> As I believe we are in the latter stages of review for the syscall >>>> API, perhaps you could take a look and ensure that the current >>>> proposed API works for what you are envisioning with Landlock? >>>> >>> Which review/patch to look for the proposed API ? >> >> https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ >> >> > How easy is it to add a customized LSM with new APIs? > I'm asking because there are some hard-coded constant/macro, i.e. I guess this question is related to the Chromium OS LSM right? I think this would be a good opportunity to think about mainlining this LSM to avoid the hassle of dealing with LSM IDs. > > +#define LSM_ID_LANDLOCK 111 > (Do IDs need to be sequential ?) > > + define LSM_CONFIG_COUNT > > Today, only security/Kconfig change is needed to add a new LSM, I think ?
On 30/05/2023 21:19, Casey Schaufler wrote: > On 5/30/2023 11:05 AM, Jeff Xu wrote: >> On Thu, May 25, 2023 at 9:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote: >>> On 5/24/2023 9:02 AM, Mickaël Salaün wrote: >>>> On 24/05/2023 17:38, Mickaël Salaün wrote: >>>>> On 23/05/2023 23:12, Paul Moore wrote: >>>>>> On Tue, May 23, 2023 at 2:13 AM Jeff Xu <jeffxu@chromium.org> wrote: >>>>>>> On Mon, May 22, 2023 at 12:56 PM Paul Moore <paul@paul-moore.com> >>>>>>> wrote: >>>>>>>> On Thu, May 18, 2023 at 5:26 PM Casey Schaufler >>>>>>>> <casey@schaufler-ca.com> wrote: >>>>>>>>> On 5/18/2023 1:45 PM, Shervin Oloumi wrote: >>>>>>>>>> Adds a new getprocattr hook function to the Landlock LSM, which >>>>>>>>>> tracks >>>>>>>>>> the landlocked state of the process. This is invoked when >>>>>>>>>> user-space >>>>>>>>>> reads /proc/[pid]/attr/domain >>>>>>>>> Please don't add a Landlock specific entry directly in the attr/ >>>>>>>>> directory. Add it only to attr/landlock. >>>>>>>>> >>>>>>>>> Also be aware that the LSM maintainer (Paul Moore) wants to move >>>>>>>>> away from the /proc/.../attr interfaces in favor of a new system >>>>>>>>> call, >>>>>>>>> which is in review. >>>>>>>> What Casey said above. >>>>>>>> >>>>>>>> There is still some uncertainty around timing, and if we're perfectly >>>>>>>> honest, acceptance of the new syscalls at the Linus level, but yes, I >>>>>>>> would very much like to see the LSM infrastructure move away from >>>>>>>> procfs and towards a syscall API. Part of the reasoning is that the >>>>>>>> current procfs API is ill-suited to handle the multiple, stacked LSMs >>>>>>>> and the other part being the complexity of procfs in a namespaced >>>>>>>> system. If the syscall API is ultimately rejected, we will need to >>>>>>>> revisit the idea of a procfs API, but even then I think we'll need to >>>>>>>> make some changes to the current approach. >>>>>>>> >>>>>>>> As I believe we are in the latter stages of review for the syscall >>>>>>>> API, perhaps you could take a look and ensure that the current >>>>>>>> proposed API works for what you are envisioning with Landlock? >>>>> I agree, and since the LSM syscalls are almost ready that should not >>>>> change much the timing. In fact, extending these syscalls might be >>>>> easier than tweaking the current procfs/attr API for Landlock specific >>>>> requirements (e.g. scoped visibility). We should ensure that these >>>>> syscalls would be a good fit to return file descriptors, but in the >>>>> short term we only need to know if a process is landlocked or not, so a >>>>> raw return value (0 or -errno) will be enough. >>>>> >>>>> Mentioning in the LSM syscalls patch series that they may deal with (and >>>>> return) file descriptors could help API reviewers though. >>>> It should be kept in mind that the current LSM syscalls only deal with >>>> the calling task, whereas the goal of this Landlock patch series is to >>>> inspect other tasks. A new LSM syscall would need to be created to >>>> handle pidfd e.g., named lsm_get_proc_attr() or lsm_get_pid_attr(). >>> I think it would be lsm_get_pid_attr(). Yes, it's the obvious next step. >>> >>>> I'm not sure if this should be a generic LSM syscall or a Landlock >>>> syscall though. I have plan to handle processes other than the caller >>>> (e.g. to restrict an existing process hierarchy), so thinking about a >>>> Landlock-specific syscall could make sense. >>>> >>>> To summarize, creating a new LSM syscall to deal with pidfd and to get >>>> LSM process "status/attr" looks OK. However, Landlock-specific >>>> syscalls to deal with Landlock specificities (e.g. ruleset or domain >>>> file descriptor) make more sense. >>>> >>>> Having one LSM-generic syscall to get minimal Landlock attributes >>>> (i.e. mainly to know if a process is sandboxed), and another >>>> Landlock-specific syscall to do more things (e.g. get the domain file >>>> descriptor, restrict a task) seems reasonable. The second one would >>>> overlap with the first one though. What do you think? >>> I find it difficult to think of a file descriptor as an attribute of >>> a process. To my (somewhat unorthodox) thinking a file descriptor is >>> a name for an object, not an attribute of the object. You can't access >>> an object by its attributes, but you can by its name. An attribute is >>> a description of the object. I'm perfectly happy with lsm_get_pid_attr() >>> returning an attribute that is a file descriptor if it describes the >>> process in some way, but not as a substitute for opening /proc/42. We're talking about two kind of file descriptor. First, pidfd which is a file descriptor referring to a task, so yes pretty similar to /proc/42 . Second, a Landlock domain file descriptor, referring to a Landlock domain which contains a set of processes, similar to cgroups. A potential landlock_get_domain() syscall would take a pidfd as argument and return a Landlock domain file descriptor. I think lsm_get_pid_attr() would be better to always return raw data (current attribute values), which is simpler and make sure a syscall only return the same types. This would be type safe and avoid issues where file descriptors would be leaked of misused. >>> >>> >> If I understand correctly: >> 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the >> process's landlock sandbox status: true/false. > > There would have to be a new LSM_ATTR_ENFORCMENT to query. > Each LSM could then report what, if any, value it choose to. > I can't say whether SELinux would take advantage of this. > I don't see that Smack would report this attribute. I think such returned status for LSM_ATTR_ENFORCMENT query would make sense, but the syscall could also return -EPERM and other error codes. > >> >> Is this a right fit for SELinux to also return the process's enforcing >> mode ? such as enforcing/permissive. Paul could answer that, but I think it would be simpler to have two different queries, something like LSM_ATTR_ENFORCMENT and LSM_ATTR_PERMISSIVE queries. >> >> 2> Landlock will have its own specific syscall to deal with Landlock >> specificities (e.g. ruleset or domain file descriptor). > > I don't see how a syscall to load arbitrary LSM policy (e.g. landlock ruleset, > Smack rules) would behave, so each LSM is on it's own regarding that. I agree, Landlock-specific file descriptors should managed by Landlock-specific syscalls. > I doubt > that the VFS crowd would be especially keen on an LSM creating file descriptors, > but stranger things have happened. This is already the case with Landlock rulesets, so no issue with that. File descriptors are more and more used nowadays and it's a good thing.
On Wed, May 31, 2023 at 6:01 AM Mickaël Salaün <mic@digikod.net> wrote: > > > On 30/05/2023 20:02, Jeff Xu wrote: > >>>> > >>>> As I believe we are in the latter stages of review for the syscall > >>>> API, perhaps you could take a look and ensure that the current > >>>> proposed API works for what you are envisioning with Landlock? > >>>> > >>> Which review/patch to look for the proposed API ? > >> > >> https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ > >> > >> > > How easy is it to add a customized LSM with new APIs? > > I'm asking because there are some hard-coded constant/macro, i.e. > > I guess this question is related to the Chromium OS LSM right? I think > this would be a good opportunity to think about mainlining this LSM to > avoid the hassle of dealing with LSM IDs. > Yes :-) I agree it is good to think about upstream, there are things chromeOS did that can be beneficial to the main. At the same time, part of it might never be accepted by upstream because it is chromeOS specific, so those need to be cleaned up.
Hi Paul, On Wed, May 31, 2023 at 6:26 AM Mickaël Salaün <mic@digikod.net> wrote: > >>> > >>> > >> If I understand correctly: > >> 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the > >> process's landlock sandbox status: true/false. > > > > There would have to be a new LSM_ATTR_ENFORCMENT to query. > > Each LSM could then report what, if any, value it choose to. > > I can't say whether SELinux would take advantage of this. > > I don't see that Smack would report this attribute. > > I think such returned status for LSM_ATTR_ENFORCMENT query would make > sense, but the syscall could also return -EPERM and other error codes. > > > > > >> > >> Is this a right fit for SELinux to also return the process's enforcing > >> mode ? such as enforcing/permissive. > > Paul could answer that, but I think it would be simpler to have two > different queries, something like LSM_ATTR_ENFORCMENT and > LSM_ATTR_PERMISSIVE queries. > Hi Paul, what do you think ? Could SELinux have something like this. Thanks! -Jeff
On 6/1/2023 1:45 PM, Jeff Xu wrote: > On Wed, May 31, 2023 at 6:01 AM Mickaël Salaün <mic@digikod.net> wrote: >> >> On 30/05/2023 20:02, Jeff Xu wrote: >>>>>> As I believe we are in the latter stages of review for the syscall >>>>>> API, perhaps you could take a look and ensure that the current >>>>>> proposed API works for what you are envisioning with Landlock? >>>>>> >>>>> Which review/patch to look for the proposed API ? >>>> https://lore.kernel.org/lkml/20230428203417.159874-3-casey@schaufler-ca.com/T/ >>>> >>>> >>> How easy is it to add a customized LSM with new APIs? >>> I'm asking because there are some hard-coded constant/macro, i.e. >> I guess this question is related to the Chromium OS LSM right? I think >> this would be a good opportunity to think about mainlining this LSM to >> avoid the hassle of dealing with LSM IDs. >> > Yes :-) > I agree it is good to think about upstream, there are things chromeOS > did that can be beneficial to the main. At the same time, part of it > might never be accepted by upstream because it is chromeOS specific, > so those need to be cleaned up. Perhaps, but look at what's been done with SELinux in support of Android. You don't believe that the binder LSM hooks are for any other purpose, do you? You'll never know what turns out to be acceptable unless you give it a try.
On 6/1/2023 1:48 PM, Jeff Xu wrote: > Hi Paul, > > On Wed, May 31, 2023 at 6:26 AM Mickaël Salaün <mic@digikod.net> wrote: >>>>> >>>> If I understand correctly: >>>> 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the >>>> process's landlock sandbox status: true/false. >>> There would have to be a new LSM_ATTR_ENFORCMENT to query. >>> Each LSM could then report what, if any, value it choose to. >>> I can't say whether SELinux would take advantage of this. >>> I don't see that Smack would report this attribute. >> I think such returned status for LSM_ATTR_ENFORCMENT query would make >> sense, but the syscall could also return -EPERM and other error codes. >> >> >>>> Is this a right fit for SELinux to also return the process's enforcing >>>> mode ? such as enforcing/permissive. >> Paul could answer that, but I think it would be simpler to have two >> different queries, something like LSM_ATTR_ENFORCMENT and >> LSM_ATTR_PERMISSIVE queries. >> > Hi Paul, what do you think ? Could SELinux have something like this. Not Paul, but answering anyway - No, those are system wide attributes, not process (task) attributes. You want some other syscall, say lsm_get_system_attr() for those. > > Thanks! > -Jeff
On 01/06/2023 23:34, Casey Schaufler wrote: > On 6/1/2023 1:48 PM, Jeff Xu wrote: >> Hi Paul, >> >> On Wed, May 31, 2023 at 6:26 AM Mickaël Salaün <mic@digikod.net> wrote: >>>>>> >>>>> If I understand correctly: >>>>> 1> A new lsm syscall - lsm_get_pid_attr(): Landlock will return the >>>>> process's landlock sandbox status: true/false. >>>> There would have to be a new LSM_ATTR_ENFORCMENT to query. I guess there is a misunderstanding. What is the link between global system enforcement and the status of a sandboxed/restricted/enforced(?) process? The attribute would then be something like LSM_ATTR_RESTRICTED to get a process restriction status, which might be the same for all processes with system-wide policies (e.g., SELinux) but not for Landlock. >>>> Each LSM could then report what, if any, value it choose to. >>>> I can't say whether SELinux would take advantage of this. >>>> I don't see that Smack would report this attribute. >>> I think such returned status for LSM_ATTR_ENFORCMENT query would make >>> sense, but the syscall could also return -EPERM and other error codes. >>> >>> >>>>> Is this a right fit for SELinux to also return the process's enforcing >>>>> mode ? such as enforcing/permissive. >>> Paul could answer that, but I think it would be simpler to have two >>> different queries, something like LSM_ATTR_ENFORCMENT and >>> LSM_ATTR_PERMISSIVE queries. >>> >> Hi Paul, what do you think ? Could SELinux have something like this. > > Not Paul, but answering anyway - No, those are system wide attributes, not > process (task) attributes. You want some other syscall, say lsm_get_system_attr() > for those.
diff --git a/fs/proc/base.c b/fs/proc/base.c index 9e479d7d202b..b257ea704666 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2851,6 +2851,13 @@ static const struct pid_entry apparmor_attr_dir_stuff[] = { LSM_DIR_OPS(apparmor); #endif +#ifdef CONFIG_SECURITY_LANDLOCK +static const struct pid_entry landlock_attr_dir_stuff[] = { + ATTR("landlock", "domain", 0444), +}; +LSM_DIR_OPS(landlock); +#endif + static const struct pid_entry attr_dir_stuff[] = { ATTR(NULL, "current", 0666), ATTR(NULL, "prev", 0444), @@ -2866,6 +2873,10 @@ static const struct pid_entry attr_dir_stuff[] = { DIR("apparmor", 0555, proc_apparmor_attr_dir_inode_ops, proc_apparmor_attr_dir_ops), #endif +#ifdef CONFIG_SECURITY_LANDLOCK + DIR("landlock", 0555, + proc_landlock_attr_dir_inode_ops, proc_landlock_attr_dir_ops), +#endif }; static int proc_attr_dir_readdir(struct file *file, struct dir_context *ctx) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index adcea0fe7e68..2f8b0837a0fd 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -1280,6 +1280,42 @@ static int hook_file_truncate(struct file *const file) return -EACCES; } +/* process attribute interfaces */ + +/** + * landlock_getprocattr - Landlock process attribute getter + * @task: the object task + * @name: the name of the attribute in /proc/.../attr + * @value: where to put the result + * + * Performs access checks and writes any applicable results to value + * + * Returns the length of the result inside value or an error code + */ +static int landlock_getprocattr(struct task_struct *task, const char *name, + char **value) +{ + char *val = ""; + int slen; + + // If the tracing process is landlocked, ensure its domain is an + // ancestor to the target process domain. + if (landlocked(current)) + if (current == task || !task_is_scoped(current, task)) + return -EACCES; + + // The only supported attribute is "domain". + if (strcmp(name, "domain") != 0) + return -EINVAL; + + if (!landlocked(task)) + val = "none"; + + slen = strlen(val); + *value = val; + return slen; +} + static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_free_security, hook_inode_free_security), @@ -1302,6 +1338,8 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(file_alloc_security, hook_file_alloc_security), LSM_HOOK_INIT(file_open, hook_file_open), LSM_HOOK_INIT(file_truncate, hook_file_truncate), + + LSM_HOOK_INIT(getprocattr, landlock_getprocattr), }; __init void landlock_add_fs_hooks(void) diff --git a/security/landlock/fs.h b/security/landlock/fs.h index 488e4813680a..64145e8b5537 100644 --- a/security/landlock/fs.h +++ b/security/landlock/fs.h @@ -13,6 +13,7 @@ #include <linux/init.h> #include <linux/rcupdate.h> +#include "ptrace.h" #include "ruleset.h" #include "setup.h" diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c index 4c5b9cd71286..de943f0f3899 100644 --- a/security/landlock/ptrace.c +++ b/security/landlock/ptrace.c @@ -47,8 +47,8 @@ static bool domain_scope_le(const struct landlock_ruleset *const parent, return false; } -static bool task_is_scoped(const struct task_struct *const parent, - const struct task_struct *const child) +const bool task_is_scoped(const struct task_struct *const parent, + const struct task_struct *const child) { bool is_scoped; const struct landlock_ruleset *dom_parent, *dom_child; diff --git a/security/landlock/ptrace.h b/security/landlock/ptrace.h index 265b220ae3bf..c6eb08951fc1 100644 --- a/security/landlock/ptrace.h +++ b/security/landlock/ptrace.h @@ -11,4 +11,7 @@ __init void landlock_add_ptrace_hooks(void); +const bool task_is_scoped(const struct task_struct *const parent, + const struct task_struct *const child); + #endif /* _SECURITY_LANDLOCK_PTRACE_H */
Adds a new getprocattr hook function to the Landlock LSM, which tracks the landlocked state of the process. This is invoked when user-space reads /proc/[pid]/attr/domain to determine whether a given process is sand-boxed using Landlock. When the target process is not sand-boxed, the result is "none", otherwise the result is empty, as we still need to decide what kind of domain information is best to provide in "domain". The hook function also performs an access check. The request is rejected if the tracing process is the same as the target process, or if the tracing process domain is not an ancestor to the target process domain. Adds a new directory for landlock under the process attribute filesystem, and defines "domain" as a read-only process attribute entry for landlock. Signed-off-by: Shervin Oloumi <enlightened@chromium.org> --- fs/proc/base.c | 11 +++++++++++ security/landlock/fs.c | 38 ++++++++++++++++++++++++++++++++++++++ security/landlock/fs.h | 1 + security/landlock/ptrace.c | 4 ++-- security/landlock/ptrace.h | 3 +++ 5 files changed, 55 insertions(+), 2 deletions(-)