| Message ID | 20230302185257.850681-1-enlightened@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | process attribute support for Landlock | expand |
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
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
> 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.
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
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.
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.
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.
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
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.
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).
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.
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.
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