| Message ID | 20220621233939.993579-1-fred@cloudflare.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce security_create_user_ns() | expand |
On 6/21/2022 4:39 PM, Frederick Lawler wrote: > While creating a LSM BPF MAC policy to block user namespace creation, we > used the LSM cred_prepare hook because that is the closest hook to prevent > a call to create_user_ns(). > > The calls look something like this: > > cred = prepare_creds() > security_prepare_creds() > call_int_hook(cred_prepare, ... > if (cred) > create_user_ns(cred) > > We noticed that error codes were not propagated from this hook and > introduced a patch [1] to propagate those errors. > > The discussion notes that security_prepare_creds() > is not appropriate for MAC policies, and instead the hook is > meant for LSM authors to prepare credentials for mutation. [2] > > Ultimately, we concluded that a better course of action is to introduce > a new security hook for LSM authors. [3] > > This patch set first introduces a new security_create_user_ns() function > and create_user_ns LSM hook, then marks the hook as sleepable in BPF. Why restrict this hook to user namespaces? It seems that an LSM that chooses to preform controls on user namespaces may want to do so for network namespaces as well. Also, the hook seems backwards. You should decide if the creation of the namespace is allowed before you create it. Passing the new namespace to a function that checks to see creating a namespace is allowed doesn't make a lot off sense. > > Links: > 1. https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/ > 2. https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/ > 3. https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/ > > Frederick Lawler (2): > security, lsm: Introduce security_create_user_ns() > bpf-lsm: Make bpf_lsm_create_user_ns() sleepable > > include/linux/lsm_hook_defs.h | 2 ++ > include/linux/lsm_hooks.h | 5 +++++ > include/linux/security.h | 8 ++++++++ > kernel/bpf/bpf_lsm.c | 1 + > kernel/user_namespace.c | 5 +++++ > security/security.c | 6 ++++++ > 6 files changed, 27 insertions(+) > > -- > 2.30.2 >
Hi Casey, On 6/21/22 7:19 PM, Casey Schaufler wrote: > On 6/21/2022 4:39 PM, Frederick Lawler wrote: >> While creating a LSM BPF MAC policy to block user namespace creation, we >> used the LSM cred_prepare hook because that is the closest hook to >> prevent >> a call to create_user_ns(). >> >> The calls look something like this: >> >> cred = prepare_creds() >> security_prepare_creds() >> call_int_hook(cred_prepare, ... >> if (cred) >> create_user_ns(cred) >> >> We noticed that error codes were not propagated from this hook and >> introduced a patch [1] to propagate those errors. >> >> The discussion notes that security_prepare_creds() >> is not appropriate for MAC policies, and instead the hook is >> meant for LSM authors to prepare credentials for mutation. [2] >> >> Ultimately, we concluded that a better course of action is to introduce >> a new security hook for LSM authors. [3] >> >> This patch set first introduces a new security_create_user_ns() function >> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. > > Why restrict this hook to user namespaces? It seems that an LSM that > chooses to preform controls on user namespaces may want to do so for > network namespaces as well. IIRC, CLONE_NEWUSER is the only namespace flag that does not require CAP_SYS_ADMIN. There is a security use case to prevent this namespace from being created within an unprivileged environment. I'm not opposed to a more generic hook, but I don't currently have a use case to block any others. We can also say the same is true for the other namespaces: add this generic security function to these too. I'm curious what others think about this too. > Also, the hook seems backwards. You should > decide if the creation of the namespace is allowed before you create it. > Passing the new namespace to a function that checks to see creating a > namespace is allowed doesn't make a lot off sense. > I think having more context to a security hook is a good thing. I believe you brought up in the previous discussions that you'd like to use this hook for xattr purposes. Doesn't that require a namespace? >> >> Links: >> 1. >> https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/ >> 2. >> https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/ >> >> 3. >> https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/ >> >> >> Frederick Lawler (2): >> security, lsm: Introduce security_create_user_ns() >> bpf-lsm: Make bpf_lsm_create_user_ns() sleepable >> >> include/linux/lsm_hook_defs.h | 2 ++ >> include/linux/lsm_hooks.h | 5 +++++ >> include/linux/security.h | 8 ++++++++ >> kernel/bpf/bpf_lsm.c | 1 + >> kernel/user_namespace.c | 5 +++++ >> security/security.c | 6 ++++++ >> 6 files changed, 27 insertions(+) >> >> -- >> 2.30.2 >>
On 6/22/2022 7:24 AM, Frederick Lawler wrote: > Hi Casey, > > On 6/21/22 7:19 PM, Casey Schaufler wrote: >> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>> While creating a LSM BPF MAC policy to block user namespace creation, we >>> used the LSM cred_prepare hook because that is the closest hook to prevent >>> a call to create_user_ns(). >>> >>> The calls look something like this: >>> >>> cred = prepare_creds() >>> security_prepare_creds() >>> call_int_hook(cred_prepare, ... >>> if (cred) >>> create_user_ns(cred) >>> >>> We noticed that error codes were not propagated from this hook and >>> introduced a patch [1] to propagate those errors. >>> >>> The discussion notes that security_prepare_creds() >>> is not appropriate for MAC policies, and instead the hook is >>> meant for LSM authors to prepare credentials for mutation. [2] >>> >>> Ultimately, we concluded that a better course of action is to introduce >>> a new security hook for LSM authors. [3] >>> >>> This patch set first introduces a new security_create_user_ns() function >>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >> >> Why restrict this hook to user namespaces? It seems that an LSM that >> chooses to preform controls on user namespaces may want to do so for >> network namespaces as well. > IIRC, CLONE_NEWUSER is the only namespace flag that does not require CAP_SYS_ADMIN. LSM hooks are (or should be) orthogonal to capabilities, except for where they are required to implement capabilities. > There is a security use case to prevent this namespace from being created within an unprivileged environment. Yes, which is why some people argued against allowing unprivileged creation of user namespaces. > I'm not opposed to a more generic hook, but I don't currently have a use case to block any others. We can also say the same is true for the other namespaces: add this generic security function to these too. If the only reason to have the hook is to disallow unprivileged user namespaces it's probably time to revise the decision to always allow them. Make it a build or runtime option. That would address the issue more directly than creating a security module. > > I'm curious what others think about this too. > > >> Also, the hook seems backwards. You should >> decide if the creation of the namespace is allowed before you create it. >> Passing the new namespace to a function that checks to see creating a >> namespace is allowed doesn't make a lot off sense. >> > > I think having more context to a security hook is a good thing. I believe you brought up in the previous discussions that you'd like to use this hook for xattr purposes. Doesn't that require a namespace? I'm not saying the information isn't required. But if you create a new namespace and then decide the user isn't allowed to create a namespace you have to tear it down. That's ugly. Better to pass the creation parameters to the hook before creating the namespace. The relationship between xattrs and namespaces is it's own can of worms. > >>> >>> Links: >>> 1. https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/ >>> 2. https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/ >>> 3. https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/ >>> >>> Frederick Lawler (2): >>> security, lsm: Introduce security_create_user_ns() >>> bpf-lsm: Make bpf_lsm_create_user_ns() sleepable >>> >>> include/linux/lsm_hook_defs.h | 2 ++ >>> include/linux/lsm_hooks.h | 5 +++++ >>> include/linux/security.h | 8 ++++++++ >>> kernel/bpf/bpf_lsm.c | 1 + >>> kernel/user_namespace.c | 5 +++++ >>> security/security.c | 6 ++++++ >>> 6 files changed, 27 insertions(+) >>> >>> -- >>> 2.30.2 >>> >
On 6/22/2022 7:24 AM, Frederick Lawler wrote: > Hi Casey, > > On 6/21/22 7:19 PM, Casey Schaufler wrote: >> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>> While creating a LSM BPF MAC policy to block user namespace creation, we >>> used the LSM cred_prepare hook because that is the closest hook to prevent >>> a call to create_user_ns(). >>> >>> The calls look something like this: >>> >>> cred = prepare_creds() >>> security_prepare_creds() >>> call_int_hook(cred_prepare, ... >>> if (cred) >>> create_user_ns(cred) >>> >>> We noticed that error codes were not propagated from this hook and >>> introduced a patch [1] to propagate those errors. >>> >>> The discussion notes that security_prepare_creds() >>> is not appropriate for MAC policies, and instead the hook is >>> meant for LSM authors to prepare credentials for mutation. [2] >>> >>> Ultimately, we concluded that a better course of action is to introduce >>> a new security hook for LSM authors. [3] >>> >>> This patch set first introduces a new security_create_user_ns() function >>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >> >> Why restrict this hook to user namespaces? It seems that an LSM that >> chooses to preform controls on user namespaces may want to do so for >> network namespaces as well. > IIRC, CLONE_NEWUSER is the only namespace flag that does not require CAP_SYS_ADMIN. LSM hooks are (or should be) orthogonal to capabilities, except for where they are required to implement capabilities. > There is a security use case to prevent this namespace from being created within an unprivileged environment. Yes, which is why some people argued against allowing unprivileged creation of user namespaces. > I'm not opposed to a more generic hook, but I don't currently have a use case to block any others. We can also say the same is true for the other namespaces: add this generic security function to these too. If the only reason to have the hook is to disallow unprivileged user namespaces it's probably time to revise the decision to always allow them. Make it a build or runtime option. That would address the issue more directly than creating a security module. > > I'm curious what others think about this too. > > >> Also, the hook seems backwards. You should >> decide if the creation of the namespace is allowed before you create it. >> Passing the new namespace to a function that checks to see creating a >> namespace is allowed doesn't make a lot off sense. >> > > I think having more context to a security hook is a good thing. I believe you brought up in the previous discussions that you'd like to use this hook for xattr purposes. Doesn't that require a namespace? I'm not saying the information isn't required. But if you create a new namespace and then decide the user isn't allowed to create a namespace you have to tear it down. That's ugly. Better to pass the creation parameters to the hook before creating the namespace. The relationship between xattrs and namespaces is it's own can of worms. > >>> >>> Links: >>> 1. https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/ >>> 2. https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/ >>> 3. https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/ >>> >>> Frederick Lawler (2): >>> security, lsm: Introduce security_create_user_ns() >>> bpf-lsm: Make bpf_lsm_create_user_ns() sleepable >>> >>> include/linux/lsm_hook_defs.h | 2 ++ >>> include/linux/lsm_hooks.h | 5 +++++ >>> include/linux/security.h | 8 ++++++++ >>> kernel/bpf/bpf_lsm.c | 1 + >>> kernel/user_namespace.c | 5 +++++ >>> security/security.c | 6 ++++++ >>> 6 files changed, 27 insertions(+) >>> >>> -- >>> 2.30.2 >>> >
On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: > On 6/21/22 7:19 PM, Casey Schaufler wrote: > > On 6/21/2022 4:39 PM, Frederick Lawler wrote: > >> While creating a LSM BPF MAC policy to block user namespace creation, we > >> used the LSM cred_prepare hook because that is the closest hook to > >> prevent > >> a call to create_user_ns(). > >> > >> The calls look something like this: > >> > >> cred = prepare_creds() > >> security_prepare_creds() > >> call_int_hook(cred_prepare, ... > >> if (cred) > >> create_user_ns(cred) > >> > >> We noticed that error codes were not propagated from this hook and > >> introduced a patch [1] to propagate those errors. > >> > >> The discussion notes that security_prepare_creds() > >> is not appropriate for MAC policies, and instead the hook is > >> meant for LSM authors to prepare credentials for mutation. [2] > >> > >> Ultimately, we concluded that a better course of action is to introduce > >> a new security hook for LSM authors. [3] > >> > >> This patch set first introduces a new security_create_user_ns() function > >> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. > > > > Why restrict this hook to user namespaces? It seems that an LSM that > > chooses to preform controls on user namespaces may want to do so for > > network namespaces as well. > > IIRC, CLONE_NEWUSER is the only namespace flag that does not require > CAP_SYS_ADMIN. There is a security use case to prevent this namespace > from being created within an unprivileged environment. I'm not opposed > to a more generic hook, but I don't currently have a use case to block > any others. We can also say the same is true for the other namespaces: > add this generic security function to these too. > > I'm curious what others think about this too. While user namespaces are obviously one of the more significant namespaces from a security perspective, I do think it seems reasonable that the LSMs could benefit from additional namespace creation hooks. However, I don't think we need to do all of them at once, starting with a userns hook seems okay to me. I also think that using the same LSM hook as an access control point for all of the different namespaces would be a mistake. At the very least we would need to pass a flag or some form of context to the hook to indicate which new namespace(s) are being requested and I fear that is a problem waiting to happen. That isn't to say someone couldn't mistakenly call the security_create_user_ns(...) from the mount namespace code, but I suspect that is much easier to identify as wrong than the equivalent security_create_ns(USER, ...). We also should acknowledge that while in most cases the current task's credentials are probably sufficient to make any LSM access control decisions around namespace creation, it's possible that for some namespaces we would need to pass additional, namespace specific info to the LSM. With a shared LSM hook this could become rather awkward. > > Also, the hook seems backwards. You should > > decide if the creation of the namespace is allowed before you create it. > > Passing the new namespace to a function that checks to see creating a > > namespace is allowed doesn't make a lot off sense. > > I think having more context to a security hook is a good thing. This is one of the reasons why I usually like to see at least one LSM implementation to go along with every new/modified hook. The implementation forces you to think about what information is necessary to perform a basic access control decision; sometimes it isn't always obvious until you have to write the access control :) [aside: If you would like to explore the SELinux implementation let me know, I'm happy to work with you on this. I suspect Casey and the other LSM maintainers would also be willing to do the same for their LSMs.] In this particular case I think the calling task's credentials are generally all that is needed. You mention that the newly created namespace would be helpful, so I'll ask: what info in the new ns do you believe would be helpful in making an access decision about its creation? Once we've sorted that we can make a better decision about the hook placement, but right now my gut feeling is that we only need to pass the task's creds, and I think placing the hook right after the UID/GID mapping check (before the new ns allocation) would be the best spot.
On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: > > On 6/21/22 7:19 PM, Casey Schaufler wrote: > > > On 6/21/2022 4:39 PM, Frederick Lawler wrote: > > >> While creating a LSM BPF MAC policy to block user namespace creation, we > > >> used the LSM cred_prepare hook because that is the closest hook to > > >> prevent > > >> a call to create_user_ns(). > > >> > > >> The calls look something like this: > > >> > > >> cred = prepare_creds() > > >> security_prepare_creds() > > >> call_int_hook(cred_prepare, ... > > >> if (cred) > > >> create_user_ns(cred) > > >> > > >> We noticed that error codes were not propagated from this hook and > > >> introduced a patch [1] to propagate those errors. > > >> > > >> The discussion notes that security_prepare_creds() > > >> is not appropriate for MAC policies, and instead the hook is > > >> meant for LSM authors to prepare credentials for mutation. [2] > > >> > > >> Ultimately, we concluded that a better course of action is to introduce > > >> a new security hook for LSM authors. [3] > > >> > > >> This patch set first introduces a new security_create_user_ns() function > > >> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. > > > > > > Why restrict this hook to user namespaces? It seems that an LSM that > > > chooses to preform controls on user namespaces may want to do so for > > > network namespaces as well. > > > > IIRC, CLONE_NEWUSER is the only namespace flag that does not require > > CAP_SYS_ADMIN. There is a security use case to prevent this namespace > > from being created within an unprivileged environment. I'm not opposed > > to a more generic hook, but I don't currently have a use case to block > > any others. We can also say the same is true for the other namespaces: > > add this generic security function to these too. > > > > I'm curious what others think about this too. > > While user namespaces are obviously one of the more significant > namespaces from a security perspective, I do think it seems reasonable > that the LSMs could benefit from additional namespace creation hooks. > However, I don't think we need to do all of them at once, starting > with a userns hook seems okay to me. > > I also think that using the same LSM hook as an access control point > for all of the different namespaces would be a mistake. At the very Agreed. > least we would need to pass a flag or some form of context to the hook > to indicate which new namespace(s) are being requested and I fear that > is a problem waiting to happen. That isn't to say someone couldn't > mistakenly call the security_create_user_ns(...) from the mount > namespace code, but I suspect that is much easier to identify as wrong > than the equivalent security_create_ns(USER, ...). Yeah, I think that's a pretty unlikely scenario. > > We also should acknowledge that while in most cases the current task's > credentials are probably sufficient to make any LSM access control > decisions around namespace creation, it's possible that for some > namespaces we would need to pass additional, namespace specific info > to the LSM. With a shared LSM hook this could become rather awkward. Agreed. > > > > Also, the hook seems backwards. You should > > > decide if the creation of the namespace is allowed before you create it. > > > Passing the new namespace to a function that checks to see creating a > > > namespace is allowed doesn't make a lot off sense. > > > > I think having more context to a security hook is a good thing. > > This is one of the reasons why I usually like to see at least one LSM > implementation to go along with every new/modified hook. The > implementation forces you to think about what information is necessary > to perform a basic access control decision; sometimes it isn't always > obvious until you have to write the access control :) I spoke to Frederick at length during LSS and as I've been given to understand there's a eBPF program that would immediately use this new hook. Now I don't want to get into the whole "Is the eBPF LSM hook infrastructure an LSM" but I think we can let this count as a legitimate first user of this hook/code. > > [aside: If you would like to explore the SELinux implementation let me > know, I'm happy to work with you on this. I suspect Casey and the > other LSM maintainers would also be willing to do the same for their > LSMs.] > > In this particular case I think the calling task's credentials are > generally all that is needed. You mention that the newly created Agreed. > namespace would be helpful, so I'll ask: what info in the new ns do > you believe would be helpful in making an access decision about its > creation? > > Once we've sorted that we can make a better decision about the hook > placement, but right now my gut feeling is that we only need to pass > the task's creds, and I think placing the hook right after the UID/GID > mapping check (before the new ns allocation) would be the best spot. When I toyed with this I placed it directly into create_user_ns() and only relied on the calling task's cred. I just created an eBPF program that verifies the caller is capable(CAP_SYS_ADMIN). Since both the chrooted and mapping check return EPERM it doesn't really matter that much where exactly. Conceptually it makes more sense to me to place it after the mapping check because then all the preliminaries are done. Christian
On 6/27/22 7:11 AM, Christian Brauner wrote: > On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >> On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: >>> On 6/21/22 7:19 PM, Casey Schaufler wrote: >>>> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>>>> While creating a LSM BPF MAC policy to block user namespace creation, we >>>>> used the LSM cred_prepare hook because that is the closest hook to >>>>> prevent >>>>> a call to create_user_ns(). >>>>> >>>>> The calls look something like this: >>>>> >>>>> cred = prepare_creds() >>>>> security_prepare_creds() >>>>> call_int_hook(cred_prepare, ... >>>>> if (cred) >>>>> create_user_ns(cred) >>>>> >>>>> We noticed that error codes were not propagated from this hook and >>>>> introduced a patch [1] to propagate those errors. >>>>> >>>>> The discussion notes that security_prepare_creds() >>>>> is not appropriate for MAC policies, and instead the hook is >>>>> meant for LSM authors to prepare credentials for mutation. [2] >>>>> >>>>> Ultimately, we concluded that a better course of action is to introduce >>>>> a new security hook for LSM authors. [3] >>>>> >>>>> This patch set first introduces a new security_create_user_ns() function >>>>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >>>> >>>> Why restrict this hook to user namespaces? It seems that an LSM that >>>> chooses to preform controls on user namespaces may want to do so for >>>> network namespaces as well. >>> >>> IIRC, CLONE_NEWUSER is the only namespace flag that does not require >>> CAP_SYS_ADMIN. There is a security use case to prevent this namespace >>> from being created within an unprivileged environment. I'm not opposed >>> to a more generic hook, but I don't currently have a use case to block >>> any others. We can also say the same is true for the other namespaces: >>> add this generic security function to these too. >>> >>> I'm curious what others think about this too. >> >> While user namespaces are obviously one of the more significant >> namespaces from a security perspective, I do think it seems reasonable >> that the LSMs could benefit from additional namespace creation hooks. >> However, I don't think we need to do all of them at once, starting >> with a userns hook seems okay to me. >> >> I also think that using the same LSM hook as an access control point >> for all of the different namespaces would be a mistake. At the very > > Agreed. > >> least we would need to pass a flag or some form of context to the hook >> to indicate which new namespace(s) are being requested and I fear that >> is a problem waiting to happen. That isn't to say someone couldn't >> mistakenly call the security_create_user_ns(...) from the mount >> namespace code, but I suspect that is much easier to identify as wrong >> than the equivalent security_create_ns(USER, ...). > > Yeah, I think that's a pretty unlikely scenario. > >> >> We also should acknowledge that while in most cases the current task's >> credentials are probably sufficient to make any LSM access control >> decisions around namespace creation, it's possible that for some >> namespaces we would need to pass additional, namespace specific info >> to the LSM. With a shared LSM hook this could become rather awkward. > > Agreed. > >> >>>> Also, the hook seems backwards. You should >>>> decide if the creation of the namespace is allowed before you create it. >>>> Passing the new namespace to a function that checks to see creating a >>>> namespace is allowed doesn't make a lot off sense. >>> >>> I think having more context to a security hook is a good thing. >> >> This is one of the reasons why I usually like to see at least one LSM >> implementation to go along with every new/modified hook. The >> implementation forces you to think about what information is necessary >> to perform a basic access control decision; sometimes it isn't always >> obvious until you have to write the access control :) > > I spoke to Frederick at length during LSS and as I've been given to > understand there's a eBPF program that would immediately use this new > hook. Now I don't want to get into the whole "Is the eBPF LSM hook > infrastructure an LSM" but I think we can let this count as a legitimate > first user of this hook/code. > >> >> [aside: If you would like to explore the SELinux implementation let me >> know, I'm happy to work with you on this. I suspect Casey and the >> other LSM maintainers would also be willing to do the same for their >> LSMs.] >> I can take a shot at making a SELinux implementation, but the question becomes: is that for v2 or a later patch? I don't think the implementation for SELinux would be too complicated (i.e. make a call to avc_has_perm()?) but, testing and revisions might take a bit longer. >> In this particular case I think the calling task's credentials are >> generally all that is needed. You mention that the newly created > > Agreed. > >> namespace would be helpful, so I'll ask: what info in the new ns do >> you believe would be helpful in making an access decision about its >> creation? >> In the other thread [1], there was mention of xattr mapping support. As I understand Caseys response to this thread [2], that feature is no longer requested for this hook. Users can still access the older parent ns from the passed in cred, but I was thinking of handling the transition point here. There's probably more suitable hooks for that case. >> Once we've sorted that we can make a better decision about the hook >> placement, but right now my gut feeling is that we only need to pass >> the task's creds, and I think placing the hook right after the UID/GID >> mapping check (before the new ns allocation) would be the best spot. > I don't specifically have a use case to pass the new user namespace for this hook at this time. I'll move the hook in v2. > When I toyed with this I placed it directly into create_user_ns() and > only relied on the calling task's cred. I just created an eBPF program > that verifies the caller is capable(CAP_SYS_ADMIN). Since both the > chrooted and mapping check return EPERM it doesn't really matter that > much where exactly. Conceptually it makes more sense to me to place it > after the mapping check because then all the preliminaries are done. > Agreed. > Christian Links: 1. https://lore.kernel.org/all/4ae12ee6-959c-51cb-9d7a-54adb3a0ea53@schaufler-ca.com/ 2. https://lore.kernel.org/all/4b62f0c5-9f3c-e0bc-d836-1b7cdea429da@schaufler-ca.com/
On Mon, Jun 27, 2022 at 10:51:48AM -0500, Frederick Lawler wrote: > On 6/27/22 7:11 AM, Christian Brauner wrote: > > On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > > > On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: > > > > On 6/21/22 7:19 PM, Casey Schaufler wrote: > > > > > On 6/21/2022 4:39 PM, Frederick Lawler wrote: > > > > > > While creating a LSM BPF MAC policy to block user namespace creation, we > > > > > > used the LSM cred_prepare hook because that is the closest hook to > > > > > > prevent > > > > > > a call to create_user_ns(). > > > > > > > > > > > > The calls look something like this: > > > > > > > > > > > > cred = prepare_creds() > > > > > > security_prepare_creds() > > > > > > call_int_hook(cred_prepare, ... > > > > > > if (cred) > > > > > > create_user_ns(cred) > > > > > > > > > > > > We noticed that error codes were not propagated from this hook and > > > > > > introduced a patch [1] to propagate those errors. > > > > > > > > > > > > The discussion notes that security_prepare_creds() > > > > > > is not appropriate for MAC policies, and instead the hook is > > > > > > meant for LSM authors to prepare credentials for mutation. [2] > > > > > > > > > > > > Ultimately, we concluded that a better course of action is to introduce > > > > > > a new security hook for LSM authors. [3] > > > > > > > > > > > > This patch set first introduces a new security_create_user_ns() function > > > > > > and create_user_ns LSM hook, then marks the hook as sleepable in BPF. > > > > > > > > > > Why restrict this hook to user namespaces? It seems that an LSM that > > > > > chooses to preform controls on user namespaces may want to do so for > > > > > network namespaces as well. > > > > > > > > IIRC, CLONE_NEWUSER is the only namespace flag that does not require > > > > CAP_SYS_ADMIN. There is a security use case to prevent this namespace > > > > from being created within an unprivileged environment. I'm not opposed > > > > to a more generic hook, but I don't currently have a use case to block > > > > any others. We can also say the same is true for the other namespaces: > > > > add this generic security function to these too. > > > > > > > > I'm curious what others think about this too. > > > > > > While user namespaces are obviously one of the more significant > > > namespaces from a security perspective, I do think it seems reasonable > > > that the LSMs could benefit from additional namespace creation hooks. > > > However, I don't think we need to do all of them at once, starting > > > with a userns hook seems okay to me. > > > > > > I also think that using the same LSM hook as an access control point > > > for all of the different namespaces would be a mistake. At the very > > > > Agreed. > > > > least we would need to pass a flag or some form of context to the hook > > > to indicate which new namespace(s) are being requested and I fear that > > > is a problem waiting to happen. That isn't to say someone couldn't > > > mistakenly call the security_create_user_ns(...) from the mount > > > namespace code, but I suspect that is much easier to identify as wrong > > > than the equivalent security_create_ns(USER, ...). > > > > Yeah, I think that's a pretty unlikely scenario. > > > > > > > > We also should acknowledge that while in most cases the current task's > > > credentials are probably sufficient to make any LSM access control > > > decisions around namespace creation, it's possible that for some > > > namespaces we would need to pass additional, namespace specific info > > > to the LSM. With a shared LSM hook this could become rather awkward. > > > > Agreed. > > > > > > > > > > Also, the hook seems backwards. You should > > > > > decide if the creation of the namespace is allowed before you create it. > > > > > Passing the new namespace to a function that checks to see creating a > > > > > namespace is allowed doesn't make a lot off sense. > > > > > > > > I think having more context to a security hook is a good thing. > > > > > > This is one of the reasons why I usually like to see at least one LSM > > > implementation to go along with every new/modified hook. The > > > implementation forces you to think about what information is necessary > > > to perform a basic access control decision; sometimes it isn't always > > > obvious until you have to write the access control :) > > > > I spoke to Frederick at length during LSS and as I've been given to > > understand there's a eBPF program that would immediately use this new > > hook. Now I don't want to get into the whole "Is the eBPF LSM hook > > infrastructure an LSM" but I think we can let this count as a legitimate > > first user of this hook/code. > > > > > > > > [aside: If you would like to explore the SELinux implementation let me > > > know, I'm happy to work with you on this. I suspect Casey and the > > > other LSM maintainers would also be willing to do the same for their > > > LSMs.] > > > > > I can take a shot at making a SELinux implementation, but the question > becomes: is that for v2 or a later patch? I don't think the implementation > for SELinux would be too complicated (i.e. make a call to avc_has_perm()?) > but, testing and revisions might take a bit longer. > > > > In this particular case I think the calling task's credentials are > > > generally all that is needed. You mention that the newly created > > > > Agreed. > > > > > namespace would be helpful, so I'll ask: what info in the new ns do > > > you believe would be helpful in making an access decision about its > > > creation? > > > > > In the other thread [1], there was mention of xattr mapping support. As I > understand Caseys response to this thread [2], that feature is no longer > requested for this hook. I think that is an orthogonal problem at least wrt to this hook. > > Users can still access the older parent ns from the passed in cred, but I > was thinking of handling the transition point here. There's probably more > suitable hooks for that case. Yes.
On 6/27/2022 8:56 AM, Christian Brauner wrote: > On Mon, Jun 27, 2022 at 10:51:48AM -0500, Frederick Lawler wrote: >> On 6/27/22 7:11 AM, Christian Brauner wrote: >>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >>>> On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: >>>>> On 6/21/22 7:19 PM, Casey Schaufler wrote: >>>>>> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>>>>>> While creating a LSM BPF MAC policy to block user namespace creation, we >>>>>>> used the LSM cred_prepare hook because that is the closest hook to >>>>>>> prevent >>>>>>> a call to create_user_ns(). >>>>>>> >>>>>>> The calls look something like this: >>>>>>> >>>>>>> cred = prepare_creds() >>>>>>> security_prepare_creds() >>>>>>> call_int_hook(cred_prepare, ... >>>>>>> if (cred) >>>>>>> create_user_ns(cred) >>>>>>> >>>>>>> We noticed that error codes were not propagated from this hook and >>>>>>> introduced a patch [1] to propagate those errors. >>>>>>> >>>>>>> The discussion notes that security_prepare_creds() >>>>>>> is not appropriate for MAC policies, and instead the hook is >>>>>>> meant for LSM authors to prepare credentials for mutation. [2] >>>>>>> >>>>>>> Ultimately, we concluded that a better course of action is to introduce >>>>>>> a new security hook for LSM authors. [3] >>>>>>> >>>>>>> This patch set first introduces a new security_create_user_ns() function >>>>>>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >>>>>> Why restrict this hook to user namespaces? It seems that an LSM that >>>>>> chooses to preform controls on user namespaces may want to do so for >>>>>> network namespaces as well. >>>>> IIRC, CLONE_NEWUSER is the only namespace flag that does not require >>>>> CAP_SYS_ADMIN. There is a security use case to prevent this namespace >>>>> from being created within an unprivileged environment. I'm not opposed >>>>> to a more generic hook, but I don't currently have a use case to block >>>>> any others. We can also say the same is true for the other namespaces: >>>>> add this generic security function to these too. >>>>> >>>>> I'm curious what others think about this too. >>>> While user namespaces are obviously one of the more significant >>>> namespaces from a security perspective, I do think it seems reasonable >>>> that the LSMs could benefit from additional namespace creation hooks. >>>> However, I don't think we need to do all of them at once, starting >>>> with a userns hook seems okay to me. >>>> >>>> I also think that using the same LSM hook as an access control point >>>> for all of the different namespaces would be a mistake. At the very >>> Agreed. > >>>> least we would need to pass a flag or some form of context to the hook >>>> to indicate which new namespace(s) are being requested and I fear that >>>> is a problem waiting to happen. That isn't to say someone couldn't >>>> mistakenly call the security_create_user_ns(...) from the mount >>>> namespace code, but I suspect that is much easier to identify as wrong >>>> than the equivalent security_create_ns(USER, ...). >>> Yeah, I think that's a pretty unlikely scenario. >>> >>>> We also should acknowledge that while in most cases the current task's >>>> credentials are probably sufficient to make any LSM access control >>>> decisions around namespace creation, it's possible that for some >>>> namespaces we would need to pass additional, namespace specific info >>>> to the LSM. With a shared LSM hook this could become rather awkward. >>> Agreed. >>> >>>>>> Also, the hook seems backwards. You should >>>>>> decide if the creation of the namespace is allowed before you create it. >>>>>> Passing the new namespace to a function that checks to see creating a >>>>>> namespace is allowed doesn't make a lot off sense. >>>>> I think having more context to a security hook is a good thing. >>>> This is one of the reasons why I usually like to see at least one LSM >>>> implementation to go along with every new/modified hook. The >>>> implementation forces you to think about what information is necessary >>>> to perform a basic access control decision; sometimes it isn't always >>>> obvious until you have to write the access control :) >>> I spoke to Frederick at length during LSS and as I've been given to >>> understand there's a eBPF program that would immediately use this new >>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>> infrastructure an LSM" but I think we can let this count as a legitimate >>> first user of this hook/code. Yes, although it would be really helpful if there were a recognized upstream for eBPF programs so that we could see not only that the hook is used but how it is being used. It is possible (even likely) that someone will want to change either the interface or the caller some day. Without having the eBPF that depends on it, it's hard to determine if a change would be a regression. >>> >>>> [aside: If you would like to explore the SELinux implementation let me >>>> know, I'm happy to work with you on this. I suspect Casey and the >>>> other LSM maintainers would also be willing to do the same for their >>>> LSMs.] >>>> >> I can take a shot at making a SELinux implementation, but the question >> becomes: is that for v2 or a later patch? I don't think the implementation >> for SELinux would be too complicated (i.e. make a call to avc_has_perm()?) >> but, testing and revisions might take a bit longer. >> >>>> In this particular case I think the calling task's credentials are >>>> generally all that is needed. You mention that the newly created >>> Agreed. >>> >>>> namespace would be helpful, so I'll ask: what info in the new ns do >>>> you believe would be helpful in making an access decision about its >>>> creation? >>>> >> In the other thread [1], there was mention of xattr mapping support. As I >> understand Caseys response to this thread [2], that feature is no longer >> requested for this hook. > I think that is an orthogonal problem at least wrt to this hook. Agreed. It was always a look deeply into the future sort of thing. At this point I don't see anything blocking the proposed hook moving forward. > >> Users can still access the older parent ns from the passed in cred, but I >> was thinking of handling the transition point here. There's probably more >> suitable hooks for that case. > Yes.
On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: > On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: ... > > This is one of the reasons why I usually like to see at least one LSM > > implementation to go along with every new/modified hook. The > > implementation forces you to think about what information is necessary > > to perform a basic access control decision; sometimes it isn't always > > obvious until you have to write the access control :) > > I spoke to Frederick at length during LSS and as I've been given to > understand there's a eBPF program that would immediately use this new > hook. Now I don't want to get into the whole "Is the eBPF LSM hook > infrastructure an LSM" but I think we can let this count as a legitimate > first user of this hook/code. Yes, for the most part I don't really worry about the "is a BPF LSM a LSM?" question, it's generally not important for most discussions. However, there is an issue unique to the BPF LSMs which I think is relevant here: there is no hook implementation code living under security/. While I talked about a hook implementation being helpful to verify the hook prototype, it is also helpful in providing an in-tree example for other LSMs; unfortunately we don't get that same example value when the initial hook implementation is a BPF LSM.
On Mon, Jun 27, 2022 at 11:51 AM Frederick Lawler <fred@cloudflare.com> wrote: > On 6/27/22 7:11 AM, Christian Brauner wrote: > > On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > >> On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.com> wrote: > >>> On 6/21/22 7:19 PM, Casey Schaufler wrote: > >>>> On 6/21/2022 4:39 PM, Frederick Lawler wrote: ... > >>>>> While creating a LSM BPF MAC policy to block user namespace creation, we > >>>>> used the LSM cred_prepare hook because that is the closest hook to > >>>>> prevent > >>>>> a call to create_user_ns(). > >>>>> > >>>>> The calls look something like this: > >>>>> > >>>>> cred = prepare_creds() > >>>>> security_prepare_creds() > >>>>> call_int_hook(cred_prepare, ... > >>>>> if (cred) > >>>>> create_user_ns(cred) > >>>>> > >>>>> We noticed that error codes were not propagated from this hook and > >>>>> introduced a patch [1] to propagate those errors. > >>>>> > >>>>> The discussion notes that security_prepare_creds() > >>>>> is not appropriate for MAC policies, and instead the hook is > >>>>> meant for LSM authors to prepare credentials for mutation. [2] > >>>>> > >>>>> Ultimately, we concluded that a better course of action is to introduce > >>>>> a new security hook for LSM authors. [3] > >>>>> > >>>>> This patch set first introduces a new security_create_user_ns() function > >>>>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. > >>>> > >>>> Why restrict this hook to user namespaces? It seems that an LSM that > >>>> chooses to preform controls on user namespaces may want to do so for > >>>> network namespaces as well. > >>> > >>> IIRC, CLONE_NEWUSER is the only namespace flag that does not require > >>> CAP_SYS_ADMIN. There is a security use case to prevent this namespace > >>> from being created within an unprivileged environment. I'm not opposed > >>> to a more generic hook, but I don't currently have a use case to block > >>> any others. We can also say the same is true for the other namespaces: > >>> add this generic security function to these too. > >>> > >>> I'm curious what others think about this too. > >> > >> While user namespaces are obviously one of the more significant > >> namespaces from a security perspective, I do think it seems reasonable > >> that the LSMs could benefit from additional namespace creation hooks. > >> However, I don't think we need to do all of them at once, starting > >> with a userns hook seems okay to me. > >> > >> I also think that using the same LSM hook as an access control point > >> for all of the different namespaces would be a mistake. At the very > > > > Agreed. > > >> least we would need to pass a flag or some form of context to the hook > >> to indicate which new namespace(s) are being requested and I fear that > >> is a problem waiting to happen. That isn't to say someone couldn't > >> mistakenly call the security_create_user_ns(...) from the mount > >> namespace code, but I suspect that is much easier to identify as wrong > >> than the equivalent security_create_ns(USER, ...). > > > > Yeah, I think that's a pretty unlikely scenario. > > > >> > >> We also should acknowledge that while in most cases the current task's > >> credentials are probably sufficient to make any LSM access control > >> decisions around namespace creation, it's possible that for some > >> namespaces we would need to pass additional, namespace specific info > >> to the LSM. With a shared LSM hook this could become rather awkward. > > > > Agreed. > > > >> > >>>> Also, the hook seems backwards. You should > >>>> decide if the creation of the namespace is allowed before you create it. > >>>> Passing the new namespace to a function that checks to see creating a > >>>> namespace is allowed doesn't make a lot off sense. > >>> > >>> I think having more context to a security hook is a good thing. > >> > >> This is one of the reasons why I usually like to see at least one LSM > >> implementation to go along with every new/modified hook. The > >> implementation forces you to think about what information is necessary > >> to perform a basic access control decision; sometimes it isn't always > >> obvious until you have to write the access control :) > > > > I spoke to Frederick at length during LSS and as I've been given to > > understand there's a eBPF program that would immediately use this new > > hook. Now I don't want to get into the whole "Is the eBPF LSM hook > > infrastructure an LSM" but I think we can let this count as a legitimate > > first user of this hook/code. > > > >> > >> [aside: If you would like to explore the SELinux implementation let me > >> know, I'm happy to work with you on this. I suspect Casey and the > >> other LSM maintainers would also be willing to do the same for their > >> LSMs.] > >> > > I can take a shot at making a SELinux implementation, but the question > becomes: is that for v2 or a later patch? I don't think the > implementation for SELinux would be too complicated (i.e. make a call to > avc_has_perm()?) but, testing and revisions might take a bit longer. Isn't that the truth, writing code is easy(ish); testing it well is the hard part ;) Joking aside, I would suggest starting with v2. As an example, the code below might be a good place to start - we would need to discuss this on the SELinux list as there are some design decisions I'm glossing over[1]. int selinux_userns_create(struct cred *new) { u32 sid = current_sid(); return avc_has_perm(&selinux_state, sid, sid, SECCLASS_NAMESPACE, NAMESPACE__USERNS_CREATE); } You would also need to add the "namespace" class and the "userns_create" permission in security/selinux/include/classmap.h. const struct security_class_mapping secclass_map[] = { ... { "namespace", { "userns_create", NULL } }, } As I mentioned earlier, if you find yourself getting stuck, or needing some help, please feel free to send mail. [1] This code snippet uses a new object class and permission for this (namespace:userns_create). I made that choice as object classes are limited to 32 unique permissions and I expect the number of namespaces to continue to grow.
On 6/27/22 11:56 PM, Paul Moore wrote: > On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: >> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > > ... > >>> This is one of the reasons why I usually like to see at least one LSM >>> implementation to go along with every new/modified hook. The >>> implementation forces you to think about what information is necessary >>> to perform a basic access control decision; sometimes it isn't always >>> obvious until you have to write the access control :) >> >> I spoke to Frederick at length during LSS and as I've been given to >> understand there's a eBPF program that would immediately use this new >> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >> infrastructure an LSM" but I think we can let this count as a legitimate >> first user of this hook/code. > > Yes, for the most part I don't really worry about the "is a BPF LSM a > LSM?" question, it's generally not important for most discussions. > However, there is an issue unique to the BPF LSMs which I think is > relevant here: there is no hook implementation code living under > security/. While I talked about a hook implementation being helpful > to verify the hook prototype, it is also helpful in providing an > in-tree example for other LSMs; unfortunately we don't get that same > example value when the initial hook implementation is a BPF LSM. I would argue that such a patch series must come together with a BPF selftest which then i) contains an in-tree usage example, ii) adds BPF CI test coverage. Shipping with a BPF selftest at least would be the usual expectation. Thanks, Daniel
On Tue, Jun 28, 2022 at 12:15 AM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 6/27/22 11:56 PM, Paul Moore wrote: > > On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: > >> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > > > > ... > > > >>> This is one of the reasons why I usually like to see at least one LSM > >>> implementation to go along with every new/modified hook. The > >>> implementation forces you to think about what information is necessary > >>> to perform a basic access control decision; sometimes it isn't always > >>> obvious until you have to write the access control :) > >> > >> I spoke to Frederick at length during LSS and as I've been given to > >> understand there's a eBPF program that would immediately use this new > >> hook. Now I don't want to get into the whole "Is the eBPF LSM hook > >> infrastructure an LSM" but I think we can let this count as a legitimate > >> first user of this hook/code. > > > > Yes, for the most part I don't really worry about the "is a BPF LSM a > > LSM?" question, it's generally not important for most discussions. > > However, there is an issue unique to the BPF LSMs which I think is > > relevant here: there is no hook implementation code living under > > security/. While I talked about a hook implementation being helpful > > to verify the hook prototype, it is also helpful in providing an > > in-tree example for other LSMs; unfortunately we don't get that same > > example value when the initial hook implementation is a BPF LSM. > > I would argue that such a patch series must come together with a BPF > selftest which then i) contains an in-tree usage example, ii) adds BPF > CI test coverage. Shipping with a BPF selftest at least would be the > usual expectation. +1 I would also recommend that this comes with a BPF selftest as suggested by Daniel. > > Thanks, > Daniel
On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > On 6/27/22 11:56 PM, Paul Moore wrote: > > On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: > >> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > > > > ... > > > >>> This is one of the reasons why I usually like to see at least one LSM > >>> implementation to go along with every new/modified hook. The > >>> implementation forces you to think about what information is necessary > >>> to perform a basic access control decision; sometimes it isn't always > >>> obvious until you have to write the access control :) > >> > >> I spoke to Frederick at length during LSS and as I've been given to > >> understand there's a eBPF program that would immediately use this new > >> hook. Now I don't want to get into the whole "Is the eBPF LSM hook > >> infrastructure an LSM" but I think we can let this count as a legitimate > >> first user of this hook/code. > > > > Yes, for the most part I don't really worry about the "is a BPF LSM a > > LSM?" question, it's generally not important for most discussions. > > However, there is an issue unique to the BPF LSMs which I think is > > relevant here: there is no hook implementation code living under > > security/. While I talked about a hook implementation being helpful > > to verify the hook prototype, it is also helpful in providing an > > in-tree example for other LSMs; unfortunately we don't get that same > > example value when the initial hook implementation is a BPF LSM. > > I would argue that such a patch series must come together with a BPF > selftest which then i) contains an in-tree usage example, ii) adds BPF > CI test coverage. Shipping with a BPF selftest at least would be the > usual expectation. I'm not going to disagree with that, I generally require matching tests for new SELinux kernel code, but I was careful to mention code under 'security/' and not necessarily just a test implementation :) I don't want to get into a big discussion about it, but I think having a working implementation somewhere under 'security/' is more discoverable for most LSM folks.
On 6/27/2022 3:27 PM, Paul Moore wrote: > On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> On 6/27/22 11:56 PM, Paul Moore wrote: >>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: >>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >>> ... >>> >>>>> This is one of the reasons why I usually like to see at least one LSM >>>>> implementation to go along with every new/modified hook. The >>>>> implementation forces you to think about what information is necessary >>>>> to perform a basic access control decision; sometimes it isn't always >>>>> obvious until you have to write the access control :) >>>> I spoke to Frederick at length during LSS and as I've been given to >>>> understand there's a eBPF program that would immediately use this new >>>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>>> infrastructure an LSM" but I think we can let this count as a legitimate >>>> first user of this hook/code. >>> Yes, for the most part I don't really worry about the "is a BPF LSM a >>> LSM?" question, it's generally not important for most discussions. >>> However, there is an issue unique to the BPF LSMs which I think is >>> relevant here: there is no hook implementation code living under >>> security/. While I talked about a hook implementation being helpful >>> to verify the hook prototype, it is also helpful in providing an >>> in-tree example for other LSMs; unfortunately we don't get that same >>> example value when the initial hook implementation is a BPF LSM. >> I would argue that such a patch series must come together with a BPF >> selftest which then i) contains an in-tree usage example, ii) adds BPF >> CI test coverage. Shipping with a BPF selftest at least would be the >> usual expectation. > I'm not going to disagree with that, I generally require matching > tests for new SELinux kernel code, but I was careful to mention code > under 'security/' and not necessarily just a test implementation :) I > don't want to get into a big discussion about it, but I think having a > working implementation somewhere under 'security/' is more > discoverable for most LSM folks. I agree. It would be unfortunate if we added a hook explicitly for eBPF only to discover that the proposed user needs something different. The LSM community should have a chance to review the code before committing to all the maintenance required in supporting it. Is there a reference on how to write an eBPF security module? There should be something out there warning the eBPF programmer of the implications of providing a secid_to_secctx hook for starters.
On 6/27/22 5:15 PM, Daniel Borkmann wrote: > On 6/27/22 11:56 PM, Paul Moore wrote: >> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> >> wrote: >>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >> >> ... >> >>>> This is one of the reasons why I usually like to see at least one LSM >>>> implementation to go along with every new/modified hook. The >>>> implementation forces you to think about what information is necessary >>>> to perform a basic access control decision; sometimes it isn't always >>>> obvious until you have to write the access control :) >>> >>> I spoke to Frederick at length during LSS and as I've been given to >>> understand there's a eBPF program that would immediately use this new >>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>> infrastructure an LSM" but I think we can let this count as a legitimate >>> first user of this hook/code. >> >> Yes, for the most part I don't really worry about the "is a BPF LSM a >> LSM?" question, it's generally not important for most discussions. >> However, there is an issue unique to the BPF LSMs which I think is >> relevant here: there is no hook implementation code living under >> security/. While I talked about a hook implementation being helpful >> to verify the hook prototype, it is also helpful in providing an >> in-tree example for other LSMs; unfortunately we don't get that same >> example value when the initial hook implementation is a BPF LSM. > > I would argue that such a patch series must come together with a BPF > selftest which then i) contains an in-tree usage example, ii) adds BPF > CI test coverage. Shipping with a BPF selftest at least would be the > usual expectation. Sounds good. I'll add both a eBPF selftest and SELinux implementation for v2. > > Thanks, > Daniel
On Tue, Jun 28, 2022 at 11:11 AM Frederick Lawler <fred@cloudflare.com> wrote: > On 6/27/22 5:15 PM, Daniel Borkmann wrote: > > On 6/27/22 11:56 PM, Paul Moore wrote: > >> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> > >> wrote: > >>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > >> > >> ... > >> > >>>> This is one of the reasons why I usually like to see at least one LSM > >>>> implementation to go along with every new/modified hook. The > >>>> implementation forces you to think about what information is necessary > >>>> to perform a basic access control decision; sometimes it isn't always > >>>> obvious until you have to write the access control :) > >>> > >>> I spoke to Frederick at length during LSS and as I've been given to > >>> understand there's a eBPF program that would immediately use this new > >>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook > >>> infrastructure an LSM" but I think we can let this count as a legitimate > >>> first user of this hook/code. > >> > >> Yes, for the most part I don't really worry about the "is a BPF LSM a > >> LSM?" question, it's generally not important for most discussions. > >> However, there is an issue unique to the BPF LSMs which I think is > >> relevant here: there is no hook implementation code living under > >> security/. While I talked about a hook implementation being helpful > >> to verify the hook prototype, it is also helpful in providing an > >> in-tree example for other LSMs; unfortunately we don't get that same > >> example value when the initial hook implementation is a BPF LSM. > > > > I would argue that such a patch series must come together with a BPF > > selftest which then i) contains an in-tree usage example, ii) adds BPF > > CI test coverage. Shipping with a BPF selftest at least would be the > > usual expectation. > > Sounds good. I'll add both a eBPF selftest and SELinux implementation > for v2. Thanks Daniel!
On 6/27/22 6:18 PM, Casey Schaufler wrote: > On 6/27/2022 3:27 PM, Paul Moore wrote: >> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> >> wrote: >>> On 6/27/22 11:56 PM, Paul Moore wrote: >>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner >>>> <brauner@kernel.org> wrote: >>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >>>> ... >>>> >>>>>> This is one of the reasons why I usually like to see at least one LSM >>>>>> implementation to go along with every new/modified hook. The >>>>>> implementation forces you to think about what information is >>>>>> necessary >>>>>> to perform a basic access control decision; sometimes it isn't always >>>>>> obvious until you have to write the access control :) >>>>> I spoke to Frederick at length during LSS and as I've been given to >>>>> understand there's a eBPF program that would immediately use this new >>>>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>>>> infrastructure an LSM" but I think we can let this count as a >>>>> legitimate >>>>> first user of this hook/code. >>>> Yes, for the most part I don't really worry about the "is a BPF LSM a >>>> LSM?" question, it's generally not important for most discussions. >>>> However, there is an issue unique to the BPF LSMs which I think is >>>> relevant here: there is no hook implementation code living under >>>> security/. While I talked about a hook implementation being helpful >>>> to verify the hook prototype, it is also helpful in providing an >>>> in-tree example for other LSMs; unfortunately we don't get that same >>>> example value when the initial hook implementation is a BPF LSM. >>> I would argue that such a patch series must come together with a BPF >>> selftest which then i) contains an in-tree usage example, ii) adds BPF >>> CI test coverage. Shipping with a BPF selftest at least would be the >>> usual expectation. >> I'm not going to disagree with that, I generally require matching >> tests for new SELinux kernel code, but I was careful to mention code >> under 'security/' and not necessarily just a test implementation :) I >> don't want to get into a big discussion about it, but I think having a >> working implementation somewhere under 'security/' is more >> discoverable for most LSM folks. > > I agree. It would be unfortunate if we added a hook explicitly for eBPF > only to discover that the proposed user needs something different. The > LSM community should have a chance to review the code before committing > to all the maintenance required in supporting it. > > Is there a reference on how to write an eBPF security module? There's a documentation page that briefly touches on a BPF LSM implementation [1]. > There should be something out there warning the eBPF programmer of the > implications of providing a secid_to_secctx hook for starters. > Links: 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm#
On 6/28/2022 8:14 AM, Frederick Lawler wrote: > On 6/27/22 6:18 PM, Casey Schaufler wrote: >> On 6/27/2022 3:27 PM, Paul Moore wrote: >>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>> On 6/27/22 11:56 PM, Paul Moore wrote: >>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: >>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >>>>> ... >>>>> >>>>>>> This is one of the reasons why I usually like to see at least one LSM >>>>>>> implementation to go along with every new/modified hook. The >>>>>>> implementation forces you to think about what information is necessary >>>>>>> to perform a basic access control decision; sometimes it isn't always >>>>>>> obvious until you have to write the access control :) >>>>>> I spoke to Frederick at length during LSS and as I've been given to >>>>>> understand there's a eBPF program that would immediately use this new >>>>>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>>>>> infrastructure an LSM" but I think we can let this count as a legitimate >>>>>> first user of this hook/code. >>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a >>>>> LSM?" question, it's generally not important for most discussions. >>>>> However, there is an issue unique to the BPF LSMs which I think is >>>>> relevant here: there is no hook implementation code living under >>>>> security/. While I talked about a hook implementation being helpful >>>>> to verify the hook prototype, it is also helpful in providing an >>>>> in-tree example for other LSMs; unfortunately we don't get that same >>>>> example value when the initial hook implementation is a BPF LSM. >>>> I would argue that such a patch series must come together with a BPF >>>> selftest which then i) contains an in-tree usage example, ii) adds BPF >>>> CI test coverage. Shipping with a BPF selftest at least would be the >>>> usual expectation. >>> I'm not going to disagree with that, I generally require matching >>> tests for new SELinux kernel code, but I was careful to mention code >>> under 'security/' and not necessarily just a test implementation :) I >>> don't want to get into a big discussion about it, but I think having a >>> working implementation somewhere under 'security/' is more >>> discoverable for most LSM folks. >> >> I agree. It would be unfortunate if we added a hook explicitly for eBPF >> only to discover that the proposed user needs something different. The >> LSM community should have a chance to review the code before committing >> to all the maintenance required in supporting it. >> >> Is there a reference on how to write an eBPF security module? > > There's a documentation page that briefly touches on a BPF LSM implementation [1]. That's a brief touch, alright. I'll grant that the LSM interface isn't especially well documented for C developers, but we have done tutorials and have multiple examples. I worry that without an in-tree example for eBPF we might well be setting developers up for spectacular failure. > >> There should be something out there warning the eBPF programmer of the >> implications of providing a secid_to_secctx hook for starters. >> > > Links: > 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm# >
On Tue, Jun 28, 2022 at 6:02 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 6/28/2022 8:14 AM, Frederick Lawler wrote: > > On 6/27/22 6:18 PM, Casey Schaufler wrote: > >> On 6/27/2022 3:27 PM, Paul Moore wrote: > >>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > >>>> On 6/27/22 11:56 PM, Paul Moore wrote: > >>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: > >>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: > >>>>> ... > >>>>> > >>>>>>> This is one of the reasons why I usually like to see at least one LSM > >>>>>>> implementation to go along with every new/modified hook. The > >>>>>>> implementation forces you to think about what information is necessary > >>>>>>> to perform a basic access control decision; sometimes it isn't always > >>>>>>> obvious until you have to write the access control :) > >>>>>> I spoke to Frederick at length during LSS and as I've been given to > >>>>>> understand there's a eBPF program that would immediately use this new > >>>>>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook > >>>>>> infrastructure an LSM" but I think we can let this count as a legitimate > >>>>>> first user of this hook/code. > >>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a > >>>>> LSM?" question, it's generally not important for most discussions. > >>>>> However, there is an issue unique to the BPF LSMs which I think is > >>>>> relevant here: there is no hook implementation code living under > >>>>> security/. While I talked about a hook implementation being helpful > >>>>> to verify the hook prototype, it is also helpful in providing an > >>>>> in-tree example for other LSMs; unfortunately we don't get that same > >>>>> example value when the initial hook implementation is a BPF LSM. > >>>> I would argue that such a patch series must come together with a BPF > >>>> selftest which then i) contains an in-tree usage example, ii) adds BPF > >>>> CI test coverage. Shipping with a BPF selftest at least would be the > >>>> usual expectation. > >>> I'm not going to disagree with that, I generally require matching > >>> tests for new SELinux kernel code, but I was careful to mention code > >>> under 'security/' and not necessarily just a test implementation :) I > >>> don't want to get into a big discussion about it, but I think having a > >>> working implementation somewhere under 'security/' is more > >>> discoverable for most LSM folks. > >> > >> I agree. It would be unfortunate if we added a hook explicitly for eBPF > >> only to discover that the proposed user needs something different. The > >> LSM community should have a chance to review the code before committing > >> to all the maintenance required in supporting it. > >> > >> Is there a reference on how to write an eBPF security module? > > > > There's a documentation page that briefly touches on a BPF LSM implementation [1]. > > That's a brief touch, alright. I'll grant that the LSM interface isn't > especially well documented for C developers, but we have done tutorials > and have multiple examples. I worry that without an in-tree example for > eBPF we might well be setting developers up for spectacular failure. > Casey, Daniel and I are recommending an in-tree example, it will be in BPF selftests and we will CC you on the reviews. Frederick, is that okay with you? > > > >> There should be something out there warning the eBPF programmer of the > >> implications of providing a secid_to_secctx hook for starters. > >> > > > > Links: > > 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm# > >
On 6/28/22 11:12 AM, KP Singh wrote: > On Tue, Jun 28, 2022 at 6:02 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> >> On 6/28/2022 8:14 AM, Frederick Lawler wrote: >>> On 6/27/22 6:18 PM, Casey Schaufler wrote: >>>> On 6/27/2022 3:27 PM, Paul Moore wrote: >>>>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >>>>>> On 6/27/22 11:56 PM, Paul Moore wrote: >>>>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote: >>>>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote: >>>>>>> ... >>>>>>> >>>>>>>>> This is one of the reasons why I usually like to see at least one LSM >>>>>>>>> implementation to go along with every new/modified hook. The >>>>>>>>> implementation forces you to think about what information is necessary >>>>>>>>> to perform a basic access control decision; sometimes it isn't always >>>>>>>>> obvious until you have to write the access control :) >>>>>>>> I spoke to Frederick at length during LSS and as I've been given to >>>>>>>> understand there's a eBPF program that would immediately use this new >>>>>>>> hook. Now I don't want to get into the whole "Is the eBPF LSM hook >>>>>>>> infrastructure an LSM" but I think we can let this count as a legitimate >>>>>>>> first user of this hook/code. >>>>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a >>>>>>> LSM?" question, it's generally not important for most discussions. >>>>>>> However, there is an issue unique to the BPF LSMs which I think is >>>>>>> relevant here: there is no hook implementation code living under >>>>>>> security/. While I talked about a hook implementation being helpful >>>>>>> to verify the hook prototype, it is also helpful in providing an >>>>>>> in-tree example for other LSMs; unfortunately we don't get that same >>>>>>> example value when the initial hook implementation is a BPF LSM. >>>>>> I would argue that such a patch series must come together with a BPF >>>>>> selftest which then i) contains an in-tree usage example, ii) adds BPF >>>>>> CI test coverage. Shipping with a BPF selftest at least would be the >>>>>> usual expectation. >>>>> I'm not going to disagree with that, I generally require matching >>>>> tests for new SELinux kernel code, but I was careful to mention code >>>>> under 'security/' and not necessarily just a test implementation :) I >>>>> don't want to get into a big discussion about it, but I think having a >>>>> working implementation somewhere under 'security/' is more >>>>> discoverable for most LSM folks. >>>> >>>> I agree. It would be unfortunate if we added a hook explicitly for eBPF >>>> only to discover that the proposed user needs something different. The >>>> LSM community should have a chance to review the code before committing >>>> to all the maintenance required in supporting it. >>>> >>>> Is there a reference on how to write an eBPF security module? >>> >>> There's a documentation page that briefly touches on a BPF LSM implementation [1]. >> >> That's a brief touch, alright. I'll grant that the LSM interface isn't >> especially well documented for C developers, but we have done tutorials >> and have multiple examples. I worry that without an in-tree example for >> eBPF we might well be setting developers up for spectacular failure. >> > > Casey, Daniel and I are recommending an in-tree example, it will be > in BPF selftests and we will CC you on the reviews. > > Frederick, is that okay with you? Yep. > >>> >>>> There should be something out there warning the eBPF programmer of the >>>> implications of providing a secid_to_secctx hook for starters. >>>> >>> >>> Links: >>> 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm# >>>
Frederick Lawler <fred@cloudflare.com> writes: > Hi Casey, > > On 6/21/22 7:19 PM, Casey Schaufler wrote: >> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>> While creating a LSM BPF MAC policy to block user namespace creation, we >>> used the LSM cred_prepare hook because that is the closest hook to prevent >>> a call to create_user_ns(). >>> >>> The calls look something like this: >>> >>> cred = prepare_creds() >>> security_prepare_creds() >>> call_int_hook(cred_prepare, ... >>> if (cred) >>> create_user_ns(cred) >>> >>> We noticed that error codes were not propagated from this hook and >>> introduced a patch [1] to propagate those errors. >>> >>> The discussion notes that security_prepare_creds() >>> is not appropriate for MAC policies, and instead the hook is >>> meant for LSM authors to prepare credentials for mutation. [2] >>> >>> Ultimately, we concluded that a better course of action is to introduce >>> a new security hook for LSM authors. [3] >>> >>> This patch set first introduces a new security_create_user_ns() function >>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >> Why restrict this hook to user namespaces? It seems that an LSM that >> chooses to preform controls on user namespaces may want to do so for >> network namespaces as well. > IIRC, CLONE_NEWUSER is the only namespace flag that does not require > CAP_SYS_ADMIN. There is a security use case to prevent this namespace > from being created within an unprivileged environment. I'm not opposed to a more > generic hook, but I don't currently have a use case to block any others. We can > also say the same is true for the other namespaces: add this generic security > function to these too. There is also a very strong security use case for not putting security checks in the creation of the user namespace. In particular there are all kinds of kernel features that are useful for building sandboxes namespaces, chroot, etc, that previous to user namespaces were not possible to make available to unprivileged users because they could confuse suid-root executables. With user namespaces the concern about confusing suid-root executable goes away. The only justification I have ever heard for restricting the user namespace is because it indirectly allows for greater kernel attack surface. Do you have a case for restricting the user namespace other than the kernel is buggy and the user namespace makes the kernel bugs easier to access? How does increasing the attack surface of the kernel make the situation that the kernel is buggy and the attack surface is too big better? Perhaps it is explained somewhere down-thread and I just have not caught up yet, but so far I have not see a description of why it makes sense for a security module to restrict the kernel here. Eric p.s. I am little disappointed that I was not copied on this thread given that it is my code you are messing with, and I was in an earlier version of this thread.
On 6/30/22 1:28 PM, Eric W. Biederman wrote: > Frederick Lawler <fred@cloudflare.com> writes: > >> Hi Casey, >> >> On 6/21/22 7:19 PM, Casey Schaufler wrote: >>> On 6/21/2022 4:39 PM, Frederick Lawler wrote: >>>> While creating a LSM BPF MAC policy to block user namespace creation, we >>>> used the LSM cred_prepare hook because that is the closest hook to prevent >>>> a call to create_user_ns(). >>>> >>>> The calls look something like this: >>>> >>>> cred = prepare_creds() >>>> security_prepare_creds() >>>> call_int_hook(cred_prepare, ... >>>> if (cred) >>>> create_user_ns(cred) >>>> >>>> We noticed that error codes were not propagated from this hook and >>>> introduced a patch [1] to propagate those errors. >>>> >>>> The discussion notes that security_prepare_creds() >>>> is not appropriate for MAC policies, and instead the hook is >>>> meant for LSM authors to prepare credentials for mutation. [2] >>>> >>>> Ultimately, we concluded that a better course of action is to introduce >>>> a new security hook for LSM authors. [3] >>>> >>>> This patch set first introduces a new security_create_user_ns() function >>>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF. >>> Why restrict this hook to user namespaces? It seems that an LSM that >>> chooses to preform controls on user namespaces may want to do so for >>> network namespaces as well. >> IIRC, CLONE_NEWUSER is the only namespace flag that does not require >> CAP_SYS_ADMIN. There is a security use case to prevent this namespace >> from being created within an unprivileged environment. I'm not opposed to a more >> generic hook, but I don't currently have a use case to block any others. We can >> also say the same is true for the other namespaces: add this generic security >> function to these too. > > There is also a very strong security use case for not putting security > checks in the creation of the user namespace. > > In particular there are all kinds of kernel features that are useful for > building sandboxes namespaces, chroot, etc, that previous to user > namespaces were not possible to make available to unprivileged users > because they could confuse suid-root executables. With user namespaces > the concern about confusing suid-root executable goes away. > > The only justification I have ever heard for restricting the user > namespace is because it indirectly allows for greater kernel attack > surface. > > Do you have a case for restricting the user namespace other than the > kernel is buggy and the user namespace makes the kernel bugs easier > to access? No. > > How does increasing the attack surface of the kernel make the situation > that the kernel is buggy and the attack surface is too big better? > We agree that completely blocking this feature may disable effective sandboxing techniques, but uncontrolled user namespace creation increases the attack surface as well. There are known examples where creating a new namespace is the first step in a priv escalation attack. > Perhaps it is explained somewhere down-thread and I just have not caught > up yet, but so far I have not see a description of why it makes sense > for a security module to restrict the kernel here. > Having a new security hook allows us to decide when unpriv user namespace creation is acceptable and unacceptable. Patching vulnerabilities takes time and effort amongst the community. We want to protect our systems so that we are not as impacted by the next 0-day disclosure. This in turn decreases our overall attack surface. Security hooks are a good fit because they offer us flexibility to protect our systems how we see fit. > Eric > > p.s. I am little disappointed that I was not copied on this thread > given that it is my code you are messing with, and I was in an earlier > version of this thread.
While creating a LSM BPF MAC policy to block user namespace creation, we used the LSM cred_prepare hook because that is the closest hook to prevent a call to create_user_ns(). The calls look something like this: cred = prepare_creds() security_prepare_creds() call_int_hook(cred_prepare, ... if (cred) create_user_ns(cred) We noticed that error codes were not propagated from this hook and introduced a patch [1] to propagate those errors. The discussion notes that security_prepare_creds() is not appropriate for MAC policies, and instead the hook is meant for LSM authors to prepare credentials for mutation. [2] Ultimately, we concluded that a better course of action is to introduce a new security hook for LSM authors. [3] This patch set first introduces a new security_create_user_ns() function and create_user_ns LSM hook, then marks the hook as sleepable in BPF. Links: 1. https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/ 2. https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/ 3. https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/ Frederick Lawler (2): security, lsm: Introduce security_create_user_ns() bpf-lsm: Make bpf_lsm_create_user_ns() sleepable include/linux/lsm_hook_defs.h | 2 ++ include/linux/lsm_hooks.h | 5 +++++ include/linux/security.h | 8 ++++++++ kernel/bpf/bpf_lsm.c | 1 + kernel/user_namespace.c | 5 +++++ security/security.c | 6 ++++++ 6 files changed, 27 insertions(+) -- 2.30.2