Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1185463 > unrolled thread

Re: [PATCH 0/7] Initial support for user namespace owned mounts

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/7] Initial support for user namespace owned mounts ebiederm@xmission.com (Eric W. Biederman) - 2015-07-16 05:30 +0200
    Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-16 17:10 +0200
      Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-16 21:00 +0200
        Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-16 23:50 +0200
          Re: [PATCH 0/7] Initial support for user namespace owned mounts Andy Lutomirski <luto@amacapital.net> - 2015-07-17 00:30 +0200
            Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-17 01:10 +0200
              Re: [PATCH 0/7] Initial support for user namespace owned mounts Andy Lutomirski <luto@amacapital.net> - 2015-07-17 01:40 +0200
                Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-17 02:50 +0200
                  Re: [PATCH 0/7] Initial support for user namespace owned mounts Andy Lutomirski <luto@amacapital.net> - 2015-07-17 03:00 +0200
                    Re: [PATCH 0/7] Initial support for user namespace owned mounts "Serge E. Hallyn" <serge@hallyn.com> - 2015-07-17 16:30 +0200
                      Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-17 17:00 +0200
                    Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-22 18:00 +0200
                      Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-23 02:10 +0200
                        Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-23 07:20 +0200
                        Re: [PATCH 0/7] Initial support for user namespace owned mounts Casey Schaufler <casey@schaufler-ca.com> - 2015-07-23 23:50 +0200
                        Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-28 22:50 +0200
          Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-17 15:30 +0200
    Re: [PATCH 0/7] Initial support for user namespace owned mounts Seth Forshee <seth.forshee@canonical.com> - 2015-07-16 18:10 +0200

#1185463 — Re: [PATCH 0/7] Initial support for user namespace owned mounts

Seth I think for the LSMs we should start with:

diff --git a/security/security.c b/security/security.c
index 062f3c997fdc..5b6ece92a8e5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -310,6 +310,8 @@ int security_sb_statfs(struct dentry *dentry)
 int security_sb_mount(const char *dev_name, struct path *path,
                        const char *type, unsigned long flags, void *data)
 {
+       if (current_user_ns() != &init_user_ns)
+               return -EPERM;
        return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
 }


Then we should push this down into all of the lsms.
Then when we should remove or relax or change the check as appropriate
in each lsm.

The point is this is good enough to see that it is trivially safe,
and this allows us to focus on the core issues, and stop worrying about
the lsms for a bit.

Then we can focus on each lsm one at at time and take the time to really
understand them and talk with their maintainers etc to make certain
we get things correct.

This should remove the need for your patches 5, 6 and 7. For the
immediate future.

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

[toc] | [next] | [standalone]

#1185948

On 7/16/2015 6:59 AM, Seth Forshee wrote:
> On Wed, Jul 15, 2015 at 10:15:21PM -0500, Eric W. Biederman wrote:
>> Seth I think for the LSMs we should start with:
>>
>> diff --git a/security/security.c b/security/security.c
>> index 062f3c997fdc..5b6ece92a8e5 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -310,6 +310,8 @@ int security_sb_statfs(struct dentry *dentry)
>>  int security_sb_mount(const char *dev_name, struct path *path,
>>                         const char *type, unsigned long flags, void *data)
>>  {
>> +       if (current_user_ns() != &init_user_ns)
>> +               return -EPERM;
>>         return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
>>  }
> This just makes it impossible to mount from a user namespace. Every
> mount from current_user_ns() != &init_user_ns will fail.
>
>> Then we should push this down into all of the lsms.
>> Then when we should remove or relax or change the check as appropriate
>> in each lsm.
>>
>> The point is this is good enough to see that it is trivially safe,
>> and this allows us to focus on the core issues, and stop worrying about
>> the lsms for a bit.

Given the extent to which LSMs are deployed I find it a bit
worrisome that they might not be considered a "core issue".

>> Then we can focus on each lsm one at at time and take the time to really
>> understand them and talk with their maintainers etc to make certain
>> we get things correct.

The "Do the easy stuff, fix the hard stuff after we've sold the product"
approach works really well until you get to the point of fixing the hard
stuff. This is the origin of the 90/90 rule of software development.

>>
>> This should remove the need for your patches 5, 6 and 7. For the
>> immediate future.
> I'm still not entirely sure what you were trying to do, maybe refuse to
> mount whenever a security module is loaded? I think this could be a good
> option to start, but couldn't we restrict it to only the LSMs which use
> xattrs for security labels? In situations where the filesystem cannot
> supply security policy metadata I can't think of any reason to disallow
> the mounts.

This whole notion of mounting a generic filesystem (e.g. ext4) that
is "owned" by a user (as opposed to the system) has lots of implications,
and I seriously doubt that many of them have been accounted for.

Think back to the "negative group access" issue. You can't just
ignore issues that are inconvenient, or claim that you have a reasonable
system just because *you* can't think of a problem.

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

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

[toc] | [prev] | [next] | [standalone]

#1186193

On Thu, Jul 16, 2015 at 08:09:20AM -0700, Casey Schaufler wrote:
> On 7/16/2015 6:59 AM, Seth Forshee wrote:
> > On Wed, Jul 15, 2015 at 10:15:21PM -0500, Eric W. Biederman wrote:
> >> Seth I think for the LSMs we should start with:
> >>
> >> diff --git a/security/security.c b/security/security.c
> >> index 062f3c997fdc..5b6ece92a8e5 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -310,6 +310,8 @@ int security_sb_statfs(struct dentry *dentry)
> >>  int security_sb_mount(const char *dev_name, struct path *path,
> >>                         const char *type, unsigned long flags, void *data)
> >>  {
> >> +       if (current_user_ns() != &init_user_ns)
> >> +               return -EPERM;
> >>         return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
> >>  }
> > This just makes it impossible to mount from a user namespace. Every
> > mount from current_user_ns() != &init_user_ns will fail.
> >
> >> Then we should push this down into all of the lsms.
> >> Then when we should remove or relax or change the check as appropriate
> >> in each lsm.
> >>
> >> The point is this is good enough to see that it is trivially safe,
> >> and this allows us to focus on the core issues, and stop worrying about
> >> the lsms for a bit.
> 
> Given the extent to which LSMs are deployed I find it a bit
> worrisome that they might not be considered a "core issue".
> 
> >> Then we can focus on each lsm one at at time and take the time to really
> >> understand them and talk with their maintainers etc to make certain
> >> we get things correct.
> 
> The "Do the easy stuff, fix the hard stuff after we've sold the product"
> approach works really well until you get to the point of fixing the hard
> stuff. This is the origin of the 90/90 rule of software development.
> 
> >>
> >> This should remove the need for your patches 5, 6 and 7. For the
> >> immediate future.
> > I'm still not entirely sure what you were trying to do, maybe refuse to
> > mount whenever a security module is loaded? I think this could be a good
> > option to start, but couldn't we restrict it to only the LSMs which use
> > xattrs for security labels? In situations where the filesystem cannot
> > supply security policy metadata I can't think of any reason to disallow
> > the mounts.
> 
> This whole notion of mounting a generic filesystem (e.g. ext4) that
> is "owned" by a user (as opposed to the system) has lots of implications,
> and I seriously doubt that many of them have been accounted for.
> 
> Think back to the "negative group access" issue. You can't just
> ignore issues that are inconvenient, or claim that you have a reasonable
> system just because *you* can't think of a problem.

I've spent a lot of time considering the implications and previous
vulnerabilities, and I've addressed everything I turned up. Now I'm
asking for review from those with more experience with and expertise of
the code in question. I'm not sure what more I should be doing.

I welcome feedback about anything I've missed, but stating generally
that you think I probably missed something isn't very helpful.

