[PATCH 0/1] process attribute support for Landlock

[PATCH 0/1] process attribute support for Landlock

[enlightened]

From: Shervin Oloumi <enlightened@chromium.org>

Hi Mickaël,

I'm looking into adding a simple process attribute getter to Landlock so
we can determine the sand-boxing state of each process based on
/proc/[PID]/attr/current. As ChromeOS is expanding Landlock support,
this would help us paint a clear picture of Landlock coverage in the
fleet. I prepared a patch as a starting point, and would love to get
your feedback.

One area I am not very sure of is the case where more than one LSM is in
use. In such cases each LSM could have its own process attribute
getters and setters. What I learned is that when this is the case, the
kernel only calls the hook function for the LSM that is loaded first in
the CONFIG_LSM option. For example if landlock comes first
(CONFIG_LSM=landlock,...), then the kernel only calls the hook function
for Landlock, when the userspace interacts with process attribute files.
This is not a blocker for us, as we only currently care about reading
the Landlock related attributes, and my understanding is that this is
working as intended, but wanted to get your input.

Shervin Oloumi (1):
  lsm: adds process attribute getter for Landlock

 fs/proc/base.c         | 11 +++++++++++
 security/landlock/fs.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)


base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770
-- 
2.39.2.722.g9855ee24e9-goog

[PATCH 1/1] lsm: adds process attribute getter for Landlock

[enlightened]

From: Shervin Oloumi <enlightened@chromium.org>

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/current to determine whether a given process is
sand-boxed using Landlock.

Adds a new directory for landlock under the process attribute
filesystem, and defines "current" 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 | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9e479d7d202b..3ab29a965911 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", "current", 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..179ba22ce0fc 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1280,6 +1280,37 @@ static int hook_file_truncate(struct file *const file)
 	return -EACCES;
 }
 
+/* process attribute interfaces */
+
+/**
+ * landlock_getprocattr - Landlock process attribute getter
+ * @p: the object task
+ * @name: the name of the attribute in /proc/.../attr
+ * @value: where to put the result
+ *
+ * Writes the status of landlock to value
+ *
+ * Returns the length of the result inside value
+ */
+static int landlock_getprocattr(struct task_struct *task, const char *name,
+				char **value)
+{
+	char *val;
+	int slen;
+
+	if (strcmp(name, "current") != 0)
+		return -EINVAL;
+
+	if (landlocked(task))
+		val = "landlocked:1";
+	else
+		val = "landlocked:0";
+
+	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 +1333,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)
-- 
2.39.2.722.g9855ee24e9-goog

Re: [PATCH 0/1] process attribute support for Landlock

[Casey Schaufler]

On 3/2/2023 10:52 AM, enlightened@chromium.org wrote:
> From: Shervin Oloumi <enlightened@chromium.org>
>
> Hi Mickaël,
>
> I'm looking into adding a simple process attribute getter to Landlock so
> we can determine the sand-boxing state of each process based on
> /proc/[PID]/attr/current. As ChromeOS is expanding Landlock support,
> this would help us paint a clear picture of Landlock coverage in the
> fleet. I prepared a patch as a starting point, and would love to get
> your feedback.
>
> One area I am not very sure of is the case where more than one LSM is in
> use. In such cases each LSM could have its own process attribute
> getters and setters. What I learned is that when this is the case, the
> kernel only calls the hook function for the LSM that is loaded first in
> the CONFIG_LSM option. For example if landlock comes first
> (CONFIG_LSM=landlock,...), then the kernel only calls the hook function
> for Landlock, when the userspace interacts with process attribute files.
> This is not a blocker for us, as we only currently care about reading
> the Landlock related attributes, and my understanding is that this is
> working as intended, but wanted to get your input.

Emphasis: DO NOT USE /proc/.../attr/current! Seriously!

I made a huge mistake reusing current in Smack. If you want to
provide a Landlock attribute in /proc create a new one. Long term
we're trying to move to a syscall interface (patches in review).
But for the time being (and backports) a new name in attr is easy
enough to do and will avoid many headaches. Better yet, a subdirectory
in attr - /proc/.../attr/landlock - will avoid all sorts of issues.


>
> Shervin Oloumi (1):
>   lsm: adds process attribute getter for Landlock
>
>  fs/proc/base.c         | 11 +++++++++++
>  security/landlock/fs.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
>
> base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770

Re: [PATCH 1/1] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

On 3/2/2023 10:52 AM, enlightened@chromium.org wrote:
> From: Shervin Oloumi <enlightened@chromium.org>
>
> 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/current to determine whether a given process is
> sand-boxed using Landlock.
>
> Adds a new directory for landlock under the process attribute
> filesystem, and defines "current" as a read-only process attribute entry
> for landlock.

Do not reuse current. Create a new name or, better yet, a landlock
subdirectory.

