* [PATCH] namei: permit linking with CAP_FOWNER in userns
@ 2015-09-30 0:05 Dirk Steinmetz
0 siblings, 0 replies; 13+ messages in thread
From: Dirk Steinmetz @ 2015-09-30 0:05 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel
Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
within an unprivileged user namespace fails, even if CAP_FOWNER is held
within the namespace. This may cause various failures, such as a gentoo
installation within a lxc container failing to build and install specific
packages.
This change permits hardlinking of files owned by mapped uids, if
CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
by using the existing inode_owner_or_capable(), which is aware of
namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
inode_capable to capable_wrt_inode_uidgid").
Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
---
fs/namei.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 726d211..29fc6a6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -955,26 +955,23 @@ static bool safe_hardlink_source(struct inode *inode)
* - sysctl_protected_hardlinks enabled
* - fsuid does not match inode
* - hardlink source is unsafe (see safe_hardlink_source() above)
- * - not CAP_FOWNER
+ * - not CAP_FOWNER in a namespace with the inode owner uid mapped
*
* Returns 0 if successful, -ve on error.
*/
static int may_linkat(struct path *link)
{
- const struct cred *cred;
struct inode *inode;
if (!sysctl_protected_hardlinks)
return 0;
- cred = current_cred();
inode = link->dentry->d_inode;
/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
* otherwise, it must be a safe source.
*/
- if (uid_eq(cred->fsuid, inode->i_uid) || safe_hardlink_source(inode) ||
- capable(CAP_FOWNER))
+ if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
return 0;
audit_log_link_denied("linkat", link);
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] namei: permit linking with CAP_FOWNER in userns
@ 2015-10-10 14:59 Dirk Steinmetz
2015-10-20 14:09 ` Dirk Steinmetz
2015-11-03 17:51 ` Kees Cook
0 siblings, 2 replies; 13+ messages in thread
From: Dirk Steinmetz @ 2015-10-10 14:59 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel, linux-kernel; +Cc: Dirk Steinmetz
Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
within an unprivileged user namespace fails, even if CAP_FOWNER is held
within the namespace. This may cause various failures, such as a gentoo
installation within a lxc container failing to build and install specific
packages.
This change permits hardlinking of files owned by mapped uids, if
CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
by using the existing inode_owner_or_capable(), which is aware of
namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
inode_capable to capable_wrt_inode_uidgid").
Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
---
fs/namei.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 726d211..29fc6a6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -955,26 +955,23 @@ static bool safe_hardlink_source(struct inode *inode)
* - sysctl_protected_hardlinks enabled
* - fsuid does not match inode
* - hardlink source is unsafe (see safe_hardlink_source() above)
- * - not CAP_FOWNER
+ * - not CAP_FOWNER in a namespace with the inode owner uid mapped
*
* Returns 0 if successful, -ve on error.
*/
static int may_linkat(struct path *link)
{
- const struct cred *cred;
struct inode *inode;
if (!sysctl_protected_hardlinks)
return 0;
- cred = current_cred();
inode = link->dentry->d_inode;
/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
* otherwise, it must be a safe source.
*/
- if (uid_eq(cred->fsuid, inode->i_uid) || safe_hardlink_source(inode) ||
- capable(CAP_FOWNER))
+ if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
return 0;
audit_log_link_denied("linkat", link);
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-10 14:59 Dirk Steinmetz
@ 2015-10-20 14:09 ` Dirk Steinmetz
2015-10-27 14:33 ` Seth Forshee
2015-11-03 17:51 ` Kees Cook
1 sibling, 1 reply; 13+ messages in thread
From: Dirk Steinmetz @ 2015-10-20 14:09 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel, linux-kernel; +Cc: Dirk Steinmetz
Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
within an unprivileged user namespace fails, even if CAP_FOWNER is held
within the namespace. This may cause various failures, such as a gentoo
installation within a lxc container failing to build and install specific
packages.
This change permits hardlinking of files owned by mapped uids, if
CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
by using the existing inode_owner_or_capable(), which is aware of
namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
inode_capable to capable_wrt_inode_uidgid").
Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
---
This is the third time I'm sending the patch, as the first two attempts did
not provoke a reply. Feel free to point out any issues you see with it --
including formal requirements, as this is the first patch I'm submitting.
I'd really appreciate your time.
Maybe a bit of rationale behind it would be helpful as well: some linux
distributions, especially gentoo in which I discovered the behaviour,
rely on root being able to hardlink arbitrary files. In the case of gentoo,
this happens when building and installing 'man': the built binary has the
suid-flag set and is owned by a user 'man'. The installation script
(running as root) then attempts to insert a hardlink towards that binary.
Thanks to user namespaces, a regular user can use subuids to create a user
namespace, and acquire root-like capabilities within said namespace. It is
then possible to install and use arbitrary linux distributions within such
namespaces. When installing gentoo in that manner, building and installing
'man' fails, as may_linkat checks the capabilities in the init namespace,
where the installation process is owned by a regular user.
In my opinion may_linkat should permit linking in this case, as the file to
link to is owned by one of the regular user's mapped subids. Note that, in
the scenario described above, it is already possible to create the hardlink
through other means (the following listing is from an unprivileged user
namespace):
> # cat /proc/$$/status | grep CapEff
> CapEff: 0000003cfdfeffff
> # ls -l
> total 0
> -rwSr--r-- 1 nobody nobody 0 Oct 20 15:40 file
> # ln file link
> ln: failed to create hard link 'link' => 'file': Operation not permitted
> # su nobody -s /bin/bash -c "ln file link"
> # ls -l
> total 0
> -rwSr--r-- 2 nobody nobody 0 Oct 20 15:40 file
> -rwSr--r-- 2 nobody nobody 0 Oct 20 15:40 link
As you can see, the process has CAP_FOWNER in the namespace, but cannot
hardlink the file owned by 'nobody'. It can, however, use su to switch to
'nobody' and then create the link. After applying this patch, linking
works as expected.
Diffstat:
fs/namei.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 33e9495..0d3340b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -955,26 +955,23 @@ static bool safe_hardlink_source(struct inode *inode)
* - sysctl_protected_hardlinks enabled
* - fsuid does not match inode
* - hardlink source is unsafe (see safe_hardlink_source() above)
- * - not CAP_FOWNER
+ * - not CAP_FOWNER in a namespace with the inode owner uid mapped
*
* Returns 0 if successful, -ve on error.
*/
static int may_linkat(struct path *link)
{
- const struct cred *cred;
struct inode *inode;
if (!sysctl_protected_hardlinks)
return 0;
- cred = current_cred();
inode = link->dentry->d_inode;
/* Source inode owner (or CAP_FOWNER) can hardlink all they like,
* otherwise, it must be a safe source.
*/
- if (uid_eq(cred->fsuid, inode->i_uid) || safe_hardlink_source(inode) ||
- capable(CAP_FOWNER))
+ if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
return 0;
audit_log_link_denied("linkat", link);
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-20 14:09 ` Dirk Steinmetz
@ 2015-10-27 14:33 ` Seth Forshee
2015-10-27 18:08 ` Dirk Steinmetz
2015-10-27 21:04 ` Eric W. Biederman
0 siblings, 2 replies; 13+ messages in thread
From: Seth Forshee @ 2015-10-27 14:33 UTC (permalink / raw)
To: Dirk Steinmetz
Cc: Alexander Viro, linux-fsdevel, linux-kernel, Eric W. Biederman,
Andy Lutomirski, Kees Cook, Serge Hallyn
On Tue, Oct 20, 2015 at 04:09:19PM +0200, Dirk Steinmetz wrote:
> Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
> within an unprivileged user namespace fails, even if CAP_FOWNER is held
> within the namespace. This may cause various failures, such as a gentoo
> installation within a lxc container failing to build and install specific
> packages.
>
> This change permits hardlinking of files owned by mapped uids, if
> CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
> by using the existing inode_owner_or_capable(), which is aware of
> namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
> inode_capable to capable_wrt_inode_uidgid").
>
> Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
Tested-by: Seth Forshee <seth.forshee@canonical.com>
This is hitting us in Ubuntu during some dpkg upgrades in containers.
When upgrading a file dpkg creates a hard link to the old file to back
it up before overwriting it. When packages upgrade suid files owned by a
non-root user the link isn't permitted, and the package upgrade fails.
This patch fixes our problem.
I did want to point what seems to be an inconsistency in how
capabilities in user namespaces are handled with respect to inodes. When
I started looking at this my initial thought was to replace
capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
the face of it this should be equivalent to what's done here, but it
turns out that capable_wrt_inode_uidgid requires that the inode's uid
and gid are both mapped into the namespace whereas
inode_owner_or_capable only requires the uid be mapped. I'm not sure how
significant that is, but it seems a bit odd.
Seth
> ---
> This is the third time I'm sending the patch, as the first two attempts did
> not provoke a reply. Feel free to point out any issues you see with it --
> including formal requirements, as this is the first patch I'm submitting.
> I'd really appreciate your time.
>
> Maybe a bit of rationale behind it would be helpful as well: some linux
> distributions, especially gentoo in which I discovered the behaviour,
> rely on root being able to hardlink arbitrary files. In the case of gentoo,
> this happens when building and installing 'man': the built binary has the
> suid-flag set and is owned by a user 'man'. The installation script
> (running as root) then attempts to insert a hardlink towards that binary.
>
> Thanks to user namespaces, a regular user can use subuids to create a user
> namespace, and acquire root-like capabilities within said namespace. It is
> then possible to install and use arbitrary linux distributions within such
> namespaces. When installing gentoo in that manner, building and installing
> 'man' fails, as may_linkat checks the capabilities in the init namespace,
> where the installation process is owned by a regular user.
>
> In my opinion may_linkat should permit linking in this case, as the file to
> link to is owned by one of the regular user's mapped subids. Note that, in
> the scenario described above, it is already possible to create the hardlink
> through other means (the following listing is from an unprivileged user
> namespace):
> > # cat /proc/$$/status | grep CapEff
> > CapEff: 0000003cfdfeffff
> > # ls -l
> > total 0
> > -rwSr--r-- 1 nobody nobody 0 Oct 20 15:40 file
> > # ln file link
> > ln: failed to create hard link 'link' => 'file': Operation not permitted
> > # su nobody -s /bin/bash -c "ln file link"
> > # ls -l
> > total 0
> > -rwSr--r-- 2 nobody nobody 0 Oct 20 15:40 file
> > -rwSr--r-- 2 nobody nobody 0 Oct 20 15:40 link
> As you can see, the process has CAP_FOWNER in the namespace, but cannot
> hardlink the file owned by 'nobody'. It can, however, use su to switch to
> 'nobody' and then create the link. After applying this patch, linking
> works as expected.
>
> Diffstat:
> fs/namei.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 33e9495..0d3340b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -955,26 +955,23 @@ static bool safe_hardlink_source(struct inode *inode)
> * - sysctl_protected_hardlinks enabled
> * - fsuid does not match inode
> * - hardlink source is unsafe (see safe_hardlink_source() above)
> - * - not CAP_FOWNER
> + * - not CAP_FOWNER in a namespace with the inode owner uid mapped
> *
> * Returns 0 if successful, -ve on error.
> */
> static int may_linkat(struct path *link)
> {
> - const struct cred *cred;
> struct inode *inode;
>
> if (!sysctl_protected_hardlinks)
> return 0;
>
> - cred = current_cred();
> inode = link->dentry->d_inode;
>
> /* Source inode owner (or CAP_FOWNER) can hardlink all they like,
> * otherwise, it must be a safe source.
> */
> - if (uid_eq(cred->fsuid, inode->i_uid) || safe_hardlink_source(inode) ||
> - capable(CAP_FOWNER))
> + if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
> return 0;
>
> audit_log_link_denied("linkat", link);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-27 14:33 ` Seth Forshee
@ 2015-10-27 18:08 ` Dirk Steinmetz
2015-10-27 20:28 ` Serge Hallyn
2015-10-27 21:04 ` Eric W. Biederman
1 sibling, 1 reply; 13+ messages in thread
From: Dirk Steinmetz @ 2015-10-27 18:08 UTC (permalink / raw)
To: Seth Forshee
Cc: Alexander Viro, linux-fsdevel, linux-kernel, Eric W. Biederman,
Andy Lutomirski, Kees Cook, Serge Hallyn, Dirk Steinmetz
On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> On Tue, Oct 20, 2015 at 04:09:19PM +0200, Dirk Steinmetz wrote:
> > Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
> > within an unprivileged user namespace fails, even if CAP_FOWNER is held
> > within the namespace. This may cause various failures, such as a gentoo
> > installation within a lxc container failing to build and install specific
> > packages.
> >
> > This change permits hardlinking of files owned by mapped uids, if
> > CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
> > by using the existing inode_owner_or_capable(), which is aware of
> > namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
> > inode_capable to capable_wrt_inode_uidgid").
> >
> > Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
>
> Tested-by: Seth Forshee <seth.forshee@canonical.com>
>
> This is hitting us in Ubuntu during some dpkg upgrades in containers.
> When upgrading a file dpkg creates a hard link to the old file to back
> it up before overwriting it. When packages upgrade suid files owned by a
> non-root user the link isn't permitted, and the package upgrade fails.
> This patch fixes our problem.
>
> I did want to point what seems to be an inconsistency in how
> capabilities in user namespaces are handled with respect to inodes. When
> I started looking at this my initial thought was to replace
> capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> the face of it this should be equivalent to what's done here, but it
> turns out that capable_wrt_inode_uidgid requires that the inode's uid
> and gid are both mapped into the namespace whereas
> inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> significant that is, but it seems a bit odd.
I agree that this seems odd. I've chosen inode_owner_or_capable over
capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
bypass the check completely; this also matches the documented behavior of
CAP_FOWNER: "Bypass permission checks on operations that normally require
the filesystem UID of the process to match the UID of the file".
However, thinking about it I get the feeling that checking the gid seems
reasonable as well. This is, however, independently of user namespaces.
Consider the following scenario in any namespace, including the init one:
- A file has the setgid and user/group executable bits set, and is owned
by user:group.
- The user 'user' is not in the group 'group', and does not have any
capabilities.
- The user 'user' hardlinks the file. The permission check will succeed,
as the user is the owner of the file.
- The file is replaced with a newer version (for example fixing a security
issue)
- Now user can still use the hardlink-pinned version to execute the file
as 'user:group' (and for example exploit the security issue).
I would have expected the user to not be able to hardlink, as he lacks
CAP_FSETID, and thus is not allowed to chmod, change or move the file
without loosing the setgid bit. So it is impossible for him to make a non-
hardlink copy with the setgid bit set -- why should he be able to make a
hardlinked one?
It seems to me as if may_linkat would additionally require a check
verifying that either
- not both setgid and group executable bit set
- fsgid == owner gid
- capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
whether to adapt chmod's behavior or keeping everything hardlink-
related in CAP_FOWNER; I don't feel qualified enough to pick ;)
This would change documented behavior (at least man proc.5's description
of /proc/sys/fs/protected_hardlinks), and I'd consider it a separate
issue, if any (as I'm unsure how realistic that scenario is). I'd
appreciate comments on that.
For other situations than setgid-executable files I do not see issues with
not checking the group id's mapping, as linking would be permitted without
privileges outside of the user namespace (disregarding namespace-internal
setuid bits).
Independently of that, it might be reasonable to consider switching
inode_owner_or_capable towards checking the gid as well and define
something along "uid checks in user namespaces with uid/gid maps require
the file's uid and gid to be mapped, else they will fail" for consistency.
Dirk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-27 18:08 ` Dirk Steinmetz
@ 2015-10-27 20:28 ` Serge Hallyn
2015-10-28 15:07 ` Dirk Steinmetz
0 siblings, 1 reply; 13+ messages in thread
From: Serge Hallyn @ 2015-10-27 20:28 UTC (permalink / raw)
To: Dirk Steinmetz
Cc: Seth Forshee, Alexander Viro, linux-fsdevel, linux-kernel,
Eric W. Biederman, Andy Lutomirski, Kees Cook, Serge Hallyn
Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > On Tue, Oct 20, 2015 at 04:09:19PM +0200, Dirk Steinmetz wrote:
> > > Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
> > > within an unprivileged user namespace fails, even if CAP_FOWNER is held
> > > within the namespace. This may cause various failures, such as a gentoo
> > > installation within a lxc container failing to build and install specific
> > > packages.
> > >
> > > This change permits hardlinking of files owned by mapped uids, if
> > > CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
> > > by using the existing inode_owner_or_capable(), which is aware of
> > > namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
> > > inode_capable to capable_wrt_inode_uidgid").
> > >
> > > Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
> >
> > Tested-by: Seth Forshee <seth.forshee@canonical.com>
> >
> > This is hitting us in Ubuntu during some dpkg upgrades in containers.
> > When upgrading a file dpkg creates a hard link to the old file to back
> > it up before overwriting it. When packages upgrade suid files owned by a
> > non-root user the link isn't permitted, and the package upgrade fails.
> > This patch fixes our problem.
> >
> > I did want to point what seems to be an inconsistency in how
> > capabilities in user namespaces are handled with respect to inodes. When
> > I started looking at this my initial thought was to replace
> > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > the face of it this should be equivalent to what's done here, but it
> > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > and gid are both mapped into the namespace whereas
> > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > significant that is, but it seems a bit odd.
>
> I agree that this seems odd. I've chosen inode_owner_or_capable over
> capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> bypass the check completely; this also matches the documented behavior of
> CAP_FOWNER: "Bypass permission checks on operations that normally require
> the filesystem UID of the process to match the UID of the file".
>
> However, thinking about it I get the feeling that checking the gid seems
> reasonable as well. This is, however, independently of user namespaces.
> Consider the following scenario in any namespace, including the init one:
> - A file has the setgid and user/group executable bits set, and is owned
> by user:group.
> - The user 'user' is not in the group 'group', and does not have any
> capabilities.
> - The user 'user' hardlinks the file. The permission check will succeed,
> as the user is the owner of the file.
> - The file is replaced with a newer version (for example fixing a security
> issue)
> - Now user can still use the hardlink-pinned version to execute the file
> as 'user:group' (and for example exploit the security issue).
> I would have expected the user to not be able to hardlink, as he lacks
> CAP_FSETID, and thus is not allowed to chmod, change or move the file
> without loosing the setgid bit. So it is impossible for him to make a non-
> hardlink copy with the setgid bit set -- why should he be able to make a
> hardlinked one?
Yeah, this sounds sensible. It allows a user without access to 'disk',
for instance, to become that group.
> It seems to me as if may_linkat would additionally require a check
> verifying that either
> - not both setgid and group executable bit set
> - fsgid == owner gid
> - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> whether to adapt chmod's behavior or keeping everything hardlink-
> related in CAP_FOWNER; I don't feel qualified enough to pick ;)
In particular just changing it is not ok since people who are using file
capabilities to grant what they currently need would be stuck with a
mysterious new failure.
> This would change documented behavior (at least man proc.5's description
> of /proc/sys/fs/protected_hardlinks), and I'd consider it a separate
> issue, if any (as I'm unsure how realistic that scenario is). I'd
> appreciate comments on that.
>
> For other situations than setgid-executable files I do not see issues with
> not checking the group id's mapping, as linking would be permitted without
> privileges outside of the user namespace (disregarding namespace-internal
> setuid bits).
>
> Independently of that, it might be reasonable to consider switching
> inode_owner_or_capable towards checking the gid as well and define
> something along "uid checks in user namespaces with uid/gid maps require
> the file's uid and gid to be mapped, else they will fail" for consistency.
>
> Dirk
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-27 14:33 ` Seth Forshee
2015-10-27 18:08 ` Dirk Steinmetz
@ 2015-10-27 21:04 ` Eric W. Biederman
1 sibling, 0 replies; 13+ messages in thread
From: Eric W. Biederman @ 2015-10-27 21:04 UTC (permalink / raw)
To: Seth Forshee
Cc: Dirk Steinmetz, Alexander Viro, linux-fsdevel, linux-kernel,
Andy Lutomirski, Kees Cook, Serge Hallyn
Seth Forshee <seth.forshee@canonical.com> writes:
> On Tue, Oct 20, 2015 at 04:09:19PM +0200, Dirk Steinmetz wrote:
>> Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
>> within an unprivileged user namespace fails, even if CAP_FOWNER is held
>> within the namespace. This may cause various failures, such as a gentoo
>> installation within a lxc container failing to build and install specific
>> packages.
>>
>> This change permits hardlinking of files owned by mapped uids, if
>> CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
>> by using the existing inode_owner_or_capable(), which is aware of
>> namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
>> inode_capable to capable_wrt_inode_uidgid").
>>
>> Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
>
> Tested-by: Seth Forshee <seth.forshee@canonical.com>
If multiple groups are hitting this issue for different reasons
I am applying the supplied patch.
> This is hitting us in Ubuntu during some dpkg upgrades in containers.
> When upgrading a file dpkg creates a hard link to the old file to back
> it up before overwriting it. When packages upgrade suid files owned by a
> non-root user the link isn't permitted, and the package upgrade fails.
> This patch fixes our problem.
>
> I did want to point what seems to be an inconsistency in how
> capabilities in user namespaces are handled with respect to inodes. When
> I started looking at this my initial thought was to replace
> capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> the face of it this should be equivalent to what's done here, but it
> turns out that capable_wrt_inode_uidgid requires that the inode's uid
> and gid are both mapped into the namespace whereas
> inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> significant that is, but it seems a bit odd.
It is a bit odd.
inode_owner_or_capable in this context is a gimme, as only being the
owner of the file in question is enough to create a hard link, and root
(in the user namespace) can become that user.
That said I think there have been some legitimate questions about setgid
executables in may_linkat (raised down thread), as well as legitimate
questions about capable_wrt_uidgid. I will add the additional question
is it sane for us to ignore the acls in capable_wrt_uidgid.
All of this appears to be an area that no one except bad actors cares
about so I expect we can change things without causing regressions, and
on that note I encourage the conversation on the oddness to continue.
Eric
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-27 20:28 ` Serge Hallyn
@ 2015-10-28 15:07 ` Dirk Steinmetz
2015-10-28 17:33 ` Serge Hallyn
0 siblings, 1 reply; 13+ messages in thread
From: Dirk Steinmetz @ 2015-10-28 15:07 UTC (permalink / raw)
To: Serge Hallyn
Cc: Seth Forshee, Alexander Viro, linux-fsdevel, linux-kernel,
Eric W. Biederman, Andy Lutomirski, Kees Cook, Serge Hallyn
On Tue, 27 Oct 2015 20:28:02 +0000, Serge Hallyn wrote:
> Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > > I did want to point what seems to be an inconsistency in how
> > > capabilities in user namespaces are handled with respect to inodes. When
> > > I started looking at this my initial thought was to replace
> > > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > > the face of it this should be equivalent to what's done here, but it
> > > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > > and gid are both mapped into the namespace whereas
> > > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > > significant that is, but it seems a bit odd.
> >
> > I agree that this seems odd. I've chosen inode_owner_or_capable over
> > capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> > a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> > bypass the check completely; this also matches the documented behavior of
> > CAP_FOWNER: "Bypass permission checks on operations that normally require
> > the filesystem UID of the process to match the UID of the file".
> >
> > However, thinking about it I get the feeling that checking the gid seems
> > reasonable as well. This is, however, independently of user namespaces.
> > Consider the following scenario in any namespace, including the init one:
> > - A file has the setgid and user/group executable bits set, and is owned
> > by user:group.
> > - The user 'user' is not in the group 'group', and does not have any
> > capabilities.
> > - The user 'user' hardlinks the file. The permission check will succeed,
> > as the user is the owner of the file.
> > - The file is replaced with a newer version (for example fixing a security
> > issue)
> > - Now user can still use the hardlink-pinned version to execute the file
> > as 'user:group' (and for example exploit the security issue).
> > I would have expected the user to not be able to hardlink, as he lacks
> > CAP_FSETID, and thus is not allowed to chmod, change or move the file
> > without loosing the setgid bit. So it is impossible for him to make a non-
> > hardlink copy with the setgid bit set -- why should he be able to make a
> > hardlinked one?
>
> Yeah, this sounds sensible. It allows a user without access to 'disk',
> for instance, to become that group.
>
> > It seems to me as if may_linkat would additionally require a check
> > verifying that either
> > - not both setgid and group executable bit set
> > - fsgid == owner gid
> > - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> > whether to adapt chmod's behavior or keeping everything hardlink-
> > related in CAP_FOWNER; I don't feel qualified enough to pick ;)
>
> In particular just changing it is not ok since people who are using file
> capabilities to grant what they currently need would be stuck with a
> mysterious new failure.
Is there any use case (besides exploiting hardlinks with malicious intent)
that would be broken when changing this? There are some (imho) rather
unlikely conditions to be met in order to observe changed behavior:
- a user owns an executable setgid-file belonging to a group he is not in
- the user does not have CAP_FSETID (or CAP_FOWNER, depending on which one
is chosen to be required)
- the user is for some legitimate reason supposed to hardlink the file
If these conditions are not met in practice, the change would not break
anything. In that case, it would be imho better to not provide
backward-compatibility to reduce complexity in these checks. Else, I'd
propose adding a new possible value '2' for
/proc/sys/fs/protected_hardlinks, while keeping '1' for the current
behavior.
I can provide an initial draft for either of the options, but would like
recommendations to which of the two ways to take (or is there a third
one?), as well as comments on the new condition itself: may_linkat would
block hardlinks when all of the following conditions are met:
- sysctrl_protected_hardlinks is enabled or 2 (depending on way)
- inode uid != fsuid and no CAP_FOWNER (for userns: with mapping on uid),
while the hardlink source is not a regular file, is a setuid-executable
or is not accessible for reading and writing
- inode gid not fsgid or in supplemental gids and no CAP_FSETID (for
userns: with mapping on gid -- not sure whether the uid is relevant?),
while the hardlink source is a setgid-executable (with group executable
bit set)
If anyone else wants to fix the issue, thats fine with me as well.
Dirk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-28 15:07 ` Dirk Steinmetz
@ 2015-10-28 17:33 ` Serge Hallyn
2015-11-02 15:10 ` Dirk Steinmetz
0 siblings, 1 reply; 13+ messages in thread
From: Serge Hallyn @ 2015-10-28 17:33 UTC (permalink / raw)
To: Dirk Steinmetz
Cc: Seth Forshee, Alexander Viro, linux-fsdevel, linux-kernel,
Eric W. Biederman, Andy Lutomirski, Kees Cook, Serge Hallyn
Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> On Tue, 27 Oct 2015 20:28:02 +0000, Serge Hallyn wrote:
> > Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > > On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > > > I did want to point what seems to be an inconsistency in how
> > > > capabilities in user namespaces are handled with respect to inodes. When
> > > > I started looking at this my initial thought was to replace
> > > > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > > > the face of it this should be equivalent to what's done here, but it
> > > > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > > > and gid are both mapped into the namespace whereas
> > > > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > > > significant that is, but it seems a bit odd.
> > >
> > > I agree that this seems odd. I've chosen inode_owner_or_capable over
> > > capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> > > a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> > > bypass the check completely; this also matches the documented behavior of
> > > CAP_FOWNER: "Bypass permission checks on operations that normally require
> > > the filesystem UID of the process to match the UID of the file".
> > >
> > > However, thinking about it I get the feeling that checking the gid seems
> > > reasonable as well. This is, however, independently of user namespaces.
> > > Consider the following scenario in any namespace, including the init one:
> > > - A file has the setgid and user/group executable bits set, and is owned
> > > by user:group.
> > > - The user 'user' is not in the group 'group', and does not have any
> > > capabilities.
> > > - The user 'user' hardlinks the file. The permission check will succeed,
> > > as the user is the owner of the file.
> > > - The file is replaced with a newer version (for example fixing a security
> > > issue)
> > > - Now user can still use the hardlink-pinned version to execute the file
> > > as 'user:group' (and for example exploit the security issue).
> > > I would have expected the user to not be able to hardlink, as he lacks
> > > CAP_FSETID, and thus is not allowed to chmod, change or move the file
> > > without loosing the setgid bit. So it is impossible for him to make a non-
> > > hardlink copy with the setgid bit set -- why should he be able to make a
> > > hardlinked one?
> >
> > Yeah, this sounds sensible. It allows a user without access to 'disk',
> > for instance, to become that group.
> >
> > > It seems to me as if may_linkat would additionally require a check
> > > verifying that either
> > > - not both setgid and group executable bit set
> > > - fsgid == owner gid
> > > - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> > > whether to adapt chmod's behavior or keeping everything hardlink-
> > > related in CAP_FOWNER; I don't feel qualified enough to pick ;)
> >
> > In particular just changing it is not ok since people who are using file
> > capabilities to grant what they currently need would be stuck with a
> > mysterious new failure.
>
> Is there any use case (besides exploiting hardlinks with malicious intent)
> that would be broken when changing this? There are some (imho) rather
> unlikely conditions to be met in order to observe changed behavior:
The simplest example would be if I wanted to run a very quick program to
just add the symbolic link. Let's say the link /usr/sbin/uuidd were owned
by root:disk and setuid and setgid. The proposed change would force me
to bind in both the root user and disk group, whereas without it I can
just bind in only the root user.
We've already dealt with such regressions and iirc agreed that they were
worthwhile.
> - a user owns an executable setgid-file belonging to a group he is not in
> - the user does not have CAP_FSETID (or CAP_FOWNER, depending on which one
> is chosen to be required)
> - the user is for some legitimate reason supposed to hardlink the file
> If these conditions are not met in practice, the change would not break
> anything. In that case, it would be imho better to not provide
> backward-compatibility to reduce complexity in these checks. Else, I'd
> propose adding a new possible value '2' for
> /proc/sys/fs/protected_hardlinks, while keeping '1' for the current
> behavior.
>
> I can provide an initial draft for either of the options, but would like
> recommendations to which of the two ways to take (or is there a third
> one?), as well as comments on the new condition itself: may_linkat would
> block hardlinks when all of the following conditions are met:
> - sysctrl_protected_hardlinks is enabled or 2 (depending on way)
> - inode uid != fsuid and no CAP_FOWNER (for userns: with mapping on uid),
> while the hardlink source is not a regular file, is a setuid-executable
> or is not accessible for reading and writing
> - inode gid not fsgid or in supplemental gids and no CAP_FSETID (for
> userns: with mapping on gid -- not sure whether the uid is relevant?),
> while the hardlink source is a setgid-executable (with group executable
> bit set)
>
> If anyone else wants to fix the issue, thats fine with me as well.
>
> Dirk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-28 17:33 ` Serge Hallyn
@ 2015-11-02 15:10 ` Dirk Steinmetz
2015-11-02 18:02 ` Serge Hallyn
0 siblings, 1 reply; 13+ messages in thread
From: Dirk Steinmetz @ 2015-11-02 15:10 UTC (permalink / raw)
To: Serge Hallyn
Cc: Seth Forshee, Alexander Viro, linux-fsdevel, linux-kernel,
Eric W. Biederman, Andy Lutomirski, Kees Cook, Serge Hallyn
On Wed, 28 Oct 2015 17:33:10 +0000, Serge Hallyn wrote:
> Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > On Tue, 27 Oct 2015 20:28:02 +0000, Serge Hallyn wrote:
> > > Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > > > On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > > > > I did want to point what seems to be an inconsistency in how
> > > > > capabilities in user namespaces are handled with respect to inodes. When
> > > > > I started looking at this my initial thought was to replace
> > > > > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > > > > the face of it this should be equivalent to what's done here, but it
> > > > > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > > > > and gid are both mapped into the namespace whereas
> > > > > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > > > > significant that is, but it seems a bit odd.
> > > >
> > > > I agree that this seems odd. I've chosen inode_owner_or_capable over
> > > > capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> > > > a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> > > > bypass the check completely; this also matches the documented behavior of
> > > > CAP_FOWNER: "Bypass permission checks on operations that normally require
> > > > the filesystem UID of the process to match the UID of the file".
> > > >
> > > > However, thinking about it I get the feeling that checking the gid seems
> > > > reasonable as well. This is, however, independently of user namespaces.
> > > > Consider the following scenario in any namespace, including the init one:
> > > > - A file has the setgid and user/group executable bits set, and is owned
> > > > by user:group.
> > > > - The user 'user' is not in the group 'group', and does not have any
> > > > capabilities.
> > > > - The user 'user' hardlinks the file. The permission check will succeed,
> > > > as the user is the owner of the file.
> > > > - The file is replaced with a newer version (for example fixing a security
> > > > issue)
> > > > - Now user can still use the hardlink-pinned version to execute the file
> > > > as 'user:group' (and for example exploit the security issue).
> > > > I would have expected the user to not be able to hardlink, as he lacks
> > > > CAP_FSETID, and thus is not allowed to chmod, change or move the file
> > > > without loosing the setgid bit. So it is impossible for him to make a non-
> > > > hardlink copy with the setgid bit set -- why should he be able to make a
> > > > hardlinked one?
> > >
> > > Yeah, this sounds sensible. It allows a user without access to 'disk',
> > > for instance, to become that group.
> > >
> > > > It seems to me as if may_linkat would additionally require a check
> > > > verifying that either
> > > > - not both setgid and group executable bit set
> > > > - fsgid == owner gid
> > > > - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> > > > whether to adapt chmod's behavior or keeping everything hardlink-
> > > > related in CAP_FOWNER; I don't feel qualified enough to pick ;)
> > >
> > > In particular just changing it is not ok since people who are using file
> > > capabilities to grant what they currently need would be stuck with a
> > > mysterious new failure.
> >
> > Is there any use case (besides exploiting hardlinks with malicious intent)
> > that would be broken when changing this? There are some (imho) rather
> > unlikely conditions to be met in order to observe changed behavior:
>
> The simplest example would be if I wanted to run a very quick program to
> just add the symbolic link. Let's say the link /usr/sbin/uuidd were owned
> by root:disk and setuid and setgid. The proposed change would force me
> to bind in both the root user and disk group, whereas without it I can
> just bind in only the root user.
While root usually has CAP_FSETID and CAP_FOWNER, which would still permit
linking in this case, I agree that the change could be visible when
performing specific maintenance tasks in some rare setups.
> We've already dealt with such regressions and iirc agreed that they were
> worthwhile.
Would you prefer to not fix the issue at all, then? Or would you prefer to
add a new value on /proc/sys/fs/protected_hardlinks -- which would still
cause the symptoms you describe on distributions using the new value, but
would be more easy to change for users knowing that this is an issue?
I personally still favor changing the behavior and documentation over a new
value there, as -- once identified by the user -- the user can easily adapt
his usage or disable protected hardlinks globally, depending on the actual
requirements. And another value will not improve the abilities of the user
to identify protected hardlinks as reason for the changed behavior.
> > - a user owns an executable setgid-file belonging to a group he is not in
> > - the user does not have CAP_FSETID (or CAP_FOWNER, depending on which one
> > is chosen to be required)
> > - the user is for some legitimate reason supposed to hardlink the file
> > If these conditions are not met in practice, the change would not break
> > anything. In that case, it would be imho better to not provide
> > backward-compatibility to reduce complexity in these checks. Else, I'd
> > propose adding a new possible value '2' for
> > /proc/sys/fs/protected_hardlinks, while keeping '1' for the current
> > behavior.
> >
> > I can provide an initial draft for either of the options, but would like
> > recommendations to which of the two ways to take (or is there a third
> > one?), as well as comments on the new condition itself: may_linkat would
> > block hardlinks when all of the following conditions are met:
> > - sysctrl_protected_hardlinks is enabled or 2 (depending on way)
> > - inode uid != fsuid and no CAP_FOWNER (for userns: with mapping on uid),
> > while the hardlink source is not a regular file, is a setuid-executable
> > or is not accessible for reading and writing
> > - inode gid not fsgid or in supplemental gids and no CAP_FSETID (for
> > userns: with mapping on gid -- not sure whether the uid is relevant?),
> > while the hardlink source is a setgid-executable (with group executable
> > bit set)
> >
> > If anyone else wants to fix the issue, thats fine with me as well.
> >
> > Dirk
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-11-02 15:10 ` Dirk Steinmetz
@ 2015-11-02 18:02 ` Serge Hallyn
2015-11-02 19:57 ` Andy Lutomirski
0 siblings, 1 reply; 13+ messages in thread
From: Serge Hallyn @ 2015-11-02 18:02 UTC (permalink / raw)
To: Dirk Steinmetz
Cc: Seth Forshee, Alexander Viro, linux-fsdevel, linux-kernel,
Eric W. Biederman, Andy Lutomirski, Kees Cook, Serge Hallyn
Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> On Wed, 28 Oct 2015 17:33:10 +0000, Serge Hallyn wrote:
> > Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > > On Tue, 27 Oct 2015 20:28:02 +0000, Serge Hallyn wrote:
> > > > Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
> > > > > On Tue, 27 Oct 2015 09:33:44 -0500, Seth Forshee wrote:
> > > > > > I did want to point what seems to be an inconsistency in how
> > > > > > capabilities in user namespaces are handled with respect to inodes. When
> > > > > > I started looking at this my initial thought was to replace
> > > > > > capable(CAP_FOWNER) with capable_wrt_inode_uidgid(inode, CAP_FOWNER). On
> > > > > > the face of it this should be equivalent to what's done here, but it
> > > > > > turns out that capable_wrt_inode_uidgid requires that the inode's uid
> > > > > > and gid are both mapped into the namespace whereas
> > > > > > inode_owner_or_capable only requires the uid be mapped. I'm not sure how
> > > > > > significant that is, but it seems a bit odd.
> > > > >
> > > > > I agree that this seems odd. I've chosen inode_owner_or_capable over
> > > > > capable_wrt_inode_uidgid(inode, CAP_FOWNER) as it seemed consistent:
> > > > > a privileged user (with CAP_SETUID) can impersonate the owner UID and thus
> > > > > bypass the check completely; this also matches the documented behavior of
> > > > > CAP_FOWNER: "Bypass permission checks on operations that normally require
> > > > > the filesystem UID of the process to match the UID of the file".
> > > > >
> > > > > However, thinking about it I get the feeling that checking the gid seems
> > > > > reasonable as well. This is, however, independently of user namespaces.
> > > > > Consider the following scenario in any namespace, including the init one:
> > > > > - A file has the setgid and user/group executable bits set, and is owned
> > > > > by user:group.
> > > > > - The user 'user' is not in the group 'group', and does not have any
> > > > > capabilities.
> > > > > - The user 'user' hardlinks the file. The permission check will succeed,
> > > > > as the user is the owner of the file.
> > > > > - The file is replaced with a newer version (for example fixing a security
> > > > > issue)
> > > > > - Now user can still use the hardlink-pinned version to execute the file
> > > > > as 'user:group' (and for example exploit the security issue).
> > > > > I would have expected the user to not be able to hardlink, as he lacks
> > > > > CAP_FSETID, and thus is not allowed to chmod, change or move the file
> > > > > without loosing the setgid bit. So it is impossible for him to make a non-
> > > > > hardlink copy with the setgid bit set -- why should he be able to make a
> > > > > hardlinked one?
> > > >
> > > > Yeah, this sounds sensible. It allows a user without access to 'disk',
> > > > for instance, to become that group.
> > > >
> > > > > It seems to me as if may_linkat would additionally require a check
> > > > > verifying that either
> > > > > - not both setgid and group executable bit set
> > > > > - fsgid == owner gid
> > > > > - capable_wrt_inode_uidgid(CAP_FSETID) -- or CAP_FOWNER, depending on
> > > > > whether to adapt chmod's behavior or keeping everything hardlink-
> > > > > related in CAP_FOWNER; I don't feel qualified enough to pick ;)
> > > >
> > > > In particular just changing it is not ok since people who are using file
> > > > capabilities to grant what they currently need would be stuck with a
> > > > mysterious new failure.
> > >
> > > Is there any use case (besides exploiting hardlinks with malicious intent)
> > > that would be broken when changing this? There are some (imho) rather
> > > unlikely conditions to be met in order to observe changed behavior:
> >
> > The simplest example would be if I wanted to run a very quick program to
> > just add the symbolic link. Let's say the link /usr/sbin/uuidd were owned
> > by root:disk and setuid and setgid. The proposed change would force me
> > to bind in both the root user and disk group, whereas without it I can
> > just bind in only the root user.
> While root usually has CAP_FSETID and CAP_FOWNER, which would still permit
> linking in this case, I agree that the change could be visible when
> performing specific maintenance tasks in some rare setups.
>
> > We've already dealt with such regressions and iirc agreed that they were
> > worthwhile.
> Would you prefer to not fix the issue at all, then? Or would you prefer to
No. I think I was saying I think it's worth adding the 'gid must be mapped'
requirement.
And I was saying that changing the capability needed is not ok.
> add a new value on /proc/sys/fs/protected_hardlinks -- which would still
> cause the symptoms you describe on distributions using the new value, but
> would be more easy to change for users knowing that this is an issue?
>
> I personally still favor changing the behavior and documentation over a new
> value there, as -- once identified by the user -- the user can easily adapt
I agree.
Note the difference - changing the capability required to link the
file can affect (probably rare, but definately) normal, non-user-namespace
setups. Changing the link requirements in a user namespace so that gid
must be mapped only affects a case which we've previously said should not
be supported.
Linus may still disagree - not changing what userspace can do is pretty
fundamental, but this was purely a missed security fix iiuc.
> his usage or disable protected hardlinks globally, depending on the actual
> requirements. And another value will not improve the abilities of the user
> to identify protected hardlinks as reason for the changed behavior.
Agreed, and more to the point changing the sysctl required woudl still be
breaking userspace. We may as well require the proper, safe setup, rather
than requiring a unsafe sysctl.
-serge
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-11-02 18:02 ` Serge Hallyn
@ 2015-11-02 19:57 ` Andy Lutomirski
0 siblings, 0 replies; 13+ messages in thread
From: Andy Lutomirski @ 2015-11-02 19:57 UTC (permalink / raw)
To: Serge Hallyn
Cc: Dirk Steinmetz, Seth Forshee, Alexander Viro, Linux FS Devel,
linux-kernel@vger.kernel.org, Eric W. Biederman, Kees Cook,
Serge Hallyn
On Mon, Nov 2, 2015 at 10:02 AM, Serge Hallyn <serge.hallyn@ubuntu.com> wrote:
> Quoting Dirk Steinmetz (public@rsjtdrjgfuzkfg.com):
>>
>> > We've already dealt with such regressions and iirc agreed that they were
>> > worthwhile.
>> Would you prefer to not fix the issue at all, then? Or would you prefer to
>
> No. I think I was saying I think it's worth adding the 'gid must be mapped'
> requirement.
>
> And I was saying that changing the capability needed is not ok.
>
>> add a new value on /proc/sys/fs/protected_hardlinks -- which would still
>> cause the symptoms you describe on distributions using the new value, but
>> would be more easy to change for users knowing that this is an issue?
>>
>> I personally still favor changing the behavior and documentation over a new
>> value there, as -- once identified by the user -- the user can easily adapt
>
> I agree.
>
> Note the difference - changing the capability required to link the
> file can affect (probably rare, but definately) normal, non-user-namespace
> setups. Changing the link requirements in a user namespace so that gid
> must be mapped only affects a case which we've previously said should not
> be supported.
I think it would have no effect at all on setups that don't use
userns, so at least the exposure to potential ABI issues would be
small.
>
> Linus may still disagree - not changing what userspace can do is pretty
> fundamental, but this was purely a missed security fix iiuc.
IIRC I just didn't do it because I didn't want to think about it at
the time, and it didn't look like a *big* security issue.
--Andy
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] namei: permit linking with CAP_FOWNER in userns
2015-10-10 14:59 Dirk Steinmetz
2015-10-20 14:09 ` Dirk Steinmetz
@ 2015-11-03 17:51 ` Kees Cook
1 sibling, 0 replies; 13+ messages in thread
From: Kees Cook @ 2015-11-03 17:51 UTC (permalink / raw)
To: Dirk Steinmetz
Cc: Eric W. Biederman, Alexander Viro, linux-fsdevel@vger.kernel.org,
LKML, Andy Lutomirski, Serge Hallyn, Seth Forshee
On Sat, Oct 10, 2015 at 7:59 AM, Dirk Steinmetz
<public@rsjtdrjgfuzkfg.com> wrote:
> Attempting to hardlink to an unsafe file (e.g. a setuid binary) from
> within an unprivileged user namespace fails, even if CAP_FOWNER is held
> within the namespace. This may cause various failures, such as a gentoo
> installation within a lxc container failing to build and install specific
> packages.
>
> This change permits hardlinking of files owned by mapped uids, if
> CAP_FOWNER is held for that namespace. Furthermore, it improves consistency
> by using the existing inode_owner_or_capable(), which is aware of
> namespaced capabilities as of 23adbe12ef7d3 ("fs,userns: Change
> inode_capable to capable_wrt_inode_uidgid").
>
> Signed-off-by: Dirk Steinmetz <public@rsjtdrjgfuzkfg.com>
Sorry for the delay: was travelling when I got put on CC. (FWIW, in
the future, please check the scripts/get_maintainer.pl script with
--git-blame to build CC lists, then I would have been CCed earlier.)
I think Eric's already taken this patch, but it looks correct to me:
Acked-by: Kees Cook <keescook@chromium.org>
I'll hop on the other thread to discuss the setgid issue.
-Kees
> ---
> fs/namei.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 726d211..29fc6a6 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -955,26 +955,23 @@ static bool safe_hardlink_source(struct inode *inode)
> * - sysctl_protected_hardlinks enabled
> * - fsuid does not match inode
> * - hardlink source is unsafe (see safe_hardlink_source() above)
> - * - not CAP_FOWNER
> + * - not CAP_FOWNER in a namespace with the inode owner uid mapped
> *
> * Returns 0 if successful, -ve on error.
> */
> static int may_linkat(struct path *link)
> {
> - const struct cred *cred;
> struct inode *inode;
>
> if (!sysctl_protected_hardlinks)
> return 0;
>
> - cred = current_cred();
> inode = link->dentry->d_inode;
>
> /* Source inode owner (or CAP_FOWNER) can hardlink all they like,
> * otherwise, it must be a safe source.
> */
> - if (uid_eq(cred->fsuid, inode->i_uid) || safe_hardlink_source(inode) ||
> - capable(CAP_FOWNER))
> + if (inode_owner_or_capable(inode) || safe_hardlink_source(inode))
> return 0;
>
> audit_log_link_denied("linkat", link);
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-11-03 17:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-30 0:05 [PATCH] namei: permit linking with CAP_FOWNER in userns Dirk Steinmetz
-- strict thread matches above, loose matches on Subject: below --
2015-10-10 14:59 Dirk Steinmetz
2015-10-20 14:09 ` Dirk Steinmetz
2015-10-27 14:33 ` Seth Forshee
2015-10-27 18:08 ` Dirk Steinmetz
2015-10-27 20:28 ` Serge Hallyn
2015-10-28 15:07 ` Dirk Steinmetz
2015-10-28 17:33 ` Serge Hallyn
2015-11-02 15:10 ` Dirk Steinmetz
2015-11-02 18:02 ` Serge Hallyn
2015-11-02 19:57 ` Andy Lutomirski
2015-10-27 21:04 ` Eric W. Biederman
2015-11-03 17:51 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).