The LSM issue is thornier than the rest of it though, which is why I
specifically asked for review there in the cover letter. There's a lot
of complexity and nuance, and I still don't have a grasp on all the
subtleties. One such subtlety is the full impact of simply ignoring the
security labels on disk (but I am still confused as to why this is
different from filesystems which don't support xattrs at all).

I was unaware of Lukasz's patches until yesterday, and I will have a
look at them. But since we don't have the LSM support for user
namespaces yet, I don't see the problem with doing something safe for
LSMs initially and evolving the LSM integration for user ns mounts along
with the rest of the user ns integration.

Your point is taken about my less-than-expert opinion about the other
security modules. We should at minimum get acks from the maintainers of
those modules that unprivileged mounts will not compromise MAC.

For Smack specifically, I believe my only concern was the SMACK64EXEC
attribute, as all the other attributes only affected subjects' access to
the files. So maybe it would be possible to simply ignore this attribute
in unprivileged mounts and respect the others, even lacking more
complete LSM support for user namespaces.

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

[toc] | [prev] | [next] | [standalone]

#1186284

On 7/16/2015 11:57 AM, Seth Forshee wrote:
> On Thu, Jul 16, 2015 at 08:09:20AM -0700, Casey Schaufler wrote:
>> On 7/16/2015 6:59 AM, Seth Forshee wrote:
>>> On Wed, Jul 15, 2015 at 10:15:21PM -0500, Eric W. Biederman wrote:
>>>> Seth I think for the LSMs we should start with:
>>>>
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 062f3c997fdc..5b6ece92a8e5 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -310,6 +310,8 @@ int security_sb_statfs(struct dentry *dentry)
>>>>  int security_sb_mount(const char *dev_name, struct path *path,
>>>>                         const char *type, unsigned long flags, void *data)
>>>>  {
>>>> +       if (current_user_ns() != &init_user_ns)
>>>> +               return -EPERM;
>>>>         return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
>>>>  }
>>> This just makes it impossible to mount from a user namespace. Every
>>> mount from current_user_ns() != &init_user_ns will fail.
>>>
>>>> Then we should push this down into all of the lsms.
>>>> Then when we should remove or relax or change the check as appropriate
>>>> in each lsm.
>>>>
>>>> The point is this is good enough to see that it is trivially safe,
>>>> and this allows us to focus on the core issues, and stop worrying about
>>>> the lsms for a bit.
>> Given the extent to which LSMs are deployed I find it a bit
>> worrisome that they might not be considered a "core issue".
>>
>>>> Then we can focus on each lsm one at at time and take the time to really
>>>> understand them and talk with their maintainers etc to make certain
>>>> we get things correct.
>> The "Do the easy stuff, fix the hard stuff after we've sold the product"
>> approach works really well until you get to the point of fixing the hard
>> stuff. This is the origin of the 90/90 rule of software development.
>>
>>>> This should remove the need for your patches 5, 6 and 7. For the
>>>> immediate future.
>>> I'm still not entirely sure what you were trying to do, maybe refuse to
>>> mount whenever a security module is loaded? I think this could be a good
>>> option to start, but couldn't we restrict it to only the LSMs which use
>>> xattrs for security labels? In situations where the filesystem cannot
>>> supply security policy metadata I can't think of any reason to disallow
>>> the mounts.
>> This whole notion of mounting a generic filesystem (e.g. ext4) that
>> is "owned" by a user (as opposed to the system) has lots of implications,
>> and I seriously doubt that many of them have been accounted for.
>>
>> Think back to the "negative group access" issue. You can't just
>> ignore issues that are inconvenient, or claim that you have a reasonable
>> system just because *you* can't think of a problem.
> I've spent a lot of time considering the implications and previous
> vulnerabilities, and I've addressed everything I turned up. Now I'm
> asking for review from those with more experience with and expertise of
> the code in question. I'm not sure what more I should be doing.

Part of the problem I see is that you're looking at the details
when there's an architectural issue. That's OK, it happens all
the time, but we have to pull the issue up slightly higher in
order to address the underlying difficulties.

You want to provide a mechanism whereby an unprivileged user (Seth)
can mount a filesystem for his own use. You want full filesystem
semantics, but you're willing to accept restrictions on certain
filesystem features to avoid opening security holes. You are not
willing to accept restrictions that make the filesystem unusable,
such as making it read-only.

I am going to present a suggestion. Feel free to correct my
assumptions and my reasoning. For simplicity let's use loop-back
mounting of a filesystem contained in a file as an example. The
principles should apply to newly created memory based filesystems
or disk partitions "owned" by Seth.

Seth wants to mount a file (~seth/myfs) which contains an ext4
filesystem. There is already a filesystem object, with security
attributes, that the system knows how to deal with. If Seth mounts
this as a filesystem he, and potentially other people, will be
able to access the content of this object without accessing the
object itself.

	seth$ mount --justforme -t ext4 ~seth/myfs /tmp/seth
	seth$ chmod 777 /tmp/seth
	seth$ ls -la /tmp/seth
	drwxrwxrwx.  3  seth     seth   260 Jul 16 12:59 .
	drwxrwxrwxt 18  root     root  4069 Jul 16 11:13 ..
	seth$

Everything's fine at this point. Wilma is also using the system,
being the sort who likes to hide things in out of the way places

	wilma$ cp ~/scandals /tmp/seth
	wilma$ chmod 600 /tmp/seth/scandals

puts her list of scandals on the unsuspecting filesystem, and changes
the mode to ensure that no one can find out what went on after the
office party.

Seth unmounts /tmp/seth. He looks in ~seth/myfs, finds out what really
happened at the office party, and the story goes from there.

Wilma did everything correctly according to the system security policy,
but the system security policy did not protect her as advertised. The
system was tricked into behaving as if it was in control of the content
of the filesystem when in fact it was not.

One way to fix this problem is for unprivileged mounts to recognize the
attributes of the object mounted and to propagate those attributes to all
the objects they present. All files on /tmp/seth would be owned by seth
and protected by the mode bits, ACL and LSM requirements of ~/seth/myfs.
opening a file on /tmp/seth would require the same permissions as opening
the file containing the mounted filesystem. These attributes would have to
be immutable, or at least demonstrably more restrictive (chmod might be
allowed in some cases, but chown would never be) when changed. I don't see
how a user other than seth could create a new file, as you'd either have
a magical change in ownership or a false sense of security.

I don't see that the presence of user namespaces changes anything. You
may reduce the set of uids available, but the problems with putting a
uid into someone else's file is just as real.

> I welcome feedback about anything I've missed, but stating generally
> that you think I probably missed something isn't very helpful.

True enough. I hope I've explained myself above.

> The LSM issue is thornier than the rest of it though, which is why I
> specifically asked for review there in the cover letter. There's a lot
> of complexity and nuance, and I still don't have a grasp on all the
> subtleties. One such subtlety is the full impact of simply ignoring the
> security labels on disk (but I am still confused as to why this is
> different from filesystems which don't support xattrs at all).

If you can mount a filesystem such that the labels are ignored you
are effectively specifying that the Smack label on the files be 
determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
Without it, it's not.

> I was unaware of Lukasz's patches until yesterday, and I will have a
> look at them. But since we don't have the LSM support for user
> namespaces yet, I don't see the problem with doing something safe for
> LSMs initially and evolving the LSM integration for user ns mounts along
> with the rest of the user ns integration.

Ignoring the security attributes is not safe!

> Your point is taken about my less-than-expert opinion about the other
> security modules. We should at minimum get acks from the maintainers of
> those modules that unprivileged mounts will not compromise MAC.

I am the Smack maintainer. Unprivileged mounts as you have
described them compromise MAC. They compromise DAC, too.


> For Smack specifically, I believe my only concern was the SMACK64EXEC
> attribute, as all the other attributes only affected subjects' access to
> the files. So maybe it would be possible to simply ignore this attribute
> in unprivileged mounts and respect the others, even lacking more
> complete LSM support for user namespaces.

SMACK64EXEC is analogous to the setuid bit, but I would rather see
exec() of programs with this attribute refused that for it to be
blindly ignored.

> Seth
>

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

[toc] | [prev] | [next] | [standalone]

#1186299

On Thu, Jul 16, 2015 at 2:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> You want to provide a mechanism whereby an unprivileged user (Seth)
> can mount a filesystem for his own use. You want full filesystem
> semantics, but you're willing to accept restrictions on certain
> filesystem features to avoid opening security holes. You are not
> willing to accept restrictions that make the filesystem unusable,
> such as making it read-only.
>
> I am going to present a suggestion. Feel free to correct my
> assumptions and my reasoning. For simplicity let's use loop-back
> mounting of a filesystem contained in a file as an example. The
> principles should apply to newly created memory based filesystems
> or disk partitions "owned" by Seth.
>
> Seth wants to mount a file (~seth/myfs) which contains an ext4
> filesystem. There is already a filesystem object, with security
> attributes, that the system knows how to deal with. If Seth mounts
> this as a filesystem he, and potentially other people, will be
> able to access the content of this object without accessing the
> object itself.
>
>         seth$ mount --justforme -t ext4 ~seth/myfs /tmp/seth
>         seth$ chmod 777 /tmp/seth
>         seth$ ls -la /tmp/seth
>         drwxrwxrwx.  3  seth     seth   260 Jul 16 12:59 .
>         drwxrwxrwxt 18  root     root  4069 Jul 16 11:13 ..
>         seth$
>
> Everything's fine at this point. Wilma is also using the system,
> being the sort who likes to hide things in out of the way places
>
>         wilma$ cp ~/scandals /tmp/seth
>         wilma$ chmod 600 /tmp/seth/scandals

This is already impossible as described.  Seth can only mount the
filesystem in a private mount namespace inside a user namespace that
he created.  Wilma can't see it unless Seth passes an fd to Wilma and
Wilma accepts and uses it.

>
> puts her list of scandals on the unsuspecting filesystem, and changes
> the mode to ensure that no one can find out what went on after the
> office party.
>
> Seth unmounts /tmp/seth. He looks in ~seth/myfs, finds out what really
> happened at the office party, and the story goes from there.
>
> Wilma did everything correctly according to the system security policy,
> but the system security policy did not protect her as advertised. The
> system was tricked into behaving as if it was in control of the content
> of the filesystem when in fact it was not.


I would argue that, if Wilma writes to some place described by an fd
and doesn't verify where she's writing to, then she has no expectation
of privacy.  After all, she could just *tell* Seth directly whatever
she wants (assuming she can communicate with Seth in the first place).

>
> One way to fix this problem is for unprivileged mounts to recognize the
> attributes of the object mounted and to propagate those attributes to all
> the objects they present. All files on /tmp/seth would be owned by seth
> and protected by the mode bits, ACL and LSM requirements of ~/seth/myfs.

This is impossible to enforce, because Seth could use FUSE instead of ext4.

> opening a file on /tmp/seth would require the same permissions as opening
> the file containing the mounted filesystem. These attributes would have to
> be immutable, or at least demonstrably more restrictive (chmod might be
> allowed in some cases, but chown would never be) when changed. I don't see
> how a user other than seth could create a new file, as you'd either have
> a magical change in ownership or a false sense of security.

This would be a very harsh restriction.  Seth might legitimately want
to give a user access to a file on backing store he owns without
giving that user access to the backing store.  Root on a normal system
does that all the time.

> If you can mount a filesystem such that the labels are ignored you
> are effectively specifying that the Smack label on the files be
> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
> Without it, it's not.

Can you explain what the threat model is here?  I don't see what it is
that you're trying to prevent.

>> Your point is taken about my less-than-expert opinion about the other
>> security modules. We should at minimum get acks from the maintainers of
>> those modules that unprivileged mounts will not compromise MAC.
>
> I am the Smack maintainer. Unprivileged mounts as you have
> described them compromise MAC. They compromise DAC, too.
>

How do they compromise DAC?

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

[toc] | [prev] | [next] | [standalone]

#1186310

On 7/16/2015 3:27 PM, Andy Lutomirski wrote:
> On Thu, Jul 16, 2015 at 2:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> You want to provide a mechanism whereby an unprivileged user (Seth)
>> can mount a filesystem for his own use. You want full filesystem
>> semantics, but you're willing to accept restrictions on certain
>> filesystem features to avoid opening security holes. You are not
>> willing to accept restrictions that make the filesystem unusable,
>> such as making it read-only.
>>
>> I am going to present a suggestion. Feel free to correct my
>> assumptions and my reasoning. For simplicity let's use loop-back
>> mounting of a filesystem contained in a file as an example. The
>> principles should apply to newly created memory based filesystems
>> or disk partitions "owned" by Seth.
>>
>> Seth wants to mount a file (~seth/myfs) which contains an ext4
>> filesystem. There is already a filesystem object, with security
>> attributes, that the system knows how to deal with. If Seth mounts
>> this as a filesystem he, and potentially other people, will be
>> able to access the content of this object without accessing the
>> object itself.
>>
>>         seth$ mount --justforme -t ext4 ~seth/myfs /tmp/seth
>>         seth$ chmod 777 /tmp/seth
>>         seth$ ls -la /tmp/seth
>>         drwxrwxrwx.  3  seth     seth   260 Jul 16 12:59 .
>>         drwxrwxrwxt 18  root     root  4069 Jul 16 11:13 ..
>>         seth$
>>
>> Everything's fine at this point. Wilma is also using the system,
>> being the sort who likes to hide things in out of the way places
>>
>>         wilma$ cp ~/scandals /tmp/seth
>>         wilma$ chmod 600 /tmp/seth/scandals
> This is already impossible as described.  Seth can only mount the
> filesystem in a private mount namespace inside a user namespace that
> he created.  Wilma can't see it unless Seth passes an fd to Wilma and
> Wilma accepts and uses it.

But you do have multiple UIDs withing your user namespace, right?
There are processes running as someone other than seth, right?

>
>> puts her list of scandals on the unsuspecting filesystem, and changes
>> the mode to ensure that no one can find out what went on after the
>> office party.
>>
>> Seth unmounts /tmp/seth. He looks in ~seth/myfs, finds out what really
>> happened at the office party, and the story goes from there.
>>
>> Wilma did everything correctly according to the system security policy,
>> but the system security policy did not protect her as advertised. The
>> system was tricked into behaving as if it was in control of the content
>> of the filesystem when in fact it was not.
>
> I would argue that, if Wilma writes to some place described by an fd
> and doesn't verify where she's writing to, then she has no expectation
> of privacy.  After all, she could just *tell* Seth directly whatever
> she wants (assuming she can communicate with Seth in the first place).

Don't ascribe either wisdom or good intentions to Wilma.

>> One way to fix this problem is for unprivileged mounts to recognize the
>> attributes of the object mounted and to propagate those attributes to all
>> the objects they present. All files on /tmp/seth would be owned by seth
>> and protected by the mode bits, ACL and LSM requirements of ~/seth/myfs.
> This is impossible to enforce, because Seth could use FUSE instead of ext4.

I never said that things aren't already broken. And, if you want
to ignore the potential DAC issues (read, negative groups) just
do it for the LSM xattrs.


>
>> opening a file on /tmp/seth would require the same permissions as opening
>> the file containing the mounted filesystem. These attributes would have to
>> be immutable, or at least demonstrably more restrictive (chmod might be
>> allowed in some cases, but chown would never be) when changed. I don't see
>> how a user other than seth could create a new file, as you'd either have
>> a magical change in ownership or a false sense of security.
> This would be a very harsh restriction.  Seth might legitimately want
> to give a user access to a file on backing store he owns without
> giving that user access to the backing store.  Root on a normal system
> does that all the time.

You already said that it was impossible for Wilma to get
access, so how is this more restrictive? Besides, Seth can
always set the mode on ~/seth so that Wilma can't read the
files it contains. This isn't an old problem or a novel
solution.

>> If you can mount a filesystem such that the labels are ignored you
>> are effectively specifying that the Smack label on the files be
>> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
>> Without it, it's not.
> Can you explain what the threat model is here?  I don't see what it is
> that you're trying to prevent.

Um, OK.
The filesystem has files with a hundred different Smack labels on it.
I mount it as an unlabeled filesystem and everything is readable by
everyone. Bad jojo.

>
>>> Your point is taken about my less-than-expert opinion about the other
>>> security modules. We should at minimum get acks from the maintainers of
>>> those modules that unprivileged mounts will not compromise MAC.
>> I am the Smack maintainer. Unprivileged mounts as you have
>> described them compromise MAC. They compromise DAC, too.
>>
> How do they compromise DAC?

Wilma's expectation (or the application running with a mapped UID)
that chmod will keep Seth out of the file.

> --Andy
>

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

[toc] | [prev] | [next] | [standalone]

#1186323

On Thu, Jul 16, 2015 at 4:08 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/16/2015 3:27 PM, Andy Lutomirski wrote:
>> On Thu, Jul 16, 2015 at 2:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> You want to provide a mechanism whereby an unprivileged user (Seth)
>>> can mount a filesystem for his own use. You want full filesystem
>>> semantics, but you're willing to accept restrictions on certain
>>> filesystem features to avoid opening security holes. You are not
>>> willing to accept restrictions that make the filesystem unusable,
>>> such as making it read-only.
>>>
>>> I am going to present a suggestion. Feel free to correct my
>>> assumptions and my reasoning. For simplicity let's use loop-back
>>> mounting of a filesystem contained in a file as an example. The
>>> principles should apply to newly created memory based filesystems
>>> or disk partitions "owned" by Seth.
>>>
>>> Seth wants to mount a file (~seth/myfs) which contains an ext4
>>> filesystem. There is already a filesystem object, with security
>>> attributes, that the system knows how to deal with. If Seth mounts
>>> this as a filesystem he, and potentially other people, will be
>>> able to access the content of this object without accessing the
>>> object itself.
>>>
>>>         seth$ mount --justforme -t ext4 ~seth/myfs /tmp/seth
>>>         seth$ chmod 777 /tmp/seth
>>>         seth$ ls -la /tmp/seth
>>>         drwxrwxrwx.  3  seth     seth   260 Jul 16 12:59 .
>>>         drwxrwxrwxt 18  root     root  4069 Jul 16 11:13 ..
>>>         seth$
>>>
>>> Everything's fine at this point. Wilma is also using the system,
>>> being the sort who likes to hide things in out of the way places
>>>
>>>         wilma$ cp ~/scandals /tmp/seth
>>>         wilma$ chmod 600 /tmp/seth/scandals
>> This is already impossible as described.  Seth can only mount the
>> filesystem in a private mount namespace inside a user namespace that
>> he created.  Wilma can't see it unless Seth passes an fd to Wilma and
>> Wilma accepts and uses it.
>
> But you do have multiple UIDs withing your user namespace, right?
> There are processes running as someone other than seth, right?
>

Only if root set it up that way.  For example, root could set up
"subuids" (this is a userspace concept) that belong to Seth.  These
would be uids that Seth controls and that represent subsets of Seth's
authority. Wilma wouldn't be one of these subuids unless she was
somehow part of Seth (or if root completely screwed up).

>>
>>> puts her list of scandals on the unsuspecting filesystem, and changes
>>> the mode to ensure that no one can find out what went on after the
>>> office party.
>>>
>>> Seth unmounts /tmp/seth. He looks in ~seth/myfs, finds out what really
>>> happened at the office party, and the story goes from there.
>>>
>>> Wilma did everything correctly according to the system security policy,
>>> but the system security policy did not protect her as advertised. The
>>> system was tricked into behaving as if it was in control of the content
>>> of the filesystem when in fact it was not.
>>
>> I would argue that, if Wilma writes to some place described by an fd
>> and doesn't verify where she's writing to, then she has no expectation
>> of privacy.  After all, she could just *tell* Seth directly whatever
>> she wants (assuming she can communicate with Seth in the first place).
>
> Don't ascribe either wisdom or good intentions to Wilma.

In that case, I'll mention the futility of solving the problem, even
without user namespaces.  If Wilma tells Seth something, he's going to
find out.  If Wilma pokes it (in whatever form) into an fd provided by
Seth, then Seth is extremely likely to find out, regardless of what
root or the MAC owner tries to do.

If Wilma writes to a path that's mounted in her namespace, then, sure,
overall policy associated with her namespace (which, in your example,
is the root namespace) must apply.  But Seth can't mount things into
Wilma's namespace without having CAP_SYS_ADMIN in that namespace and,
if he has CAP_SYS_ADMIN, it's already game over.

>
>>> One way to fix this problem is for unprivileged mounts to recognize the
>>> attributes of the object mounted and to propagate those attributes to all
>>> the objects they present. All files on /tmp/seth would be owned by seth
>>> and protected by the mode bits, ACL and LSM requirements of ~/seth/myfs.
>> This is impossible to enforce, because Seth could use FUSE instead of ext4.
>
> I never said that things aren't already broken. And, if you want
> to ignore the potential DAC issues (read, negative groups) just
> do it for the LSM xattrs.
>

Negative groups are a solved problem, I believe.

>
>>
>>> opening a file on /tmp/seth would require the same permissions as opening
>>> the file containing the mounted filesystem. These attributes would have to
>>> be immutable, or at least demonstrably more restrictive (chmod might be
>>> allowed in some cases, but chown would never be) when changed. I don't see
>>> how a user other than seth could create a new file, as you'd either have
>>> a magical change in ownership or a false sense of security.
>> This would be a very harsh restriction.  Seth might legitimately want
>> to give a user access to a file on backing store he owns without
>> giving that user access to the backing store.  Root on a normal system
>> does that all the time.
>
> You already said that it was impossible for Wilma to get
> access, so how is this more restrictive? Besides, Seth can
> always set the mode on ~/seth so that Wilma can't read the
> files it contains. This isn't an old problem or a novel
> solution.

Seth can pass an fd around.  This is actually a plausible thing to do:
Seth creates a userns to sandbox himself, mounts some FUSE thing in
there, and passes an fd out for the benefit of some daemon.  That
daemon had better validate the thing before using it, though.

I really don't see the benefit of making up extra rules that apply to
users outside a userns who try to access specifically a filesystem
with backing store.  They wouldn't make sense for filesystems without
backing store.

>
>>> If you can mount a filesystem such that the labels are ignored you
>>> are effectively specifying that the Smack label on the files be
>>> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
>>> Without it, it's not.
>> Can you explain what the threat model is here?  I don't see what it is
>> that you're trying to prevent.
>
> Um, OK.
> The filesystem has files with a hundred different Smack labels on it.
> I mount it as an unlabeled filesystem and everything is readable by
> everyone. Bad jojo.

I still don't understand.  If it's a filesystem backed by a file that
Seth has RW access to, then Seth can read everything on it, full stop.
The security labels in the filesystem are irrelevant.

This is like saying that, if you put restrictive labels in the
filesystem that lives on /dev/sda2 and give Seth ownership of
/dev/sda2, then you expect Seth to be unable to bypass the policy
specifies by your labels.

Or maybe I'm misunderstanding you.

>
>>
>>>> Your point is taken about my less-than-expert opinion about the other
>>>> security modules. We should at minimum get acks from the maintainers of
>>>> those modules that unprivileged mounts will not compromise MAC.
>>> I am the Smack maintainer. Unprivileged mounts as you have
>>> described them compromise MAC. They compromise DAC, too.
>>>
>> How do they compromise DAC?
>
> Wilma's expectation (or the application running with a mapped UID)
> that chmod will keep Seth out of the file.

That was never true.  If Seth has an open fd, Wilma can chmod all day
and it won't matter.  In this example, Seth owns the entire filesystem
along with its backing store.

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

[toc] | [prev] | [next] | [standalone]

#1186351

On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
> On Thu, Jul 16, 2015 at 4:08 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 7/16/2015 3:27 PM, Andy Lutomirski wrote:
>>> On Thu, Jul 16, 2015 at 2:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> You want to provide a mechanism whereby an unprivileged user (Seth)
>>>> can mount a filesystem for his own use. You want full filesystem
>>>> semantics, but you're willing to accept restrictions on certain
>>>> filesystem features to avoid opening security holes. You are not
>>>> willing to accept restrictions that make the filesystem unusable,
>>>> such as making it read-only.
>>>>
>>>> I am going to present a suggestion. Feel free to correct my
>>>> assumptions and my reasoning. For simplicity let's use loop-back
>>>> mounting of a filesystem contained in a file as an example. The
>>>> principles should apply to newly created memory based filesystems
>>>> or disk partitions "owned" by Seth.
>>>>
>>>> Seth wants to mount a file (~seth/myfs) which contains an ext4
>>>> filesystem. There is already a filesystem object, with security
>>>> attributes, that the system knows how to deal with. If Seth mounts
>>>> this as a filesystem he, and potentially other people, will be
>>>> able to access the content of this object without accessing the
>>>> object itself.
>>>>
>>>>         seth$ mount --justforme -t ext4 ~seth/myfs /tmp/seth
>>>>         seth$ chmod 777 /tmp/seth
>>>>         seth$ ls -la /tmp/seth
>>>>         drwxrwxrwx.  3  seth     seth   260 Jul 16 12:59 .
>>>>         drwxrwxrwxt 18  root     root  4069 Jul 16 11:13 ..
>>>>         seth$
>>>>
>>>> Everything's fine at this point. Wilma is also using the system,
>>>> being the sort who likes to hide things in out of the way places
>>>>
>>>>         wilma$ cp ~/scandals /tmp/seth
>>>>         wilma$ chmod 600 /tmp/seth/scandals
>>> This is already impossible as described.  Seth can only mount the
>>> filesystem in a private mount namespace inside a user namespace that
>>> he created.  Wilma can't see it unless Seth passes an fd to Wilma and
>>> Wilma accepts and uses it.
>> But you do have multiple UIDs withing your user namespace, right?
>> There are processes running as someone other than seth, right?
>>
> Only if root set it up that way.  For example, root could set up
> "subuids" (this is a userspace concept) that belong to Seth.  These
> would be uids that Seth controls and that represent subsets of Seth's
> authority. Wilma wouldn't be one of these subuids unless she was
> somehow part of Seth (or if root completely screwed up).

Or if root had some really unexpected and inappropriate ideas
on what qualifies as "clever". But I'll back off. It looks like
this particular objection of mine is covered.

>
>>>> puts her list of scandals on the unsuspecting filesystem, and changes
>>>> the mode to ensure that no one can find out what went on after the
>>>> office party.
>>>>
>>>> Seth unmounts /tmp/seth. He looks in ~seth/myfs, finds out what really
>>>> happened at the office party, and the story goes from there.
>>>>
>>>> Wilma did everything correctly according to the system security policy,
>>>> but the system security policy did not protect her as advertised. The
>>>> system was tricked into behaving as if it was in control of the content
>>>> of the filesystem when in fact it was not.
>>> I would argue that, if Wilma writes to some place described by an fd
>>> and doesn't verify where she's writing to, then she has no expectation
>>> of privacy.  After all, she could just *tell* Seth directly whatever
>>> she wants (assuming she can communicate with Seth in the first place).
>> Don't ascribe either wisdom or good intentions to Wilma.
> In that case, I'll mention the futility of solving the problem, even
> without user namespaces.  If Wilma tells Seth something, he's going to
> find out.  If Wilma pokes it (in whatever form) into an fd provided by
> Seth, then Seth is extremely likely to find out, regardless of what
> root or the MAC owner tries to do.

I'll buy that, too. I still get queasy every time someone
tells me that passing file descriptors is a security feature.

> If Wilma writes to a path that's mounted in her namespace, then, sure,
> overall policy associated with her namespace (which, in your example,
> is the root namespace) must apply.  But Seth can't mount things into
> Wilma's namespace without having CAP_SYS_ADMIN in that namespace and,
> if he has CAP_SYS_ADMIN, it's already game over.

And so long as it's restricted to the namespace ...
I'm starting to get it now.

>>>> One way to fix this problem is for unprivileged mounts to recognize the
>>>> attributes of the object mounted and to propagate those attributes to all
>>>> the objects they present. All files on /tmp/seth would be owned by seth
>>>> and protected by the mode bits, ACL and LSM requirements of ~/seth/myfs.
>>> This is impossible to enforce, because Seth could use FUSE instead of ext4.
>> I never said that things aren't already broken. And, if you want
>> to ignore the potential DAC issues (read, negative groups) just
>> do it for the LSM xattrs.
>>
> Negative groups are a solved problem, I believe.

My position is that there's a workaround but that the
design is still fundamentally flawed. 

>
>>>> opening a file on /tmp/seth would require the same permissions as opening
>>>> the file containing the mounted filesystem. These attributes would have to
>>>> be immutable, or at least demonstrably more restrictive (chmod might be
>>>> allowed in some cases, but chown would never be) when changed. I don't see
>>>> how a user other than seth could create a new file, as you'd either have
>>>> a magical change in ownership or a false sense of security.
>>> This would be a very harsh restriction.  Seth might legitimately want
>>> to give a user access to a file on backing store he owns without
>>> giving that user access to the backing store.  Root on a normal system
>>> does that all the time.
>> You already said that it was impossible for Wilma to get
>> access, so how is this more restrictive? Besides, Seth can
>> always set the mode on ~/seth so that Wilma can't read the
>> files it contains. This isn't an old problem or a novel
>> solution.
> Seth can pass an fd around.  This is actually a plausible thing to do:
> Seth creates a userns to sandbox himself, mounts some FUSE thing in
> there, and passes an fd out for the benefit of some daemon.  That
> daemon had better validate the thing before using it, though.

Point. It won't, but it should.

> I really don't see the benefit of making up extra rules that apply to
> users outside a userns who try to access specifically a filesystem
> with backing store.  They wouldn't make sense for filesystems without
> backing store.

Sure it would. For Smack, it would be the label a file would be
created with, which would be the label of the process creating
the memory based filesystem. For SELinux the rules are more a
touch more sophisticated, but I'm sure that Paul or Stephen could
come up with how to determine it.

The point, looping all the way back to the beginning, where we
were talking about just ignoring the labels on the filesystem,
is that if you use the same Smack label on the files in the
filesystem as the backing store file has, we'll all be happy.
If that label isn't something user can write to, he won't be
able to write to the mounted objects, either. If there is no
backing store then use the label of the process creating the
filesystem, which will be the user, which will mean everything
will work hunky dory.

Yes, there's work involved, but I doubt there's a lot. Getting
the label from the backing store or the creating process is
simple enough.


>>>> If you can mount a filesystem such that the labels are ignored you
>>>> are effectively specifying that the Smack label on the files be
>>>> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
>>>> Without it, it's not.
>>> Can you explain what the threat model is here?  I don't see what it is
>>> that you're trying to prevent.
>> Um, OK.
>> The filesystem has files with a hundred different Smack labels on it.
>> I mount it as an unlabeled filesystem and everything is readable by
>> everyone. Bad jojo.
> I still don't understand.  If it's a filesystem backed by a file that
> Seth has RW access to, then Seth can read everything on it, full stop.
> The security labels in the filesystem are irrelevant.

Well, they can't be trusted, if that's what you mean.
That's why I'm saying that the objects exposed by mounting
this backing store need to be treated with the same security
attributes as the backing store. Fudge it for DAC if you are
so inclined, but I think it's the right way to go for MAC.

> This is like saying that, if you put restrictive labels in the
> filesystem that lives on /dev/sda2 and give Seth ownership of
> /dev/sda2, then you expect Seth to be unable to bypass the policy
> specifies by your labels.

Consider the Smack label on /dev/sda2. Smack does not care
who owns it, just what the Smack label is. Just like on
~/seth/myfs. The backing store "object" is /dev/sda2 in the
one case, ~/seth/myfs in the other, and something in the ether
for a memory based filesystem. So long as the labels of the
files exposed on the mount point match those of the backing
store "object", Smack is going to be happy. Since you're
running without privilege, you can't change the labels on
the files.

Now Seth, being the sneaky person that he is, could change
the Smack labels on the files in the backing store while it's
offline. Since he has access to the backing store, he can't
give himself more access by changing the labels within the
filesystem. He can give himself less, but I'm OK with that.

> Or maybe I'm misunderstanding you.

Probably, but I'm undoubtedly doing the same.

If you're going to be at LinuxCon in Seattle we should
continue this discussion over the beverage of your choice.

>>>>> Your point is taken about my less-than-expert opinion about the other
>>>>> security modules. We should at minimum get acks from the maintainers of
>>>>> those modules that unprivileged mounts will not compromise MAC.
>>>> I am the Smack maintainer. Unprivileged mounts as you have
>>>> described them compromise MAC. They compromise DAC, too.
>>>>
>>> How do they compromise DAC?
>> Wilma's expectation (or the application running with a mapped UID)
>> that chmod will keep Seth out of the file.
> That was never true.  If Seth has an open fd, Wilma can chmod all day
> and it won't matter.  In this example, Seth owns the entire filesystem
> along with its backing store.
>
> --Andy
> --
> 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/
>

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

[toc] | [prev] | [next] | [standalone]

#1186352

On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
>> I really don't see the benefit of making up extra rules that apply to
>> users outside a userns who try to access specifically a filesystem
>> with backing store.  They wouldn't make sense for filesystems without
>> backing store.
>
> Sure it would. For Smack, it would be the label a file would be
> created with, which would be the label of the process creating
> the memory based filesystem. For SELinux the rules are more a
> touch more sophisticated, but I'm sure that Paul or Stephen could
> come up with how to determine it.
>
> The point, looping all the way back to the beginning, where we
> were talking about just ignoring the labels on the filesystem,
> is that if you use the same Smack label on the files in the
> filesystem as the backing store file has, we'll all be happy.
> If that label isn't something user can write to, he won't be
> able to write to the mounted objects, either. If there is no
> backing store then use the label of the process creating the
> filesystem, which will be the user, which will mean everything
> will work hunky dory.
>
> Yes, there's work involved, but I doubt there's a lot. Getting
> the label from the backing store or the creating process is
> simple enough.
>

So what if Smack used the label of the user creating the filesystem
even for filesystems with backing store?  IMO this ought to be doable
with the LSM hooks -- it certainly seems reasonable for the LSM to be
aware of who created a filesystem.  In fact, I'd argue that if Smack
can't do this with the proposed LSM hooks, then the hooks are
insufficient.

Presumably Smack could also figure out what was mounted, but keep in
mind that there are filesystems like ntfs-3g out there.  While ntfs-3g
logically has backing store, I don't think the kernel actually knows
about it.

>
>>>>> If you can mount a filesystem such that the labels are ignored you
>>>>> are effectively specifying that the Smack label on the files be
>>>>> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
>>>>> Without it, it's not.
>>>> Can you explain what the threat model is here?  I don't see what it is
>>>> that you're trying to prevent.
>>> Um, OK.
>>> The filesystem has files with a hundred different Smack labels on it.
>>> I mount it as an unlabeled filesystem and everything is readable by
>>> everyone. Bad jojo.
>> I still don't understand.  If it's a filesystem backed by a file that
>> Seth has RW access to, then Seth can read everything on it, full stop.
>> The security labels in the filesystem are irrelevant.
>
> Well, they can't be trusted, if that's what you mean.
> That's why I'm saying that the objects exposed by mounting
> this backing store need to be treated with the same security
> attributes as the backing store. Fudge it for DAC if you are
> so inclined, but I think it's the right way to go for MAC.
>
>> This is like saying that, if you put restrictive labels in the
>> filesystem that lives on /dev/sda2 and give Seth ownership of
>> /dev/sda2, then you expect Seth to be unable to bypass the policy
>> specifies by your labels.
>
> Consider the Smack label on /dev/sda2. Smack does not care
> who owns it, just what the Smack label is. Just like on
> ~/seth/myfs. The backing store "object" is /dev/sda2 in the
> one case, ~/seth/myfs in the other, and something in the ether
> for a memory based filesystem. So long as the labels of the
> files exposed on the mount point match those of the backing
> store "object", Smack is going to be happy. Since you're
> running without privilege, you can't change the labels on
> the files.
>
> Now Seth, being the sneaky person that he is, could change
> the Smack labels on the files in the backing store while it's
> offline. Since he has access to the backing store, he can't
> give himself more access by changing the labels within the
> filesystem. He can give himself less, but I'm OK with that.
>
>> Or maybe I'm misunderstanding you.
>
> Probably, but I'm undoubtedly doing the same.
>
> If you're going to be at LinuxCon in Seattle we should
> continue this discussion over the beverage of your choice.

There's a small but not quite zero chance I'll be there.  I'll
probably be in Seoul.  It's too bad that LSS and KS are in different
places this year.

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

[toc] | [prev] | [next] | [standalone]

#1186867

On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
> >> I really don't see the benefit of making up extra rules that apply to
> >> users outside a userns who try to access specifically a filesystem
> >> with backing store.  They wouldn't make sense for filesystems without
> >> backing store.
> >
> > Sure it would. For Smack, it would be the label a file would be
> > created with, which would be the label of the process creating
> > the memory based filesystem. For SELinux the rules are more a
> > touch more sophisticated, but I'm sure that Paul or Stephen could
> > come up with how to determine it.
> >
> > The point, looping all the way back to the beginning, where we
> > were talking about just ignoring the labels on the filesystem,
> > is that if you use the same Smack label on the files in the
> > filesystem as the backing store file has, we'll all be happy.
> > If that label isn't something user can write to, he won't be
> > able to write to the mounted objects, either. If there is no
> > backing store then use the label of the process creating the
> > filesystem, which will be the user, which will mean everything
> > will work hunky dory.
> >
> > Yes, there's work involved, but I doubt there's a lot. Getting
> > the label from the backing store or the creating process is
> > simple enough.
> >
> 
> So what if Smack used the label of the user creating the filesystem
> even for filesystems with backing store?  IMO this ought to be doable

The more usual LSM-ish way to handle this would be to ask the LSM, at
mount time, with a new security_mount_bdev_in_userns() hook, passing
it the user's label and the backing store's label (if any), and storing
the label to be used for the files.  Even more LSM-ish (though risking
performance hit) would be to then have the LSM at each inode_init_security
decide whether to use that label or trust what's in the fs anyway (or
do something else).  That could allow the LSM to use policy to decide
that.

Because I don't know that for all LSMs it makes sense for a 'subject'
label to be assigned to an object.

> with the LSM hooks -- it certainly seems reasonable for the LSM to be
> aware of who created a filesystem.  In fact, I'd argue that if Smack
> can't do this with the proposed LSM hooks, then the hooks are
> insufficient.
> 
> Presumably Smack could also figure out what was mounted, but keep in
> mind that there are filesystems like ntfs-3g out there.  While ntfs-3g
> logically has backing store, I don't think the kernel actually knows
> about it.
> 
> >
> >>>>> If you can mount a filesystem such that the labels are ignored you
> >>>>> are effectively specifying that the Smack label on the files be
> >>>>> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
> >>>>> Without it, it's not.
> >>>> Can you explain what the threat model is here?  I don't see what it is
> >>>> that you're trying to prevent.
> >>> Um, OK.
> >>> The filesystem has files with a hundred different Smack labels on it.
> >>> I mount it as an unlabeled filesystem and everything is readable by
> >>> everyone. Bad jojo.
> >> I still don't understand.  If it's a filesystem backed by a file that
> >> Seth has RW access to, then Seth can read everything on it, full stop.
> >> The security labels in the filesystem are irrelevant.
> >
> > Well, they can't be trusted, if that's what you mean.
> > That's why I'm saying that the objects exposed by mounting
> > this backing store need to be treated with the same security
> > attributes as the backing store. Fudge it for DAC if you are
> > so inclined, but I think it's the right way to go for MAC.
> >
> >> This is like saying that, if you put restrictive labels in the
> >> filesystem that lives on /dev/sda2 and give Seth ownership of
> >> /dev/sda2, then you expect Seth to be unable to bypass the policy
> >> specifies by your labels.
> >
> > Consider the Smack label on /dev/sda2. Smack does not care
> > who owns it, just what the Smack label is. Just like on
> > ~/seth/myfs. The backing store "object" is /dev/sda2 in the
> > one case, ~/seth/myfs in the other, and something in the ether
> > for a memory based filesystem. So long as the labels of the
> > files exposed on the mount point match those of the backing
> > store "object", Smack is going to be happy. Since you're
> > running without privilege, you can't change the labels on
> > the files.
> >
> > Now Seth, being the sneaky person that he is, could change
> > the Smack labels on the files in the backing store while it's
> > offline. Since he has access to the backing store, he can't
> > give himself more access by changing the labels within the
> > filesystem. He can give himself less, but I'm OK with that.
> >
> >> Or maybe I'm misunderstanding you.
> >
> > Probably, but I'm undoubtedly doing the same.
> >
> > If you're going to be at LinuxCon in Seattle we should
> > continue this discussion over the beverage of your choice.
> 
> There's a small but not quite zero chance I'll be there.  I'll
> probably be in Seoul.  It's too bad that LSS and KS are in different
> places this year.

FWIW I'll be there and happy to discuss.

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

[toc] | [prev] | [next] | [standalone]

#1186891

On Fri, Jul 17, 2015 at 09:28:32AM -0500, Serge E. Hallyn wrote:
> > > If you're going to be at LinuxCon in Seattle we should
> > > continue this discussion over the beverage of your choice.
> > 
> > There's a small but not quite zero chance I'll be there.  I'll
> > probably be in Seoul.  It's too bad that LSS and KS are in different
> > places this year.
> 
> FWIW I'll be there and happy to discuss.

I'll also be in Seattle and happy to discuss.

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

[toc] | [prev] | [next] | [standalone]

#1190081

On Tue, Jul 21, 2015 at 06:52:31PM -0700, Casey Schaufler wrote:
> On 7/21/2015 1:35 PM, Seth Forshee wrote:
> > On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
> >> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
> >>>> I really don't see the benefit of making up extra rules that apply to
> >>>> users outside a userns who try to access specifically a filesystem
> >>>> with backing store.  They wouldn't make sense for filesystems without
> >>>> backing store.
> >>> Sure it would. For Smack, it would be the label a file would be
> >>> created with, which would be the label of the process creating
> >>> the memory based filesystem. For SELinux the rules are more a
> >>> touch more sophisticated, but I'm sure that Paul or Stephen could
> >>> come up with how to determine it.
> >>>
> >>> The point, looping all the way back to the beginning, where we
> >>> were talking about just ignoring the labels on the filesystem,
> >>> is that if you use the same Smack label on the files in the
> >>> filesystem as the backing store file has, we'll all be happy.
> >>> If that label isn't something user can write to, he won't be
> >>> able to write to the mounted objects, either. If there is no
> >>> backing store then use the label of the process creating the
> >>> filesystem, which will be the user, which will mean everything
> >>> will work hunky dory.
> >>>
> >>> Yes, there's work involved, but I doubt there's a lot. Getting
> >>> the label from the backing store or the creating process is
> >>> simple enough.
> >>>
> > So something like the diff below (untested)?
> 
> I think that this is close, and quite good for someone
> who isn't very familiar with Smack. It's definitely headed
> in the right direction.
> 
> > All I'm really doing is setting smk_default as you describe above and
> > then using it instead of smk_of_current() in
> > smack_inode_alloc_security() and instead of the label from the disk in
> > smack_d_instantiate().
> 
> Let's say your backing store is a file labeled Rubble.
> 
> mount -o smackfsroot=Rubble,smackfsdef=Rubble ...
> 
> It is completely reasonable for a process labeled Flintstone to
> have rwxa access to a file labeled Rubble.
> 
> Smack rule: Flintstone Rubble rwxa
> 
> In the case of writing to an existing Rubble file, what you
> have looks fine. What's not so great is that if the Flintstone
> process creates a file, it should be labeled Flintstone. Your
> use of the smk_default, which is going to violate the principle
> of least astonishment, and break the Smack policy as well.
> 
> Let's make a minor change. Instead of using smackfsroot let's
> use smackfstransmute and a slightly different access rule:
> 
> mount -o smackfstransmute=Rubble,smackfsdef=Rubble ...
> 
> Smack rule: Flintstone Rubble rwxat
> 
> Now the only change we have to make to the Smack code is
> that we don't want to create any files unless either the
> process is labeled Rubble or the rule allowing the creation
> has the "t" for transmute access. That should ensure that
> everything is labeled Rubble. If it isn't, someone has mucked
> with the metadata in a detectable way.

All right, that kind of makes sense, but I'm still missing some pieces.
Questions follow.

> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 32f598db0b0d..4597420ab933 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1486,6 +1486,10 @@ static inline void sb_start_intwrite(struct super_block *sb)
> >  	__sb_start_write(sb, SB_FREEZE_FS, true);
> >  }
> >  
> > +static inline bool sb_in_userns(struct super_block *sb)
> > +{
> > +	return sb->s_user_ns != &init_user_ns;
> > +}
> >  
> >  extern bool inode_owner_or_capable(const struct inode *inode);
> >  
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index a143328f75eb..591fd19294e7 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -255,6 +255,10 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
> >  	char *buffer;
> >  	struct smack_known *skp = NULL;
> >  
> > +	/* Should never fetch xattrs from untrusted mounts */
> > +	if (WARN_ON(sb_in_userns(ip->i_sb)))
> > +		return ERR_PTR(-EPERM);
> > +
> 
> Go ahead and fetch it, we'll check to make sure it's viable later.
> 
> >  	if (ip->i_op->getxattr == NULL)
> >  		return ERR_PTR(-EOPNOTSUPP);
> >  
> > @@ -656,10 +660,14 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
> >  		 */
> >  		if (specified)
> >  			return -EPERM;
> > +
> >  		/*
> > -		 * Unprivileged mounts get root and default from the caller.
> > +		 * User namespace mounts get root and default from the backing
> > +		 * store, if there is one. Other unprivileged mounts get them
> > +		 * from the caller.
> >  		 */
> > -		skp = smk_of_current();
> > +		skp = (sb_in_userns(sb) && sb->s_bdev) ?
> > +			smk_of_inode(sb->s_bdev->bd_inode) : smk_of_current();
> >  		sp->smk_root = skp;
> >  		sp->smk_default = skp;
> 
> 			sp->smk_flags |= SMK_INODE_TRANSMUTE;