>
> Signed-off-by: Shervin Oloumi <enlightened@chromium.org>
> ---
>  fs/proc/base.c         | 11 +++++++++++
>  security/landlock/fs.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 9e479d7d202b..3ab29a965911 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", "current", 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..179ba22ce0fc 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -1280,6 +1280,37 @@ static int hook_file_truncate(struct file *const file)
>  	return -EACCES;
>  }
>  
> +/* process attribute interfaces */
> +
> +/**
> + * landlock_getprocattr - Landlock process attribute getter
> + * @p: the object task
> + * @name: the name of the attribute in /proc/.../attr
> + * @value: where to put the result
> + *
> + * Writes the status of landlock to value
> + *
> + * Returns the length of the result inside value
> + */
> +static int landlock_getprocattr(struct task_struct *task, const char *name,
> +				char **value)
> +{
> +	char *val;
> +	int slen;
> +
> +	if (strcmp(name, "current") != 0)
> +		return -EINVAL;
> +
> +	if (landlocked(task))
> +		val = "landlocked:1";
> +	else
> +		val = "landlocked:0";
> +
> +	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 +1333,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)

Re: [PATCH 1/1] lsm: adds process attribute getter for Landlock

[Günther Noack]

Hello Shervin!

On Thu, Mar 02, 2023 at 10:52:57AM -0800, enlightened@chromium.org wrote:
> +	if (landlocked(task))
> +		val = "landlocked:1";
> +	else
> +		val = "landlocked:0";

Landlock policies can be stacked on top of each other, similar to
seccomp-bpf.

If a parent process has already enforced a (potentially trivial)
Landlock policy, then you can't tell apart based on this boolean
whether any additional policies are stacked on top. So what does
Chromium do with that information, if the flag is true for all the
involved processes that it manages?

Does this meet the needs of your intended use case?  Should your API
expose more information about the stacked policies, so that it becomes
possible to tell it apart?

Thanks,
–Günther

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

Hi Shervin,

Thanks for this initial patch.

On 02/03/2023 19:52, enlightened@chromium.org wrote:
> From: Shervin Oloumi <enlightened@chromium.org>
> 
> Hi Mickaël,
> 
> I'm looking into adding a simple process attribute getter to Landlock so
> we can determine the sand-boxing state of each process based on
> /proc/[PID]/attr/current. As ChromeOS is expanding Landlock support,
> this would help us paint a clear picture of Landlock coverage in the
> fleet. I prepared a patch as a starting point, and would love to get
> your feedback.

It would help to know exactly what are your needs short term, and long 
term. As Günther is wondering, what about nested sandboxing?

I'm thinking about a new /sys/kernel/security/landlock filesystem to be 
able to audit Landlock domains (i.e. sandboxes). As for your use case, 
it would be useful to be able to tie a process to a Landlock domain 
thanks to IDs.