I assume that you meant skp and not sp here.

> 
> >  	}
> > @@ -792,7 +800,12 @@ static int smack_bprm_secureexec(struct linux_binprm *bprm)
> >   */
> >  static int smack_inode_alloc_security(struct inode *inode)
> >  {
> > -	struct smack_known *skp = smk_of_current();
> > +	struct smack_known *skp;
> > +
> > +	if (sb_in_userns(inode->i_sb))
> > +		skp = ((struct superblock_smack *)(inode->i_sb->s_security))->smk_default;
> > +	else
> > +		skp = smk_of_current();
> 
> This should be left alone.
> smack_inode_init_security is where you could disallow access that doesn't
> legitimately result in a Rubble label on the file. It's something like
> 
> 	... after the call may = smk_access_entry(...)
> 	if (sb_in_userns(inode->i_sb))
> 		if (skp != dsp && (may & MAY_TRANSMUTE) == 0)
> 			return -EACCES; 

I'm not getting how this covers all cases.

So we've set the transmute flag on the root inode. Files and directories
created in the root directory get the same label, and directories also
get the transmute attribute. That's all fine.

What about an existing directory in the filesystem that already has a
Slate label? I'm not getting what happens with this directory, or for
new files created in this directory, which also relates to my other
questions below.

Also an aside - smk_access_entry looks weird. may is initialized to
-ENOENT, and then rule_list is searched for a rule which matches the
object and subject labels. Presumably it's possible that no rule could
be found, otherwise the prior initialization of may is pointless. If
this happens the following code treats it as though it always contains
access flags even though it might contain -ENOENT. Nothing bad actually
happens with a two's compliement representation of -ENOENT since it will
just set a bit that's already set, but it still seems like it should
have a may > 0 condition, for clarity if for no other reason.

> 
> >  	inode->i_security = new_inode_smack(skp);
> >  	if (inode->i_security == NULL)
> > @@ -3175,6 +3188,11 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
> >  			break;
> >  		}
> >  		/*
> > +		 * Don't use labels from xattrs for unprivileged mounts.
> > +		 */
> > +		if (sb_in_userns(inode->i_sb))
> > +			break;
> > +		/*
> 
> Again, use the label. Just check to make sure it's what you expect.

What happens if it's not what I expect? smack_d_instantiate cannot fail
... so just use the default label? In that case why bother reading it at
all? Or would we actually want to change the on-disk label if it didn't
match?

> 
> >  		 * No xattr support means, alas, no SMACK label.
> >  		 * Use the aforeapplied default.
> >  		 * It would be curious if the label of the task
> 
> Also untested.
> 
> > --
> > 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/
> >
> 
--
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/

[toc] | [prev] | [next] | [standalone]

#1190377

On 7/22/2015 12:32 PM, Seth Forshee wrote:
> On Wed, Jul 22, 2015 at 11:10:46AM -0700, Casey Schaufler wrote:
>> On 7/22/2015 8:56 AM, Seth Forshee wrote:
>>> On Tue, Jul 21, 2015 at 06:52:31PM -0700, Casey Schaufler wrote:
>>>> On 7/21/2015 1:35 PM, Seth Forshee wrote:
>>>>> On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
>>>>>> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
>>>>>>>> I really don't see the benefit of making up extra rules that apply to
>>>>>>>> users outside a userns who try to access specifically a filesystem
>>>>>>>> with backing store.  They wouldn't make sense for filesystems without
>>>>>>>> backing store.
>>>>>>> Sure it would. For Smack, it would be the label a file would be
>>>>>>> created with, which would be the label of the process creating
>>>>>>> the memory based filesystem. For SELinux the rules are more a
>>>>>>> touch more sophisticated, but I'm sure that Paul or Stephen could
>>>>>>> come up with how to determine it.
>>>>>>>
>>>>>>> The point, looping all the way back to the beginning, where we
>>>>>>> were talking about just ignoring the labels on the filesystem,
>>>>>>> is that if you use the same Smack label on the files in the
>>>>>>> filesystem as the backing store file has, we'll all be happy.
>>>>>>> If that label isn't something user can write to, he won't be
>>>>>>> able to write to the mounted objects, either. If there is no
>>>>>>> backing store then use the label of the process creating the
>>>>>>> filesystem, which will be the user, which will mean everything
>>>>>>> will work hunky dory.
>>>>>>>
>>>>>>> Yes, there's work involved, but I doubt there's a lot. Getting
>>>>>>> the label from the backing store or the creating process is
>>>>>>> simple enough.
>>>>>>>
>>>>> So something like the diff below (untested)?
>>>> I think that this is close, and quite good for someone
>>>> who isn't very familiar with Smack. It's definitely headed
>>>> in the right direction.
>>>>
>>>>> All I'm really doing is setting smk_default as you describe above and
>>>>> then using it instead of smk_of_current() in
>>>>> smack_inode_alloc_security() and instead of the label from the disk in
>>>>> smack_d_instantiate().
>>>> Let's say your backing store is a file labeled Rubble.
>>>>
>>>> mount -o smackfsroot=Rubble,smackfsdef=Rubble ...
>>>>
>>>> It is completely reasonable for a process labeled Flintstone to
>>>> have rwxa access to a file labeled Rubble.
>>>>
>>>> Smack rule: Flintstone Rubble rwxa
>>>>
>>>> In the case of writing to an existing Rubble file, what you
>>>> have looks fine. What's not so great is that if the Flintstone
>>>> process creates a file, it should be labeled Flintstone. Your
>>>> use of the smk_default, which is going to violate the principle
>>>> of least astonishment, and break the Smack policy as well.
>>>>
>>>> Let's make a minor change. Instead of using smackfsroot let's
>>>> use smackfstransmute and a slightly different access rule:
>>>>
>>>> mount -o smackfstransmute=Rubble,smackfsdef=Rubble ...
>>>>
>>>> Smack rule: Flintstone Rubble rwxat
>>>>
>>>> Now the only change we have to make to the Smack code is
>>>> that we don't want to create any files unless either the
>>>> process is labeled Rubble or the rule allowing the creation
>>>> has the "t" for transmute access. That should ensure that
>>>> everything is labeled Rubble. If it isn't, someone has mucked
>>>> with the metadata in a detectable way.
>>> All right, that kind of makes sense, but I'm still missing some pieces.
>>> Questions follow.
>>>
>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>>> index 32f598db0b0d..4597420ab933 100644
>>>>> --- a/include/linux/fs.h
>>>>> +++ b/include/linux/fs.h
>>>>> @@ -1486,6 +1486,10 @@ static inline void sb_start_intwrite(struct super_block *sb)
>>>>>  	__sb_start_write(sb, SB_FREEZE_FS, true);
>>>>>  }
>>>>>  
>>>>> +static inline bool sb_in_userns(struct super_block *sb)
>>>>> +{
>>>>> +	return sb->s_user_ns != &init_user_ns;
>>>>> +}
>>>>>  
>>>>>  extern bool inode_owner_or_capable(const struct inode *inode);
>>>>>  
>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>> index a143328f75eb..591fd19294e7 100644
>>>>> --- a/security/smack/smack_lsm.c
>>>>> +++ b/security/smack/smack_lsm.c
>>>>> @@ -255,6 +255,10 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
>>>>>  	char *buffer;
>>>>>  	struct smack_known *skp = NULL;
>>>>>  
>>>>> +	/* Should never fetch xattrs from untrusted mounts */
>>>>> +	if (WARN_ON(sb_in_userns(ip->i_sb)))
>>>>> +		return ERR_PTR(-EPERM);
>>>>> +
>>>> Go ahead and fetch it, we'll check to make sure it's viable later.
>>>>
>>>>>  	if (ip->i_op->getxattr == NULL)
>>>>>  		return ERR_PTR(-EOPNOTSUPP);
>>>>>  
>>>>> @@ -656,10 +660,14 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
>>>>>  		 */
>>>>>  		if (specified)
>>>>>  			return -EPERM;
>>>>> +
>>>>>  		/*
>>>>> -		 * Unprivileged mounts get root and default from the caller.
>>>>> +		 * User namespace mounts get root and default from the backing
>>>>> +		 * store, if there is one. Other unprivileged mounts get them
>>>>> +		 * from the caller.
>>>>>  		 */
>>>>> -		skp = smk_of_current();
>>>>> +		skp = (sb_in_userns(sb) && sb->s_bdev) ?
>>>>> +			smk_of_inode(sb->s_bdev->bd_inode) : smk_of_current();
>>>>>  		sp->smk_root = skp;
>>>>>  		sp->smk_default = skp;
>>>> 			sp->smk_flags |= SMK_INODE_TRANSMUTE;
>>> I assume that you meant skp and not sp here.
>> Actually, neither is correct. You want to set SMK_INODE_TRANSMUTE
>> in the smk_flags field of the root inode. That's easy:
>>
>> 			transmute = 1;
>>
>> and the code after "Initialize the root inode" will take care of it.
> Yeah, that's what I've actually done.
>
>>>>>  	}
>>>>> @@ -792,7 +800,12 @@ static int smack_bprm_secureexec(struct linux_binprm *bprm)
>>>>>   */
>>>>>  static int smack_inode_alloc_security(struct inode *inode)
>>>>>  {
>>>>> -	struct smack_known *skp = smk_of_current();
>>>>> +	struct smack_known *skp;
>>>>> +
>>>>> +	if (sb_in_userns(inode->i_sb))
>>>>> +		skp = ((struct superblock_smack *)(inode->i_sb->s_security))->smk_default;
>>>>> +	else
>>>>> +		skp = smk_of_current();
>>>> This should be left alone.
>>>> smack_inode_init_security is where you could disallow access that doesn't
>>>> legitimately result in a Rubble label on the file. It's something like
>>>>
>>>> 	... after the call may = smk_access_entry(...)
>>>> 	if (sb_in_userns(inode->i_sb))
>>>> 		if (skp != dsp && (may & MAY_TRANSMUTE) == 0)
>>>> 			return -EACCES; 
>>> I'm not getting how this covers all cases.
>>>
>>> So we've set the transmute flag on the root inode. Files and directories
>>> created in the root directory get the same label, and directories also
>>> get the transmute attribute. That's all fine.
>>>
>>> What about an existing directory in the filesystem that already has a
>>> Slate label? I'm not getting what happens with this directory, or for
>>> new files created in this directory, which also relates to my other
>>> questions below.
>>>
>>> Also an aside - smk_access_entry looks weird. may is initialized to
>>> -ENOENT, and then rule_list is searched for a rule which matches the
>>> object and subject labels. Presumably it's possible that no rule could
>>> be found, otherwise the prior initialization of may is pointless. If
>>> this happens the following code treats it as though it always contains
>>> access flags even though it might contain -ENOENT. Nothing bad actually
>>> happens with a two's compliement representation of -ENOENT since it will
>>> just set a bit that's already set, but it still seems like it should
>>> have a may > 0 condition, for clarity if for no other reason.
>> My suggested code is just wrong. I wasn't looking at the whole code,
>> only the patch, and got myself confused. Apologies.
>>
>> If we want to go straight for the jugular how about this? I'm assuming
>> that inode->i_sb->s_bdev->bd_inode is the inode of the backing store.
> Yes.
>
>> static int smack_inode_permission(struct inode *inode, int mask)
>> {
>> 	struct smk_audit_info ad;
>> 	int no_block = mask & MAY_NOT_BLOCK;
>> 	int rc;
>>
>> 	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>> 	/*
>> 	 * No permission to check. Existence test. Yup, it's there.
>> 	 */
>> 	if (mask == 0)
>> 		return 0;
>>
>> +	if (sb_in_userns(inode->i_sb)) &&
>> +	    smk_of_inode(inode) != smk_of_inode(inode->i_sb->s_bdev->bd_inode))
>> +		return -EACCES;
>> +
>> 	/* May be droppable after audit */
>> 	if (no_block)
>> 		return -ECHILD;
>> 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
>> 	smk_ad_setfield_u_fs_inode(&ad, inode);
>> 	rc = smk_curacc(smk_of_inode(inode), mask, &ad);
>> 	rc = smk_bu_inode(inode, mask, rc);
>> 	return rc;
>> }
> Hmm, okay. I think I've been a little confused all this time about how
> you want to handle these unprivileged mounts.

Not your problem. I'm not the most consistent of reviewers.

> Originally I thought you wanted all objects in the filesystem to get the
> same label as the backing store. That's what I tried to implement
> originally, i.e. smk_root=smk_default=smk_of_inode(...->bd_inode), then
> assign every object (new and existing) smk_default and completely ignore
> the labels on disk.

I want everything to have the label of the backing store, but
I don't want to ignore it if it somehow got something else. Because
the only legitimate label for this example is Rubble, I want to
reject anything else that appears. If someone builds a filesystem
by hand with Slate labels I want it treated "safely".

> This is what I currently think you want for user ns mounts:
>
>  1. smk_root and smk_default are assigned the label of the backing
>     device.
>  2. s_root is assigned the transmute property.
>  3. For existing files:
>     a. Files with the same label as the backing device are accessible.
>     b. Files with any other label are not accessible.

That's right. Accept correct data, reject anything that's not right.

> If this is right, there are a couple lingering questions in my mind.
>
> First, what happens with files created in directories with the same
> label as the backing device but without the transmute property set? The
> inode for the new file will initially be labeled with smk_of_current(),
> but then during d_instantiate it will get smk_default and thus end up
> with the label we want. So that seems okay.

Yes.

> The second is whether files with the SMACK64EXEC attribute is still a
> problem. It seems it is, for files with the same label as the backing
> store at least. I think we can simply skip the code that reads out this
> xattr and sets smk_task for user ns mounts, or else skip assigning the
> label to the new task in bprm_set_creds. The latter seems more
> consistent with the approach you've suggested for dealing with labels
> from disk.