Here are the guiding principles I think would make sense:
1. A sandboxed thread shall not be able to directly know if it is 
sandbox nor get any specific information from it's restrictions. The 
reason for this principle is to avoid applications to simply jump to 
conclusions (and change behavior) if they see that they are sandboxed 
with Landlock, instead of trying to access resources and falling back 
accordingly. A thread should only be able to inspect its 
own/children/nested domains.
2. Access to any Landlock domain information should be checked according 
to PTRACE_MODE_READ_FSCREDS, the Landlock domain hierarchy (cf. 
ptrace.c:domain_scope_le), and the first principle.
3. Any (domain) ID should be unique to the whole system (or maybe to the 
reader's PID namespace, and then in theory relative to the /proc 
content) to make it possible to compare Landlock domains (like 
/proc/[pid]/ns/* symlinks enable), and avoid trivial races.
4. These IDs should be the same during the whole lifetime of the related 
domain.
5. These IDs should not enable to infer information from other Landlock 
domains (e.g. how many are in use, current and parent domains), nor the 
kernel internals (e.g. addresses).
6. These IDs should not be sequential nor easily guessed to avoid 
anti-patterns (cf. file descriptors).
7. These IDs should be CRIU-friendly, to be able to easily restore such 
state. This doesn't help the previous principles and I don't know how/if 
CRIU supports namespace IDs though.

The /proc/[pid]/ns/* symlinks should be a good inspiration for a 
/proc/[pid]/attr/landlock/domain symlink with similar properties. Such 
file could then be used to pin or enforce the same Landlock domain on 
other threads in the future (out of scope for this patch series). Being 
able to open such "domain" file would make it possible to avoid races 
while reading the related ID and looking for the related entry in 
/sys/kernel/security/landlock/ by holding this file open.

It would be nice if the /proc/[pid]/attr/landlock directory would only 
exists if Landlock is enabled.

Similarly, /proc/[pid]/attr/landlock/domain should only exist (or be 
viewable) for a thread if [pid] is part of one of its child domain.

For now, I don't see any file in /proc/[pid]/attr/landlock/ other than 
"domain" that would make sense, but a dedicated directory is useful anyway.

I though about an entire file hierarchy to reflect a Landlock domain 
(e.g., with rule attributes), but that would make the /proc filesystem 
dynamically deep, so this should be dedicated to the 
/sys/kernel/security/landlock filesystem, but tied with /proc in some 
way, in this case with same domain IDs.


> 
> One area I am not very sure of is the case where more than one LSM is in
> use. In such cases each LSM could have its own process attribute
> getters and setters. What I learned is that when this is the case, the
> kernel only calls the hook function for the LSM that is loaded first in
> the CONFIG_LSM option. For example if landlock comes first
> (CONFIG_LSM=landlock,...), then the kernel only calls the hook function
> for Landlock, when the userspace interacts with process attribute files.
> This is not a blocker for us, as we only currently care about reading
> the Landlock related attributes, and my understanding is that this is
> working as intended, but wanted to get your input.

Using the /proc/[pid]/attr/landlock/domain path will remove this issue.

> 
> Shervin Oloumi (1):
>    lsm: adds process attribute getter for Landlock
> 
>   fs/proc/base.c         | 11 +++++++++++
>   security/landlock/fs.c | 33 +++++++++++++++++++++++++++++++++
>   2 files changed, 44 insertions(+)
> 
> 
> base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770

Re: [PATCH 0/1] process attribute support for Landlock

[Shervin Oloumi]

> Emphasis: DO NOT USE /proc/.../attr/current! Seriously!
>
> I made a huge mistake reusing current in Smack. If you want to
> provide a Landlock attribute in /proc create a new one. Long term
> we're trying to move to a syscall interface (patches in review).
> But for the time being (and backports) a new name in attr is easy
> enough to do and will avoid many headaches. Better yet, a subdirectory
> in attr - /proc/.../attr/landlock - will avoid all sorts of issues.

Thanks for flagging this. Creating a new directory and attribute name
for this makes sense, but you can still only interact with process
attributes of a single LSM on the system right? Just want to make sure
my understanding is correct, because even when Landlock uses a
different name and a new subdirectory for this, still the kernel only
calls the hook function of the first LSM on the list (Landlock in our
case). So reading proc/.../attr/current or any other attribute besides
/proc/.../attr/landlock/some_attribute would result in EINVAL if
Landlock was first on the CONFIG_LSM list.

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

On 06/03/2023 20:18, Mickaël Salaün wrote:
> Hi Shervin,
> 
> Thanks for this initial patch.
> 
> On 02/03/2023 19:52, enlightened@chromium.org wrote:
>> From: Shervin Oloumi <enlightened@chromium.org>
>>
>> Hi Mickaël,
>>
>> I'm looking into adding a simple process attribute getter to Landlock so
>> we can determine the sand-boxing state of each process based on
>> /proc/[PID]/attr/current. As ChromeOS is expanding Landlock support,
>> this would help us paint a clear picture of Landlock coverage in the
>> fleet. I prepared a patch as a starting point, and would love to get
>> your feedback.
> 
> It would help to know exactly what are your needs short term, and long
> term. As Günther is wondering, what about nested sandboxing?
> 
> I'm thinking about a new /sys/kernel/security/landlock filesystem to be
> able to audit Landlock domains (i.e. sandboxes). As for your use case,
> it would be useful to be able to tie a process to a Landlock domain
> thanks to IDs.
> 
> Here are the guiding principles I think would make sense:
> 1. A sandboxed thread shall not be able to directly know if it is
> sandbox nor get any specific information from it's restrictions. The
> reason for this principle is to avoid applications to simply jump to
> conclusions (and change behavior) if they see that they are sandboxed
> with Landlock, instead of trying to access resources and falling back
> accordingly. A thread should only be able to inspect its
> own/children/nested domains.
> 2. Access to any Landlock domain information should be checked according
> to PTRACE_MODE_READ_FSCREDS, the Landlock domain hierarchy (cf.
> ptrace.c:domain_scope_le), and the first principle.

We could get some inspiration from pidfd and read the domain ID (or even 
the domain hierarchy) from /proc/self/fdinfo/*. This doesn't require a 
symlink (just a regular file), and it enables to have a way to control 
the domain lifetime by keeping the FD opened (e.g. to look into 
/sys/kernel/security/landlock/*). For now, we can then postpone the 
domain ID design (and the related fdinfo specificity).

To summarize, we would be able to identify if Landlock is enabled 
(according to the "attr/landlock" directory existence) and if a thread 
is sandboxed (according to the "attr/landlock/domain" file existence), 
but nothing more for now. The "domain" file won't even need any file 
operation.

I'd still like to know the exact requirements to identify future 
developments.


> 3. Any (domain) ID should be unique to the whole system (or maybe to the
> reader's PID namespace, and then in theory relative to the /proc
> content) to make it possible to compare Landlock domains (like
> /proc/[pid]/ns/* symlinks enable), and avoid trivial races.
> 4. These IDs should be the same during the whole lifetime of the related
> domain.
> 5. These IDs should not enable to infer information from other Landlock
> domains (e.g. how many are in use, current and parent domains), nor the
> kernel internals (e.g. addresses).
> 6. These IDs should not be sequential nor easily guessed to avoid
> anti-patterns (cf. file descriptors).
> 7. These IDs should be CRIU-friendly, to be able to easily restore such
> state. This doesn't help the previous principles and I don't know how/if
> CRIU supports namespace IDs though.
> 
> The /proc/[pid]/ns/* symlinks should be a good inspiration for a
> /proc/[pid]/attr/landlock/domain symlink with similar properties. Such
> file could then be used to pin or enforce the same Landlock domain on
> other threads in the future (out of scope for this patch series). Being
> able to open such "domain" file would make it possible to avoid races
> while reading the related ID and looking for the related entry in
> /sys/kernel/security/landlock/ by holding this file open.
> 
> It would be nice if the /proc/[pid]/attr/landlock directory would only
> exists if Landlock is enabled.
> 
> Similarly, /proc/[pid]/attr/landlock/domain should only exist (or be
> viewable) for a thread if [pid] is part of one of its child domain.
> 
> For now, I don't see any file in /proc/[pid]/attr/landlock/ other than
> "domain" that would make sense, but a dedicated directory is useful anyway.
> 
> I though about an entire file hierarchy to reflect a Landlock domain
> (e.g., with rule attributes), but that would make the /proc filesystem
> dynamically deep, so this should be dedicated to the
> /sys/kernel/security/landlock filesystem, but tied with /proc in some
> way, in this case with same domain IDs.
> 
> 
>>
>> One area I am not very sure of is the case where more than one LSM is in
>> use. In such cases each LSM could have its own process attribute
>> getters and setters. What I learned is that when this is the case, the
>> kernel only calls the hook function for the LSM that is loaded first in
>> the CONFIG_LSM option. For example if landlock comes first
>> (CONFIG_LSM=landlock,...), then the kernel only calls the hook function
>> for Landlock, when the userspace interacts with process attribute files.
>> This is not a blocker for us, as we only currently care about reading
>> the Landlock related attributes, and my understanding is that this is
>> working as intended, but wanted to get your input.
> 
> Using the /proc/[pid]/attr/landlock/domain path will remove this issue.
> 
>>
>> Shervin Oloumi (1):
>>     lsm: adds process attribute getter for Landlock
>>
>>    fs/proc/base.c         | 11 +++++++++++
>>    security/landlock/fs.c | 33 +++++++++++++++++++++++++++++++++
>>    2 files changed, 44 insertions(+)
>>
>>
>> base-commit: e2ca6ba6ba0152361aa4fcbf6067db71b2c7a770

Re: [PATCH 0/1] process attribute support for Landlock

[Casey Schaufler]

On 3/6/2023 2:40 PM, Shervin Oloumi wrote:
>> Emphasis: DO NOT USE /proc/.../attr/current! Seriously!
>>
>> I made a huge mistake reusing current in Smack. If you want to
>> provide a Landlock attribute in /proc create a new one. Long term
>> we're trying to move to a syscall interface (patches in review).
>> But for the time being (and backports) a new name in attr is easy
>> enough to do and will avoid many headaches. Better yet, a subdirectory
>> in attr - /proc/.../attr/landlock - will avoid all sorts of issues.
> Thanks for flagging this. Creating a new directory and attribute name
> for this makes sense, but you can still only interact with process
> attributes of a single LSM on the system right? Just want to make sure
> my understanding is correct, because even when Landlock uses a
> different name and a new subdirectory for this, still the kernel only
> calls the hook function of the first LSM on the list (Landlock in our
> case). So reading proc/.../attr/current or any other attribute besides
> /proc/.../attr/landlock/some_attribute would result in EINVAL if
> Landlock was first on the CONFIG_LSM list.

That's true, but we've been able to move parts of the general stacking
process forward when there's a specific need for them. In this case we'd
need changes for security_[gs]etprocattr() that are already understood.

Re: [PATCH 0/1] process attribute support for Landlock

[Shervin Oloumi]

Thanks all for the feedback. This is in reply to Mickaël, but should
answer Günther's questions as well.

> It would help to know exactly what are your needs short term, and long
> term. As Günther is wondering, what about nested sandboxing?

Our plan is to use the "landlocked" process attribute defined in the
patch to determine the sandbox state of the system processes and send
information to our metrics server regarding Landlock coverage. For
example, the percentage of processes on the system that are sandboxed
using Landlock.

Given that we use Landlock in a very specific and controlled way, we
are not concerned about the inheritance behavior and nested policies,
at least for the use case of metrics. When daemons are launched in
ChromiumOS, they have a pre-defined sandboxing configuration that
dictates whether Landlock should be applied or not. So this attribute
would help us verify that the processes running on devices in the wild
indeed have the general sandboxing state that we expect and the
reality matches our expectation.

Long-term, it would be useful to learn more information about domains
and policies through the process attribute interface, but we do not
currently have a need for that, apart from maybe doing troubleshooting
when defining Landlock rules for system daemons.

> I'm thinking about a new /sys/kernel/security/landlock filesystem to be
> able to audit Landlock domains (i.e. sandboxes). As for your use case,
> it would be useful to be able to tie a process to a Landlock domain
> thanks to IDs.

I think this goes beyond the scope for our current needs, but
certainly a nice feature that we could potentially use in the future.
So given this, I was wondering what would be the minimum changes we
can make now (if any) that would serve our purpose AND would be
compatible with your long-term vision, without getting too deep into
the implementation of broader concepts. We are flexible on the
approach for querying the landlocked property (for example whether it
is based on the presence of a /proc/.../attr/domain or actually
reading an attribute).

> Here are the guiding principles I think would make sense:
> 1. A sandboxed thread shall not be able to directly know if it is
> sandbox nor get any specific information from it's restrictions. The
> reason for this principle is to avoid applications to simply jump to
> conclusions (and change behavior) if they see that they are sandboxed
> with Landlock, instead of trying to access resources and falling back
> accordingly. A thread should only be able to inspect its
> own/children/nested domains.
> 2. Access to any Landlock domain information should be checked according
> to PTRACE_MODE_READ_FSCREDS, the Landlock domain hierarchy (cf.
> ptrace.c:domain_scope_le), and the first principle.

One thing worth noting is that we use a system daemon to read process
attributes. We have the ptrace_scope set to 1 and the daemon reading
the attributes does have cap_sys_ptrace, however it is not related to
the other processes on the system. Do you see this as a problem given
principle#1?

> 3. Any (domain) ID should be unique to the whole system (or maybe to the
> reader's PID namespace, and then in theory relative to the /proc
> content) to make it possible to compare Landlock domains (like
> /proc/[pid]/ns/* symlinks enable), and avoid trivial races.
> 4. These IDs should be the same during the whole lifetime of the related
> domain.
> 5. These IDs should not enable to infer information from other Landlock
> domains (e.g. how many are in use, current and parent domains), nor the
> kernel internals (e.g. addresses).
> 6. These IDs should not be sequential nor easily guessed to avoid
> anti-patterns (cf. file descriptors).
> 7. These IDs should be CRIU-friendly, to be able to easily restore such
> state. This doesn't help the previous principles and I don't know how/if
> CRIU supports namespace IDs though.

Since these points are regarding the properties of the domain IDs,
they should not interfere with anything we would implement for
determining the process sandbox status in any initial patch, but are
good to know.

> It would be nice if the /proc/[pid]/attr/landlock directory would only
> exists if Landlock is enabled.

This is the current default behavior I believe.

> Similarly, /proc/[pid]/attr/landlock/domain should only exist (or be
> viewable) for a thread if [pid] is part of one of its child domain.

I am not sure if this is a blocker for our model of a single daemon
querying the attribute for all processes. Are you suggesting that the
file would not exist from the view of the other processes if they are
not the parent process?

> For now, I don't see any file in /proc/[pid]/attr/landlock/ other than
> "domain" that would make sense, but a dedicated directory is useful anyway.

Determining the sandbox status of processes based on the existence of
/proc/[pid]/landlock/domain would serve our simple use case, pending
the open questions/potential blockers above and a clarification on
minimum requirements for an initial version.

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

On 08/03/2023 23:25, Shervin Oloumi wrote:
> Thanks all for the feedback. This is in reply to Mickaël, but should
> answer Günther's questions as well.
> 
>> It would help to know exactly what are your needs short term, and long
>> term. As Günther is wondering, what about nested sandboxing?
> 
> Our plan is to use the "landlocked" process attribute defined in the
> patch to determine the sandbox state of the system processes and send
> information to our metrics server regarding Landlock coverage. For
> example, the percentage of processes on the system that are sandboxed
> using Landlock.
> 
> Given that we use Landlock in a very specific and controlled way, we
> are not concerned about the inheritance behavior and nested policies,
> at least for the use case of metrics. When daemons are launched in
> ChromiumOS, they have a pre-defined sandboxing configuration that
> dictates whether Landlock should be applied or not. So this attribute
> would help us verify that the processes running on devices in the wild
> indeed have the general sandboxing state that we expect and the
> reality matches our expectation.
> 
> Long-term, it would be useful to learn more information about domains
> and policies through the process attribute interface, but we do not
> currently have a need for that, apart from maybe doing troubleshooting
> when defining Landlock rules for system daemons.

OK, it makes sense.


> 
>> I'm thinking about a new /sys/kernel/security/landlock filesystem to be
>> able to audit Landlock domains (i.e. sandboxes). As for your use case,
>> it would be useful to be able to tie a process to a Landlock domain
>> thanks to IDs.
> 
> I think this goes beyond the scope for our current needs, but
> certainly a nice feature that we could potentially use in the future.
> So given this, I was wondering what would be the minimum changes we
> can make now (if any) that would serve our purpose AND would be
> compatible with your long-term vision, without getting too deep into
> the implementation of broader concepts. We are flexible on the
> approach for querying the landlocked property (for example whether it
> is based on the presence of a /proc/.../attr/domain or actually
> reading an attribute).

Yes, the approach I suggested, check the /proc/.../attr/landlock/domain 
presence would enable you to check the landlocked state of a process. It 
should not change much from your initial patch. In fact it will be 
quicker to check because there is no need for the open/read/close 
syscalls, but only faccessat2.


>> Here are the guiding principles I think would make sense:
>> 1. A sandboxed thread shall not be able to directly know if it is
>> sandbox nor get any specific information from it's restrictions. The
>> reason for this principle is to avoid applications to simply jump to
>> conclusions (and change behavior) if they see that they are sandboxed
>> with Landlock, instead of trying to access resources and falling back
>> accordingly. A thread should only be able to inspect its
>> own/children/nested domains.
>> 2. Access to any Landlock domain information should be checked according
>> to PTRACE_MODE_READ_FSCREDS, the Landlock domain hierarchy (cf.
>> ptrace.c:domain_scope_le), and the first principle.
> 
> One thing worth noting is that we use a system daemon to read process
> attributes. We have the ptrace_scope set to 1 and the daemon reading
> the attributes does have cap_sys_ptrace, however it is not related to
> the other processes on the system. Do you see this as a problem given
> principle#1?

That should work fine because your deamon is more privileged than the 
checked processes.


>> 3. Any (domain) ID should be unique to the whole system (or maybe to the
>> reader's PID namespace, and then in theory relative to the /proc
>> content) to make it possible to compare Landlock domains (like
>> /proc/[pid]/ns/* symlinks enable), and avoid trivial races.
>> 4. These IDs should be the same during the whole lifetime of the related
>> domain.
>> 5. These IDs should not enable to infer information from other Landlock
>> domains (e.g. how many are in use, current and parent domains), nor the
>> kernel internals (e.g. addresses).
>> 6. These IDs should not be sequential nor easily guessed to avoid
>> anti-patterns (cf. file descriptors).
>> 7. These IDs should be CRIU-friendly, to be able to easily restore such
>> state. This doesn't help the previous principles and I don't know how/if
>> CRIU supports namespace IDs though.
> 
> Since these points are regarding the properties of the domain IDs,
> they should not interfere with anything we would implement for
> determining the process sandbox status in any initial patch, but are
> good to know.
> 
>> It would be nice if the /proc/[pid]/attr/landlock directory would only
>> exists if Landlock is enabled.
> 
> This is the current default behavior I believe.
> 
>> Similarly, /proc/[pid]/attr/landlock/domain should only exist (or be
>> viewable) for a thread if [pid] is part of one of its child domain.
> 
> I am not sure if this is a blocker for our model of a single daemon
> querying the attribute for all processes. Are you suggesting that the
> file would not exist from the view of the other processes if they are
> not the parent process?

Not the parent process, but a parent domain, *or in no domain at all*, 
which is your case.

> 
>> For now, I don't see any file in /proc/[pid]/attr/landlock/ other than
>> "domain" that would make sense, but a dedicated directory is useful anyway.
> 
> Determining the sandbox status of processes based on the existence of
> /proc/[pid]/landlock/domain would serve our simple use case, pending
> the open questions/potential blockers above and a clarification on
> minimum requirements for an initial version.

It should be fine for all these use cases, and only requires a small set 
of changes for now.

Re: [PATCH 0/1] process attribute support for Landlock

[Günther Noack]

Hi!

On Wed, Mar 15, 2023 at 10:56:03AM +0100, Mickaël Salaün wrote:
> On 08/03/2023 23:25, Shervin Oloumi wrote:
> > Thanks all for the feedback. This is in reply to Mickaël, but should
> > answer Günther's questions as well.
> > 
> > > It would help to know exactly what are your needs short term, and long
> > > term. As Günther is wondering, what about nested sandboxing?
> > 
> > Our plan is to use the "landlocked" process attribute defined in the
> > patch to determine the sandbox state of the system processes and send
> > information to our metrics server regarding Landlock coverage. For
> > example, the percentage of processes on the system that are sandboxed
> > using Landlock.
> > 
> > Given that we use Landlock in a very specific and controlled way, we
> > are not concerned about the inheritance behavior and nested policies,
> > at least for the use case of metrics. When daemons are launched in
> > ChromiumOS, they have a pre-defined sandboxing configuration that
> > dictates whether Landlock should be applied or not. So this attribute
> > would help us verify that the processes running on devices in the wild
> > indeed have the general sandboxing state that we expect and the
> > reality matches our expectation.
> > 
> > Long-term, it would be useful to learn more information about domains
> > and policies through the process attribute interface, but we do not
> > currently have a need for that, apart from maybe doing troubleshooting
> > when defining Landlock rules for system daemons.
> 
> OK, it makes sense.

Fair enough.  I missed the fact that this was about the OS rather than
the browser.

Still, out of curiosity: Hypothetically, if you were to expose the
number of stacked Landlock policies instead of the boolean in that
place -- would there be any drawbacks to that which I'm overlooking?

It seems to me, superficially, that the implementation should be
similarly simple, it would be useful in more cases where Landlock
users do not have control over the full OS, and I can't currently see
any cases where having a number instead of a boolean would complicate
the usage from userspace?  Am I missing something?

(But in any case, the boolean is also fine I think.)


> > > Here are the guiding principles I think would make sense:
> > > 1. A sandboxed thread shall not be able to directly know if it is
> > > sandbox nor get any specific information from it's restrictions. The
> > > reason for this principle is to avoid applications to simply jump to
> > > conclusions (and change behavior) if they see that they are sandboxed
> > > with Landlock, instead of trying to access resources and falling back
> > > accordingly. A thread should only be able to inspect its
> > > own/children/nested domains.

(Small remark:

Doing anything differently depending on whether and how you are
landlocked is definitely an antipattern which we should not encourage.
But I'm not sure whether we can hide the fact very easily.

It's already possible for a thread to detect whether it is landlocked,
by using this hack: Create a new thread and then in that thread count
how many additional sandboxes you can stack on top.

If you have knowledge about what Landlock configuration you are
looking for, it will be even easier to detect.

I hope noone takes the above example as inspiration.)

–Günther

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

On 16/03/2023 07:19, Günther Noack wrote:
> Hi!
> 
> On Wed, Mar 15, 2023 at 10:56:03AM +0100, Mickaël Salaün wrote:
>> On 08/03/2023 23:25, Shervin Oloumi wrote:
>>> Thanks all for the feedback. This is in reply to Mickaël, but should
>>> answer Günther's questions as well.
>>>
>>>> It would help to know exactly what are your needs short term, and long
>>>> term. As Günther is wondering, what about nested sandboxing?
>>>
>>> Our plan is to use the "landlocked" process attribute defined in the
>>> patch to determine the sandbox state of the system processes and send
>>> information to our metrics server regarding Landlock coverage. For
>>> example, the percentage of processes on the system that are sandboxed
>>> using Landlock.
>>>
>>> Given that we use Landlock in a very specific and controlled way, we
>>> are not concerned about the inheritance behavior and nested policies,
>>> at least for the use case of metrics. When daemons are launched in
>>> ChromiumOS, they have a pre-defined sandboxing configuration that
>>> dictates whether Landlock should be applied or not. So this attribute
>>> would help us verify that the processes running on devices in the wild
>>> indeed have the general sandboxing state that we expect and the
>>> reality matches our expectation.
>>>
>>> Long-term, it would be useful to learn more information about domains
>>> and policies through the process attribute interface, but we do not
>>> currently have a need for that, apart from maybe doing troubleshooting
>>> when defining Landlock rules for system daemons.
>>
>> OK, it makes sense.
> 
> Fair enough.  I missed the fact that this was about the OS rather than
> the browser.
> 
> Still, out of curiosity: Hypothetically, if you were to expose the
> number of stacked Landlock policies instead of the boolean in that
> place -- would there be any drawbacks to that which I'm overlooking?
> 
> It seems to me, superficially, that the implementation should be
> similarly simple, it would be useful in more cases where Landlock
> users do not have control over the full OS, and I can't currently see
> any cases where having a number instead of a boolean would complicate
> the usage from userspace?  Am I missing something?

I'd like to hear from Shervin, but here is my reasoning.

I'd like to avoid as much as possible the procfs interface (for security 
and usability reasons specific to Landlock), but to only extend it to 
the minimal requirement needed to tie a process to a Landlock domain. 
Exposing any domain information (e.g. nested domain depth) should then 
be managed by a new interface (i.e. /sys/kernel/security/landlock), and 
we should avoid duplicating this information in the procfs interface. 
Making an attr/landlock/domain file gives the information that a 
(nested) domain exists for this PID, which is anyway a required minimal 
interface.


> 
> (But in any case, the boolean is also fine I think.)
> 
> 
>>>> Here are the guiding principles I think would make sense:
>>>> 1. A sandboxed thread shall not be able to directly know if it is
>>>> sandbox nor get any specific information from it's restrictions. The
>>>> reason for this principle is to avoid applications to simply jump to
>>>> conclusions (and change behavior) if they see that they are sandboxed
>>>> with Landlock, instead of trying to access resources and falling back
>>>> accordingly. A thread should only be able to inspect its
>>>> own/children/nested domains.

For a more up-to-date idea, see 
https://lore.kernel.org/all/ee878a04-51f4-a8aa-7d4c-13e519b7409d@digikod.net/
The fdinfo trick would not be required though, I found a better design 
to tie an opened domain to its properties. Anyway, this is future work 
and would be compatible with the /proc/[pid]/attr/landlock/domain file.

> 
> (Small remark:
> 
> Doing anything differently depending on whether and how you are
> landlocked is definitely an antipattern which we should not encourage.
> But I'm not sure whether we can hide the fact very easily.
> 
> It's already possible for a thread to detect whether it is landlocked,
> by using this hack: Create a new thread and then in that thread count
> how many additional sandboxes you can stack on top.
> 
> If you have knowledge about what Landlock configuration you are
> looking for, it will be even easier to detect.
> 
> I hope noone takes the above example as inspiration.)

Indeed, there are multiple ways to detect that a thread is landlocked, 
but we should not make any effort to make it easy to check unless there 
is at least a valid use case. I'd like to only add/show new interfaces 
were/when they are needed, in this case, "a thread should only be able 
to inspect/see its nested domains". For now, the only valid usage I can 
think of to detect sandboxing is for debug and metrics, not for a 
legitimate sandboxed application. Furthermore, what I'd like to have for 
Landlock is the ability to use this "domain" file to get access to 
domain properties (e.g. handled accesses, rules), and giving the sandbox 
configuration to the sandboxed process looks like a bad idea.

Re: [PATCH 0/1] process attribute support for Landlock

[Shervin Oloumi]

Sorry for the delay on this. I think there is a fundamental issue here
that needs to be resolved first, and that is the limitation of the
kernel that only one LSM's hook function can be called through the
procfs attribute interface. This is a blocker for us (and I imagine
for others), since implementing any LandLock attribute API would block
the existing SELinux hook function, which is used to surface domain
information. `ps` also uses it to display domain information when you
pass `-Z`. Please note, this is independent of which path and filename
we use for LandLock. Even when the "domain" file is placed under a
different directory, for example `/proc/[pid]/attr/landlock/domain`
the kernel only calls the Landlock hook function for any interaction
with any files under attr (the kernel always calls only the hook
function for the first loaded LSM in the kernel config). So if anyone
in this thread has any information on whether there is work on
progress for addressing this issue, that would be helpful.

As for the patch, I will just provide what I have so far, which I
think is more in line with the approach you suggested, so that it can
perhaps at some point be useful, once the above limitation is
resolved.

> Yes, the approach I suggested, check the /proc/.../attr/landlock/domain
> presence would enable you to check the landlocked state of a process. It
> should not change much from your initial patch. In fact it will be
> quicker to check because there is no need for the open/read/close
> syscalls, but only faccessat2.

I played around with this idea but ran into a problem; I'm not sure if
it is possible to implement a behavior where the existence/viewability
of the `/proc/.../attr/landlock/domain` is conditional. The `domain`
file is predefined with set permissions in `fs/proc/base.c` (as done
in the patch) and it is always present if landlock is enabled.
Additionally, the `landlock_getprocattr` hook function only gets
called when the file `/proc/.../attr/landlock/domain` is opened and
read, so I'm not sure how the file visibility can be manipulated.

The closest way I can think of to imitate the suggested behavior is to
return `EACCES` in the hook function if the checking process domain is
not related to the target process domain and return "none" (indicating
there is no Lanldock domain associated with this process) if the
domain check passes and the target process is not landlocked. In cases
where the access check passes (or when the checking process is not
landlocked) and the target process is landlocked reading the file
could just return nothing (maybe in the future this will return the
domain ID...TBD).

[PATCH v2] lsm: adds process attribute getter for Landlock

[Shervin Oloumi]

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))
+			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 */
-- 
2.40.1.698.g37aff9b760-goog

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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 */

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Paul Moore]

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?