Yes, I think that skipping the smk_fetch(XATTR_NAME_SMACKEXEC, ...) in
smack_d_instantiate for unprivileged mounts would do the trick.

> So I guess all of that seems okay, though perhaps a bit restrictive
> given that the user who mounted the filesystem already has full access
> to the backing store.

In truth, there is no reason to expect that the "user" who did the
mount will ever have a Smack label that differs from the label of
the backing store. If what we've got here seems restrictive, it's
because you've got access from someone other than the "user".

> Please let me know whether or not this matches up with what you are
> thinking, then I can procede with the implementation.

My current mindset is that, if you're going to allow unprivileged
mounts of user defined backing stores, this is as safe as we can
make it.

>
> Thanks,
> Seth
>
> --
> 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/
>

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

[toc] | [prev] | [next] | [standalone]

#1190621

On Wed, Jul 22, 2015 at 07:15:19PM -0500, Eric W. Biederman wrote:
> Casey Schaufler <casey@schaufler-ca.com> writes:
> 
> > On 7/22/2015 12:32 PM, Seth Forshee wrote:
> >> On Wed, Jul 22, 2015 at 11:10:46AM -0700, Casey Schaufler wrote:
> >>> On 7/22/2015 8:56 AM, Seth Forshee wrote:
> >>>> On Tue, Jul 21, 2015 at 06:52:31PM -0700, Casey Schaufler wrote:
> >>>>> On 7/21/2015 1:35 PM, Seth Forshee wrote:
> >>>>>> On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
> >>>>>>> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>>>>>> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
> >>>>>>>>> I really don't see the benefit of making up extra rules that apply to
> >>>>>>>>> users outside a userns who try to access specifically a filesystem
> >>>>>>>>> with backing store.  They wouldn't make sense for filesystems without
> >>>>>>>>> backing store.
> >>>>>>>> Sure it would. For Smack, it would be the label a file would be
> >>>>>>>> created with, which would be the label of the process creating
> >>>>>>>> the memory based filesystem. For SELinux the rules are more a
> >>>>>>>> touch more sophisticated, but I'm sure that Paul or Stephen could
> >>>>>>>> come up with how to determine it.
> >>>>>>>>
> >>>>>>>> The point, looping all the way back to the beginning, where we
> >>>>>>>> were talking about just ignoring the labels on the filesystem,
> >>>>>>>> is that if you use the same Smack label on the files in the
> >>>>>>>> filesystem as the backing store file has, we'll all be happy.
> >>>>>>>> If that label isn't something user can write to, he won't be
> >>>>>>>> able to write to the mounted objects, either. If there is no
> >>>>>>>> backing store then use the label of the process creating the
> >>>>>>>> filesystem, which will be the user, which will mean everything
> >>>>>>>> will work hunky dory.
> >>>>>>>>
> >>>>>>>> Yes, there's work involved, but I doubt there's a lot. Getting
> >>>>>>>> the label from the backing store or the creating process is
> >>>>>>>> simple enough.
> >>>>>>>>
> >>>>>> So something like the diff below (untested)?
> >>>>> I think that this is close, and quite good for someone
> >>>>> who isn't very familiar with Smack. It's definitely headed
> >>>>> in the right direction.
> >>>>>
> >>>>>> All I'm really doing is setting smk_default as you describe above and
> >>>>>> then using it instead of smk_of_current() in
> >>>>>> smack_inode_alloc_security() and instead of the label from the disk in
> >>>>>> smack_d_instantiate().
> >>>>> Let's say your backing store is a file labeled Rubble.
> >>>>>
> >>>>> mount -o smackfsroot=Rubble,smackfsdef=Rubble ...
> >>>>>
> >>>>> It is completely reasonable for a process labeled Flintstone to
> >>>>> have rwxa access to a file labeled Rubble.
> >>>>>
> >>>>> Smack rule: Flintstone Rubble rwxa
> >>>>>
> >>>>> In the case of writing to an existing Rubble file, what you
> >>>>> have looks fine. What's not so great is that if the Flintstone
> >>>>> process creates a file, it should be labeled Flintstone. Your
> >>>>> use of the smk_default, which is going to violate the principle
> >>>>> of least astonishment, and break the Smack policy as well.
> >>>>>
> >>>>> Let's make a minor change. Instead of using smackfsroot let's
> >>>>> use smackfstransmute and a slightly different access rule:
> >>>>>
> >>>>> mount -o smackfstransmute=Rubble,smackfsdef=Rubble ...
> >>>>>
> >>>>> Smack rule: Flintstone Rubble rwxat
> >>>>>
> >>>>> Now the only change we have to make to the Smack code is
> >>>>> that we don't want to create any files unless either the
> >>>>> process is labeled Rubble or the rule allowing the creation
> >>>>> has the "t" for transmute access. That should ensure that
> >>>>> everything is labeled Rubble. If it isn't, someone has mucked
> >>>>> with the metadata in a detectable way.
> >>>> All right, that kind of makes sense, but I'm still missing some pieces.
> >>>> Questions follow.
> >>>>
> >>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
> >>>>>> index 32f598db0b0d..4597420ab933 100644
> >>>>>> --- a/include/linux/fs.h
> >>>>>> +++ b/include/linux/fs.h
> >>>>>> @@ -1486,6 +1486,10 @@ static inline void sb_start_intwrite(struct super_block *sb)
> >>>>>>  	__sb_start_write(sb, SB_FREEZE_FS, true);
> >>>>>>  }
> >>>>>>  
> >>>>>> +static inline bool sb_in_userns(struct super_block *sb)
> >>>>>> +{
> >>>>>> +	return sb->s_user_ns != &init_user_ns;
> >>>>>> +}
> >>>>>>  
> >>>>>>  extern bool inode_owner_or_capable(const struct inode *inode);
> >>>>>>  
> >>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> >>>>>> index a143328f75eb..591fd19294e7 100644
> >>>>>> --- a/security/smack/smack_lsm.c
> >>>>>> +++ b/security/smack/smack_lsm.c
> >>>>>> @@ -255,6 +255,10 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
> >>>>>>  	char *buffer;
> >>>>>>  	struct smack_known *skp = NULL;
> >>>>>>  
> >>>>>> +	/* Should never fetch xattrs from untrusted mounts */
> >>>>>> +	if (WARN_ON(sb_in_userns(ip->i_sb)))
> >>>>>> +		return ERR_PTR(-EPERM);
> >>>>>> +
> >>>>> Go ahead and fetch it, we'll check to make sure it's viable later.
> >>>>>
> >>>>>>  	if (ip->i_op->getxattr == NULL)
> >>>>>>  		return ERR_PTR(-EOPNOTSUPP);
> >>>>>>  
> >>>>>> @@ -656,10 +660,14 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
> >>>>>>  		 */
> >>>>>>  		if (specified)
> >>>>>>  			return -EPERM;
> >>>>>> +
> >>>>>>  		/*
> >>>>>> -		 * Unprivileged mounts get root and default from the caller.
> >>>>>> +		 * User namespace mounts get root and default from the backing
> >>>>>> +		 * store, if there is one. Other unprivileged mounts get them
> >>>>>> +		 * from the caller.
> >>>>>>  		 */
> >>>>>> -		skp = smk_of_current();
> >>>>>> +		skp = (sb_in_userns(sb) && sb->s_bdev) ?
> >>>>>> +			smk_of_inode(sb->s_bdev->bd_inode) : smk_of_current();
> >>>>>>  		sp->smk_root = skp;
> >>>>>>  		sp->smk_default = skp;
> >>>>> 			sp->smk_flags |= SMK_INODE_TRANSMUTE;
> >>>> I assume that you meant skp and not sp here.
> >>> Actually, neither is correct. You want to set SMK_INODE_TRANSMUTE
> >>> in the smk_flags field of the root inode. That's easy:
> >>>
> >>> 			transmute = 1;
> >>>
> >>> and the code after "Initialize the root inode" will take care of it.
> >> Yeah, that's what I've actually done.
> >>
> >>>>>>  	}
> >>>>>> @@ -792,7 +800,12 @@ static int smack_bprm_secureexec(struct linux_binprm *bprm)
> >>>>>>   */
> >>>>>>  static int smack_inode_alloc_security(struct inode *inode)
> >>>>>>  {
> >>>>>> -	struct smack_known *skp = smk_of_current();
> >>>>>> +	struct smack_known *skp;
> >>>>>> +
> >>>>>> +	if (sb_in_userns(inode->i_sb))
> >>>>>> +		skp = ((struct superblock_smack *)(inode->i_sb->s_security))->smk_default;
> >>>>>> +	else
> >>>>>> +		skp = smk_of_current();
> >>>>> This should be left alone.
> >>>>> smack_inode_init_security is where you could disallow access that doesn't
> >>>>> legitimately result in a Rubble label on the file. It's something like
> >>>>>
> >>>>> 	... after the call may = smk_access_entry(...)
> >>>>> 	if (sb_in_userns(inode->i_sb))
> >>>>> 		if (skp != dsp && (may & MAY_TRANSMUTE) == 0)
> >>>>> 			return -EACCES; 
> >>>> I'm not getting how this covers all cases.
> >>>>
> >>>> So we've set the transmute flag on the root inode. Files and directories
> >>>> created in the root directory get the same label, and directories also
> >>>> get the transmute attribute. That's all fine.
> >>>>
> >>>> What about an existing directory in the filesystem that already has a
> >>>> Slate label? I'm not getting what happens with this directory, or for
> >>>> new files created in this directory, which also relates to my other
> >>>> questions below.
> >>>>
> >>>> Also an aside - smk_access_entry looks weird. may is initialized to
> >>>> -ENOENT, and then rule_list is searched for a rule which matches the
> >>>> object and subject labels. Presumably it's possible that no rule could
> >>>> be found, otherwise the prior initialization of may is pointless. If
> >>>> this happens the following code treats it as though it always contains
> >>>> access flags even though it might contain -ENOENT. Nothing bad actually
> >>>> happens with a two's compliement representation of -ENOENT since it will
> >>>> just set a bit that's already set, but it still seems like it should
> >>>> have a may > 0 condition, for clarity if for no other reason.
> >>> My suggested code is just wrong. I wasn't looking at the whole code,
> >>> only the patch, and got myself confused. Apologies.
> >>>
> >>> If we want to go straight for the jugular how about this? I'm assuming
> >>> that inode->i_sb->s_bdev->bd_inode is the inode of the backing store.
> >> Yes.
> >>
> >>> static int smack_inode_permission(struct inode *inode, int mask)
> >>> {
> >>> 	struct smk_audit_info ad;
> >>> 	int no_block = mask & MAY_NOT_BLOCK;
> >>> 	int rc;
> >>>
> >>> 	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
> >>> 	/*
> >>> 	 * No permission to check. Existence test. Yup, it's there.
> >>> 	 */
> >>> 	if (mask == 0)
> >>> 		return 0;
> >>>
> >>> +	if (sb_in_userns(inode->i_sb)) &&
> >>> +	    smk_of_inode(inode) != smk_of_inode(inode->i_sb->s_bdev->bd_inode))
> >>> +		return -EACCES;
> >>> +
> >>> 	/* May be droppable after audit */
> >>> 	if (no_block)
> >>> 		return -ECHILD;
> >>> 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
> >>> 	smk_ad_setfield_u_fs_inode(&ad, inode);
> >>> 	rc = smk_curacc(smk_of_inode(inode), mask, &ad);
> >>> 	rc = smk_bu_inode(inode, mask, rc);
> >>> 	return rc;
> >>> }
> >> Hmm, okay. I think I've been a little confused all this time about how
> >> you want to handle these unprivileged mounts.
> >
> > Not your problem. I'm not the most consistent of reviewers.
> >
> >> Originally I thought you wanted all objects in the filesystem to get the
> >> same label as the backing store. That's what I tried to implement
> >> originally, i.e. smk_root=smk_default=smk_of_inode(...->bd_inode), then
> >> assign every object (new and existing) smk_default and completely ignore
> >> the labels on disk.
> >
> > I want everything to have the label of the backing store, but
> > I don't want to ignore it if it somehow got something else. Because
> > the only legitimate label for this example is Rubble, I want to
> > reject anything else that appears. If someone builds a filesystem
> > by hand with Slate labels I want it treated "safely".
> >
> >> This is what I currently think you want for user ns mounts:
> >>
> >>  1. smk_root and smk_default are assigned the label of the backing
> >>     device.
> >>  2. s_root is assigned the transmute property.
> >>  3. For existing files:
> >>     a. Files with the same label as the backing device are accessible.
> >>     b. Files with any other label are not accessible.
> >
> > That's right. Accept correct data, reject anything that's not right.
> >
> >> If this is right, there are a couple lingering questions in my mind.
> >>
> >> First, what happens with files created in directories with the same
> >> label as the backing device but without the transmute property set? The
> >> inode for the new file will initially be labeled with smk_of_current(),
> >> but then during d_instantiate it will get smk_default and thus end up
> >> with the label we want. So that seems okay.
> >
> > Yes.
> >
> >> The second is whether files with the SMACK64EXEC attribute is still a
> >> problem. It seems it is, for files with the same label as the backing
> >> store at least. I think we can simply skip the code that reads out this
> >> xattr and sets smk_task for user ns mounts, or else skip assigning the
> >> label to the new task in bprm_set_creds. The latter seems more
> >> consistent with the approach you've suggested for dealing with labels
> >> from disk.
> >
> > Yes, I think that skipping the smk_fetch(XATTR_NAME_SMACKEXEC, ...) in
> > smack_d_instantiate for unprivileged mounts would do the trick.
> >
> >> So I guess all of that seems okay, though perhaps a bit restrictive
> >> given that the user who mounted the filesystem already has full access
> >> to the backing store.
> >
> > In truth, there is no reason to expect that the "user" who did the
> > mount will ever have a Smack label that differs from the label of
> > the backing store. If what we've got here seems restrictive, it's
> > because you've got access from someone other than the "user".
> >
> >> Please let me know whether or not this matches up with what you are
> >> thinking, then I can procede with the implementation.
> >
> > My current mindset is that, if you're going to allow unprivileged
> > mounts of user defined backing stores, this is as safe as we can
> > make it.
> 
> That actually sounds very reasonable to me.  It is essentially what we
> do with uid and gids already.  I presume the smack namespace support
> would when integrated with all of this would allow a set of labels to be
> set.
> 
> Have I missed a part of the conversation you talk about fileystems that
> don't have support for storing labels?  Filesystems like vfat, isofs,
> etc.

As I read the code they should all end up with the superblock's
smk_default label for the objects in RAM, i.e. the label of the backing
store. The same would be true for existing files in a filesystem which
does support storing labels but has no labels on the files.

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

[toc] | [prev] | [next] | [standalone]

#1191341

On 7/22/2015 5:15 PM, Eric W. Biederman wrote:
> Casey Schaufler <casey@schaufler-ca.com> writes:
>
>> On 7/22/2015 12:32 PM, Seth Forshee wrote:
>>> On Wed, Jul 22, 2015 at 11:10:46AM -0700, Casey Schaufler wrote:
>>>> On 7/22/2015 8:56 AM, Seth Forshee wrote:
>>>>> On Tue, Jul 21, 2015 at 06:52:31PM -0700, Casey Schaufler wrote:
>>>>>> On 7/21/2015 1:35 PM, Seth Forshee wrote:
>>>>>>> On Thu, Jul 16, 2015 at 05:59:22PM -0700, Andy Lutomirski wrote:
>>>>>>>> On Thu, Jul 16, 2015 at 5:45 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>>>>> On 7/16/2015 4:29 PM, Andy Lutomirski wrote:
>>>>>>>>>> I really don't see the benefit of making up extra rules that apply to
>>>>>>>>>> users outside a userns who try to access specifically a filesystem
>>>>>>>>>> with backing store.  They wouldn't make sense for filesystems without
>>>>>>>>>> backing store.
>>>>>>>>> Sure it would. For Smack, it would be the label a file would be
>>>>>>>>> created with, which would be the label of the process creating
>>>>>>>>> the memory based filesystem. For SELinux the rules are more a
>>>>>>>>> touch more sophisticated, but I'm sure that Paul or Stephen could
>>>>>>>>> come up with how to determine it.
>>>>>>>>>
>>>>>>>>> The point, looping all the way back to the beginning, where we
>>>>>>>>> were talking about just ignoring the labels on the filesystem,
>>>>>>>>> is that if you use the same Smack label on the files in the
>>>>>>>>> filesystem as the backing store file has, we'll all be happy.
>>>>>>>>> If that label isn't something user can write to, he won't be
>>>>>>>>> able to write to the mounted objects, either. If there is no
>>>>>>>>> backing store then use the label of the process creating the
>>>>>>>>> filesystem, which will be the user, which will mean everything
>>>>>>>>> will work hunky dory.
>>>>>>>>>
>>>>>>>>> Yes, there's work involved, but I doubt there's a lot. Getting
>>>>>>>>> the label from the backing store or the creating process is
>>>>>>>>> simple enough.
>>>>>>>>>
>>>>>>> So something like the diff below (untested)?
>>>>>> I think that this is close, and quite good for someone
>>>>>> who isn't very familiar with Smack. It's definitely headed
>>>>>> in the right direction.
>>>>>>
>>>>>>> All I'm really doing is setting smk_default as you describe above and
>>>>>>> then using it instead of smk_of_current() in
>>>>>>> smack_inode_alloc_security() and instead of the label from the disk in
>>>>>>> smack_d_instantiate().
>>>>>> Let's say your backing store is a file labeled Rubble.
>>>>>>
>>>>>> mount -o smackfsroot=Rubble,smackfsdef=Rubble ...
>>>>>>
>>>>>> It is completely reasonable for a process labeled Flintstone to
>>>>>> have rwxa access to a file labeled Rubble.
>>>>>>
>>>>>> Smack rule: Flintstone Rubble rwxa
>>>>>>
>>>>>> In the case of writing to an existing Rubble file, what you
>>>>>> have looks fine. What's not so great is that if the Flintstone
>>>>>> process creates a file, it should be labeled Flintstone. Your
>>>>>> use of the smk_default, which is going to violate the principle
>>>>>> of least astonishment, and break the Smack policy as well.
>>>>>>
>>>>>> Let's make a minor change. Instead of using smackfsroot let's
>>>>>> use smackfstransmute and a slightly different access rule:
>>>>>>
>>>>>> mount -o smackfstransmute=Rubble,smackfsdef=Rubble ...
>>>>>>
>>>>>> Smack rule: Flintstone Rubble rwxat
>>>>>>
>>>>>> Now the only change we have to make to the Smack code is
>>>>>> that we don't want to create any files unless either the
>>>>>> process is labeled Rubble or the rule allowing the creation
>>>>>> has the "t" for transmute access. That should ensure that
>>>>>> everything is labeled Rubble. If it isn't, someone has mucked
>>>>>> with the metadata in a detectable way.
>>>>> All right, that kind of makes sense, but I'm still missing some pieces.
>>>>> Questions follow.
>>>>>
>>>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>>>>> index 32f598db0b0d..4597420ab933 100644
>>>>>>> --- a/include/linux/fs.h
>>>>>>> +++ b/include/linux/fs.h
>>>>>>> @@ -1486,6 +1486,10 @@ static inline void sb_start_intwrite(struct super_block *sb)
>>>>>>>  	__sb_start_write(sb, SB_FREEZE_FS, true);
>>>>>>>  }
>>>>>>>  
>>>>>>> +static inline bool sb_in_userns(struct super_block *sb)
>>>>>>> +{
>>>>>>> +	return sb->s_user_ns != &init_user_ns;
>>>>>>> +}
>>>>>>>  
>>>>>>>  extern bool inode_owner_or_capable(const struct inode *inode);
>>>>>>>  
>>>>>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>>>>>> index a143328f75eb..591fd19294e7 100644
>>>>>>> --- a/security/smack/smack_lsm.c
>>>>>>> +++ b/security/smack/smack_lsm.c
>>>>>>> @@ -255,6 +255,10 @@ static struct smack_known *smk_fetch(const char *name, struct inode *ip,
>>>>>>>  	char *buffer;
>>>>>>>  	struct smack_known *skp = NULL;
>>>>>>>  
>>>>>>> +	/* Should never fetch xattrs from untrusted mounts */
>>>>>>> +	if (WARN_ON(sb_in_userns(ip->i_sb)))
>>>>>>> +		return ERR_PTR(-EPERM);
>>>>>>> +
>>>>>> Go ahead and fetch it, we'll check to make sure it's viable later.
>>>>>>
>>>>>>>  	if (ip->i_op->getxattr == NULL)
>>>>>>>  		return ERR_PTR(-EOPNOTSUPP);
>>>>>>>  
>>>>>>> @@ -656,10 +660,14 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
>>>>>>>  		 */
>>>>>>>  		if (specified)
>>>>>>>  			return -EPERM;
>>>>>>> +
>>>>>>>  		/*
>>>>>>> -		 * Unprivileged mounts get root and default from the caller.
>>>>>>> +		 * User namespace mounts get root and default from the backing
>>>>>>> +		 * store, if there is one. Other unprivileged mounts get them
>>>>>>> +		 * from the caller.
>>>>>>>  		 */
>>>>>>> -		skp = smk_of_current();
>>>>>>> +		skp = (sb_in_userns(sb) && sb->s_bdev) ?
>>>>>>> +			smk_of_inode(sb->s_bdev->bd_inode) : smk_of_current();
>>>>>>>  		sp->smk_root = skp;
>>>>>>>  		sp->smk_default = skp;
>>>>>> 			sp->smk_flags |= SMK_INODE_TRANSMUTE;
>>>>> I assume that you meant skp and not sp here.
>>>> Actually, neither is correct. You want to set SMK_INODE_TRANSMUTE
>>>> in the smk_flags field of the root inode. That's easy:
>>>>
>>>> 			transmute = 1;
>>>>
>>>> and the code after "Initialize the root inode" will take care of it.
>>> Yeah, that's what I've actually done.
>>>
>>>>>>>  	}
>>>>>>> @@ -792,7 +800,12 @@ static int smack_bprm_secureexec(struct linux_binprm *bprm)
>>>>>>>   */
>>>>>>>  static int smack_inode_alloc_security(struct inode *inode)
>>>>>>>  {
>>>>>>> -	struct smack_known *skp = smk_of_current();
>>>>>>> +	struct smack_known *skp;
>>>>>>> +
>>>>>>> +	if (sb_in_userns(inode->i_sb))
>>>>>>> +		skp = ((struct superblock_smack *)(inode->i_sb->s_security))->smk_default;
>>>>>>> +	else
>>>>>>> +		skp = smk_of_current();
>>>>>> This should be left alone.
>>>>>> smack_inode_init_security is where you could disallow access that doesn't
>>>>>> legitimately result in a Rubble label on the file. It's something like
>>>>>>
>>>>>> 	... after the call may = smk_access_entry(...)
>>>>>> 	if (sb_in_userns(inode->i_sb))
>>>>>> 		if (skp != dsp && (may & MAY_TRANSMUTE) == 0)
>>>>>> 			return -EACCES; 
>>>>> I'm not getting how this covers all cases.
>>>>>
>>>>> So we've set the transmute flag on the root inode. Files and directories
>>>>> created in the root directory get the same label, and directories also
>>>>> get the transmute attribute. That's all fine.
>>>>>
>>>>> What about an existing directory in the filesystem that already has a
>>>>> Slate label? I'm not getting what happens with this directory, or for
>>>>> new files created in this directory, which also relates to my other
>>>>> questions below.
>>>>>
>>>>> Also an aside - smk_access_entry looks weird. may is initialized to
>>>>> -ENOENT, and then rule_list is searched for a rule which matches the
>>>>> object and subject labels. Presumably it's possible that no rule could
>>>>> be found, otherwise the prior initialization of may is pointless. If
>>>>> this happens the following code treats it as though it always contains
>>>>> access flags even though it might contain -ENOENT. Nothing bad actually
>>>>> happens with a two's compliement representation of -ENOENT since it will
>>>>> just set a bit that's already set, but it still seems like it should
>>>>> have a may > 0 condition, for clarity if for no other reason.
>>>> My suggested code is just wrong. I wasn't looking at the whole code,
>>>> only the patch, and got myself confused. Apologies.
>>>>
>>>> If we want to go straight for the jugular how about this? I'm assuming
>>>> that inode->i_sb->s_bdev->bd_inode is the inode of the backing store.
>>> Yes.
>>>
>>>> static int smack_inode_permission(struct inode *inode, int mask)
>>>> {
>>>> 	struct smk_audit_info ad;
>>>> 	int no_block = mask & MAY_NOT_BLOCK;
>>>> 	int rc;
>>>>
>>>> 	mask &= (MAY_READ|MAY_WRITE|MAY_EXEC|MAY_APPEND);
>>>> 	/*
>>>> 	 * No permission to check. Existence test. Yup, it's there.
>>>> 	 */
>>>> 	if (mask == 0)
>>>> 		return 0;
>>>>
>>>> +	if (sb_in_userns(inode->i_sb)) &&
>>>> +	    smk_of_inode(inode) != smk_of_inode(inode->i_sb->s_bdev->bd_inode))
>>>> +		return -EACCES;
>>>> +
>>>> 	/* May be droppable after audit */
>>>> 	if (no_block)
>>>> 		return -ECHILD;
>>>> 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_INODE);
>>>> 	smk_ad_setfield_u_fs_inode(&ad, inode);
>>>> 	rc = smk_curacc(smk_of_inode(inode), mask, &ad);
>>>> 	rc = smk_bu_inode(inode, mask, rc);
>>>> 	return rc;
>>>> }
>>> Hmm, okay. I think I've been a little confused all this time about how
>>> you want to handle these unprivileged mounts.
>> Not your problem. I'm not the most consistent of reviewers.
>>
>>> Originally I thought you wanted all objects in the filesystem to get the
>>> same label as the backing store. That's what I tried to implement
>>> originally, i.e. smk_root=smk_default=smk_of_inode(...->bd_inode), then
>>> assign every object (new and existing) smk_default and completely ignore
>>> the labels on disk.
>> I want everything to have the label of the backing store, but
>> I don't want to ignore it if it somehow got something else. Because
>> the only legitimate label for this example is Rubble, I want to
>> reject anything else that appears. If someone builds a filesystem
>> by hand with Slate labels I want it treated "safely".
>>
>>> This is what I currently think you want for user ns mounts:
>>>
>>>  1. smk_root and smk_default are assigned the label of the backing
>>>     device.
>>>  2. s_root is assigned the transmute property.
>>>  3. For existing files:
>>>     a. Files with the same label as the backing device are accessible.
>>>     b. Files with any other label are not accessible.
>> That's right. Accept correct data, reject anything that's not right.
>>
>>> If this is right, there are a couple lingering questions in my mind.
>>>
>>> First, what happens with files created in directories with the same
>>> label as the backing device but without the transmute property set? The
>>> inode for the new file will initially be labeled with smk_of_current(),
>>> but then during d_instantiate it will get smk_default and thus end up
>>> with the label we want. So that seems okay.
>> Yes.
>>
>>> The second is whether files with the SMACK64EXEC attribute is still a
>>> problem. It seems it is, for files with the same label as the backing
>>> store at least. I think we can simply skip the code that reads out this
>>> xattr and sets smk_task for user ns mounts, or else skip assigning the
>>> label to the new task in bprm_set_creds. The latter seems more
>>> consistent with the approach you've suggested for dealing with labels
>>> from disk.
>> Yes, I think that skipping the smk_fetch(XATTR_NAME_SMACKEXEC, ...) in
>> smack_d_instantiate for unprivileged mounts would do the trick.
>>
>>> So I guess all of that seems okay, though perhaps a bit restrictive
>>> given that the user who mounted the filesystem already has full access
>>> to the backing store.
>> In truth, there is no reason to expect that the "user" who did the
>> mount will ever have a Smack label that differs from the label of
>> the backing store. If what we've got here seems restrictive, it's
>> because you've got access from someone other than the "user".
>>
>>> Please let me know whether or not this matches up with what you are
>>> thinking, then I can procede with the implementation.
>> My current mindset is that, if you're going to allow unprivileged
>> mounts of user defined backing stores, this is as safe as we can
>> make it.
> That actually sounds very reasonable to me.  It is essentially what we
> do with uid and gids already.  I presume the smack namespace support
> would when integrated with all of this would allow a set of labels to be
> set.
>
> Have I missed a part of the conversation you talk about fileystems that
> don't have support for storing labels?  Filesystems like vfat, isofs,
> etc.