-- 
paul-moore.com

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Jeff Xu]

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

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Paul Moore]

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.

-- 
paul-moore.com

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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?

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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.

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

On 18/05/2023 22:44, Shervin Oloumi wrote:
> Sorry for the delay on this. I think there is a fundamental issue here
> that needs to be resolved first, and that is the limitation of the
> kernel that only one LSM's hook function can be called through the
> procfs attribute interface. This is a blocker for us (and I imagine
> for others), since implementing any LandLock attribute API would block
> the existing SELinux hook function, which is used to surface domain
> information. `ps` also uses it to display domain information when you
> pass `-Z`. Please note, this is independent of which path and filename
> we use for LandLock. Even when the "domain" file is placed under a
> different directory, for example `/proc/[pid]/attr/landlock/domain`
> the kernel only calls the Landlock hook function for any interaction
> with any files under attr (the kernel always calls only the hook
> function for the first loaded LSM in the kernel config). So if anyone
> in this thread has any information on whether there is work on
> progress for addressing this issue, that would be helpful.

This seems to be an LSM stacking issue. Do the LSM syscalls also have 
this issue? This should be part of tests.

Re: [PATCH 0/1] process attribute support for Landlock

[Mickaël Salaün]

On 18/05/2023 22:44, Shervin Oloumi wrote:

[...]

> 
> As for the patch, I will just provide what I have so far, which I
> think is more in line with the approach you suggested, so that it can
> perhaps at some point be useful, once the above limitation is
> resolved.
> 
>> Yes, the approach I suggested, check the /proc/.../attr/landlock/domain
>> presence would enable you to check the landlocked state of a process. It
>> should not change much from your initial patch. In fact it will be
>> quicker to check because there is no need for the open/read/close
>> syscalls, but only faccessat2.
> 
> I played around with this idea but ran into a problem; I'm not sure if
> it is possible to implement a behavior where the existence/viewability
> of the `/proc/.../attr/landlock/domain` is conditional. The `domain`
> file is predefined with set permissions in `fs/proc/base.c` (as done
> in the patch) and it is always present if landlock is enabled.
> Additionally, the `landlock_getprocattr` hook function only gets
> called when the file `/proc/.../attr/landlock/domain` is opened and
> read, so I'm not sure how the file visibility can be manipulated.

It would work the same as proc/self/fd, but may require some API changes 
to be in line with the LSM API. Relying on the LSM syscalls would not 
require to change this API.


> 
> The closest way I can think of to imitate the suggested behavior is to
> return `EACCES` in the hook function if the checking process domain is
> not related to the target process domain and return "none" (indicating
> there is no Lanldock domain associated with this process) if the
> domain check passes and the target process is not landlocked. In cases
> where the access check passes (or when the checking process is not
> landlocked) and the target process is landlocked reading the file
> could just return nothing (maybe in the future this will return the
> domain ID...TBD).

I really want the concept I proposed to be used: a sandbox process 
should not be able to get any data from processes in the same sandbox 
(except through side effects such as nesting limit) nor for processes 
not in a nested sandbox. In fact, this should just use 
ptrace_may_access() (as already done for sensitive procfs files), and 
checking the current domain as you did.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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 */

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Jeff Xu]

> >>
> >> 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 ?

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Jeff Xu]

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).

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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 ?

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Jeff Xu]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Jeff Xu]

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

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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.

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Casey Schaufler]

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

Re: [PATCH v2] lsm: adds process attribute getter for Landlock

[Mickaël Salaün]

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.