They are easier. Set smackfsroot=Rubble,smackfsdef=Rubble and all objects
there will get labeled Rubble. Processes with different labels that can
write there will end up creating Rubble objects. For privileged mounts you
can set the values at will. For unprivileged mounts, you should take the
label values from the backing store.

>
> Eric
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[toc] | [prev] | [next] | [standalone]

#1194660

On Wed, Jul 22, 2015 at 05:05:17PM -0700, Casey Schaufler wrote:
> > This is what I currently think you want for user ns mounts:
> >
> >  1. smk_root and smk_default are assigned the label of the backing
> >     device.
> >  2. s_root is assigned the transmute property.
> >  3. For existing files:
> >     a. Files with the same label as the backing device are accessible.
> >     b. Files with any other label are not accessible.
> 
> That's right. Accept correct data, reject anything that's not right.
> 
> > If this is right, there are a couple lingering questions in my mind.
> >
> > First, what happens with files created in directories with the same
> > label as the backing device but without the transmute property set? The
> > inode for the new file will initially be labeled with smk_of_current(),
> > but then during d_instantiate it will get smk_default and thus end up
> > with the label we want. So that seems okay.
> 
> Yes.
> 
> > The second is whether files with the SMACK64EXEC attribute is still a
> > problem. It seems it is, for files with the same label as the backing
> > store at least. I think we can simply skip the code that reads out this
> > xattr and sets smk_task for user ns mounts, or else skip assigning the
> > label to the new task in bprm_set_creds. The latter seems more
> > consistent with the approach you've suggested for dealing with labels
> > from disk.
> 
> Yes, I think that skipping the smk_fetch(XATTR_NAME_SMACKEXEC, ...) in
> smack_d_instantiate for unprivileged mounts would do the trick.
> 
> > So I guess all of that seems okay, though perhaps a bit restrictive
> > given that the user who mounted the filesystem already has full access
> > to the backing store.
> 
> In truth, there is no reason to expect that the "user" who did the
> mount will ever have a Smack label that differs from the label of
> the backing store. If what we've got here seems restrictive, it's
> because you've got access from someone other than the "user".
> 
> > Please let me know whether or not this matches up with what you are
> > thinking, then I can procede with the implementation.
> 
> My current mindset is that, if you're going to allow unprivileged
> mounts of user defined backing stores, this is as safe as we can
> make it.

All right, I've got a patch which I think does this, and I've managed to
do some testing to confirm that it behaves like I expect. How does this
look?

What's missing is getting the label from the block device inode; as
Stephen discovered the inode that I thought we could get the label from
turned out to be the wrong one. Afaict we would need a new hook in order
to do that, so for now I'm using the label of the proccess calling
mount.

---

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index a143328f75eb..8e631a66b03c 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -662,6 +662,8 @@ static int smack_sb_kern_mount(struct super_block *sb, int flags, void *data)
 		skp = smk_of_current();
 		sp->smk_root = skp;
 		sp->smk_default = skp;
+		if (sb_in_userns(sb))
+			transmute = 1;
 	}
 	/*
 	 * Initialize the root inode.
@@ -1023,6 +1025,12 @@ static int smack_inode_permission(struct inode *inode, int mask)
 	if (mask == 0)
 		return 0;
 
+	if (sb_in_userns(inode->i_sb)) {
+		struct superblock_smack *sbsp = inode->i_sb->s_security;
+		if (smk_of_inode(inode) != sbsp->smk_root)
+			return -EACCES;
+	}
+
 	/* May be droppable after audit */
 	if (no_block)
 		return -ECHILD;
@@ -3220,14 +3228,16 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
 			if (rc >= 0)
 				transflag = SMK_INODE_TRANSMUTE;
 		}
-		/*
-		 * Don't let the exec or mmap label be "*" or "@".
-		 */
-		skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp);
-		if (IS_ERR(skp) || skp == &smack_known_star ||
-		    skp == &smack_known_web)
-			skp = NULL;
-		isp->smk_task = skp;
+		if (!sb_in_userns(inode->i_sb)) {
+			/*
+			 * Don't let the exec or mmap label be "*" or "@".
+			 */
+			skp = smk_fetch(XATTR_NAME_SMACKEXEC, inode, dp);
+			if (IS_ERR(skp) || skp == &smack_known_star ||
+			    skp == &smack_known_web)
+				skp = NULL;
+			isp->smk_task = skp;
+		}
 
 		skp = smk_fetch(XATTR_NAME_SMACKMMAP, inode, dp);
 		if (IS_ERR(skp) || skp == &smack_known_star ||
--
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/

[toc] | [prev] | [next] | [standalone]

#1186821

On Thu, Jul 16, 2015 at 02:42:22PM -0700, Casey Schaufler wrote:

<snip>

> > I welcome feedback about anything I've missed, but stating generally
> > that you think I probably missed something isn't very helpful.
> 
> True enough. I hope I've explained myself above.

Thanks, that definitely clarified where we were having a disconnect.
Andy's done a fantastic job explaining how those concerns are addressed.

> > The LSM issue is thornier than the rest of it though, which is why I
> > specifically asked for review there in the cover letter. There's a lot
> > of complexity and nuance, and I still don't have a grasp on all the
> > subtleties. One such subtlety is the full impact of simply ignoring the
> > security labels on disk (but I am still confused as to why this is
> > different from filesystems which don't support xattrs at all).
> 
> If you can mount a filesystem such that the labels are ignored you
> are effectively specifying that the Smack label on the files be 
> determined by the defaulting rules. With CAP_MAC_ADMIN that's fine.
> Without it, it's not.
> 
> > I was unaware of Lukasz's patches until yesterday, and I will have a
> > look at them. But since we don't have the LSM support for user
> > namespaces yet, I don't see the problem with doing something safe for
> > LSMs initially and evolving the LSM integration for user ns mounts along
> > with the rest of the user ns integration.
> 
> Ignoring the security attributes is not safe!

Understood. It's surely safe for each LSM to deny such mounts until it
has a way to handle them safely though.

I'm not trying to completely punt on the issue of security modules, just
break this down into more manageable chunks. You've given good guidance
for Smack (thanks very much for that), so I can plan to work on that
soon.

> > Your point is taken about my less-than-expert opinion about the other
> > security modules. We should at minimum get acks from the maintainers of
> > those modules that unprivileged mounts will not compromise MAC.
> 
> I am the Smack maintainer. Unprivileged mounts as you have
> described them compromise MAC. They compromise DAC, too.

It looks like Andy's more or less convinced you that DAC isn't
(additionally?) compromised. And there's a plan for MAC, that the
security module can deny mounts from user namespaces until it has a
solution for allowing them safely.

> > For Smack specifically, I believe my only concern was the SMACK64EXEC
> > attribute, as all the other attributes only affected subjects' access to
> > the files. So maybe it would be possible to simply ignore this attribute
> > in unprivileged mounts and respect the others, even lacking more
> > complete LSM support for user namespaces.
> 
> SMACK64EXEC is analogous to the setuid bit, but I would rather see
> exec() of programs with this attribute refused that for it to be
> blindly ignored.

That's fine, it's your call.

Thanks,
Seth
--
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/

[toc] | [prev] | [next] | [standalone]

#1186035

On Thu, Jul 16, 2015 at 08:59:47AM -0500, Seth Forshee wrote:
> On Wed, Jul 15, 2015 at 10:15:21PM -0500, Eric W. Biederman wrote:
> > 
> > Seth I think for the LSMs we should start with:
> > 
> > diff --git a/security/security.c b/security/security.c
> > index 062f3c997fdc..5b6ece92a8e5 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -310,6 +310,8 @@ int security_sb_statfs(struct dentry *dentry)
> >  int security_sb_mount(const char *dev_name, struct path *path,
> >                         const char *type, unsigned long flags, void *data)
> >  {
> > +       if (current_user_ns() != &init_user_ns)
> > +               return -EPERM;
> >         return call_int_hook(sb_mount, 0, dev_name, path, type, flags, data);
> >  }
> 
> This just makes it impossible to mount from a user namespace. Every
> mount from current_user_ns() != &init_user_ns will fail.

What might work instead is to add a check in security_sb_kern_mount.
Then it would need to check s_user_ns, that way if proc, sysfs, etc.
use sget_userns(..., &init_user_ns) they can still be mounted in
containers.

It would be nicer to have a hook after sget but before fill_super so
that a bunch of work doesn't have to be done and then undone. Right now
there doesn't seem to be any suitable hook.

> > Then we should push this down into all of the lsms.
> > Then when we should remove or relax or change the check as appropriate
> > in each lsm.
> > 
> > The point is this is good enough to see that it is trivially safe,
> > and this allows us to focus on the core issues, and stop worrying about
> > the lsms for a bit.
> > 
> > Then we can focus on each lsm one at at time and take the time to really
> > understand them and talk with their maintainers etc to make certain
> > we get things correct.
> > 
> > This should remove the need for your patches 5, 6 and 7. For the
> > immediate future.
> 
> I'm still not entirely sure what you were trying to do, maybe refuse to
> mount whenever a security module is loaded? I think this could be a good
> option to start, but couldn't we restrict it to only the LSMs which use
> xattrs for security labels? In situations where the filesystem cannot
> supply security policy metadata I can't think of any reason to disallow
> the mounts.
> 
> Seth
--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web