Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Christoph Hellwig]

On Thu, Apr 10, 2014 at 08:19:48AM -0400, Brian Foster wrote:
> Are you saying it doesn't have to initialize security or the default
> acl, or both?

The ACLs for sure.  LSM do run-time access decisions, so they will
probably rely on the security data being initialized.  Given that
O_TMPFILE files aren't publicly available I'm not sure there's a point
in them doing that, though.

LSMs are also affected by the lack of a proper parent I'll discuss for
ACLs below.

> The intent here was to have the case covered where the inode happens to
> be linked back into the namespace since we don't do this work in the
> link path.

That's an interesting one.  O_TMFILE files don't have a real parent
to inherit ACLs from, the pathname passed in just needs to point to
a directory to find the filesystem to create the tmpfile in.  On
the other hand it seem like the extN implementations do inherity the
ACL in this case.

The link into the namespace is irrelavant here as ACL inheritance only
happens on initial create, not at link time.

I also think we'll absolutely need a test case for ACLs+tmpfile to
make sure all filesystems handle it the same way.

> The bulk of the refactoring was with the idea that the inode setup for
> the tmpfile case is generally equivalent for the traditional create
> case. The original version was posted here:
> 
> http://oss.sgi.com/archives/xfs/2014-04/msg00149.html
> 
> ... and it just fixes the deadlock and adds the security initialization.
> I suppose I could still break that out into multiple patches, but that
> aside, is that behavior preferred?

I think just fixing the deadlock and initializing the security is enough
for the first pass.  If you want to do the refactoring on top send it as
a second series on top of the actual fixes.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Christoph Hellwig]

So any opinions from other fs / security people on how O_TMPFILE files
should behave for ACL inheritance / labeling?

On Thu, Apr 10, 2014 at 05:29:44AM -0700, Christoph Hellwig wrote:
> On Thu, Apr 10, 2014 at 08:19:48AM -0400, Brian Foster wrote:
> > Are you saying it doesn't have to initialize security or the default
> > acl, or both?
> 
> The ACLs for sure.  LSM do run-time access decisions, so they will
> probably rely on the security data being initialized.  Given that
> O_TMPFILE files aren't publicly available I'm not sure there's a point
> in them doing that, though.
> 
> LSMs are also affected by the lack of a proper parent I'll discuss for
> ACLs below.
> 
> > The intent here was to have the case covered where the inode happens to
> > be linked back into the namespace since we don't do this work in the
> > link path.
> 
> That's an interesting one.  O_TMFILE files don't have a real parent
> to inherit ACLs from, the pathname passed in just needs to point to
> a directory to find the filesystem to create the tmpfile in.  On
> the other hand it seem like the extN implementations do inherity the
> ACL in this case.
> 
> The link into the namespace is irrelavant here as ACL inheritance only
> happens on initial create, not at link time.
> 
> I also think we'll absolutely need a test case for ACLs+tmpfile to
> make sure all filesystems handle it the same way.
> 
> > The bulk of the refactoring was with the idea that the inode setup for
> > the tmpfile case is generally equivalent for the traditional create
> > case. The original version was posted here:
> > 
> > http://oss.sgi.com/archives/xfs/2014-04/msg00149.html
> > 
> > ... and it just fixes the deadlock and adds the security initialization.
> > I suppose I could still break that out into multiple patches, but that
> > aside, is that behavior preferred?
> 
> I think just fixing the deadlock and initializing the security is enough
> for the first pass.  If you want to do the refactoring on top send it as
> a second series on top of the actual fixes.
> 
> _______________________________________________
> xfs mailing list
> xfs-VZNHf3L845pBDgjK7y7TUQ@public.gmane.org
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Andreas Gruenbacher]

Christoph,

On Tue, Apr 15, 2014 at 10:52:28AM -0700, Christoph Hellwig wrote:
> So any opinions from other fs / security people on how O_TMPFILE files
> should behave for ACL inheritance / labeling?

from how O_TMPFILE is documented right now [*], creating such a file and
then linking it into the namespace is one of the obvious use cases.  The
intent seems to be to make it seem like the file was created and populated
atomically, possibly with inherited permissions and all.  I think this
behavior require that the O_TMPFILE file inherits from the directory it
was "created" in.

Adding code to achieve the effect of create-time inheritance at link
time, only for O_TMPFILE files or files without any links, doesn't seem 
reasonable to me: it would duplicate create code in the link code path,
and it would make it harder to override inherited permissions or labels.

(Trying to fake inheritance by reimplementing it in user space seems like
a much worse idea still.)

[*] http://man7.org/linux/man-pages/man2/open.2.html

Thanks,
Andreas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Christoph Hellwig]

On Tue, Apr 15, 2014 at 09:31:02PM +0200, Andreas Gruenbacher wrote:
> from how O_TMPFILE is documented right now [*], creating such a file and
> then linking it into the namespace is one of the obvious use cases.

Btw, I think the man page is wrong - given that the tmpfile is not
visible in the namespace it is obviously not created in the directory.
The directory passed in is just a handle for the filesystem it should be
created in.

Michael, should I send you an update for this, or do you want to do it
yourself as you can probably come up with better language anyway?

> The
> intent seems to be to make it seem like the file was created and populated
> atomically, possibly with inherited permissions and all.
> I think this
> behavior require that the O_TMPFILE file inherits from the directory it
> was "created" in.
> 
> Adding code to achieve the effect of create-time inheritance at link
> time, only for O_TMPFILE files or files without any links, doesn't seem 
> reasonable to me: it would duplicate create code in the link code path,
> and it would make it harder to override inherited permissions or labels.

Inheriting any ACL on creating an anonymous file seems utterly wrong.
Inheriting on link seems somewhat more sensible and not too bad in terms
of code, but very confusing in terms of semantics.  I think the best
method is to make sure it simply does not inherit any ACL and document
that clearly.

> (Trying to fake inheritance by reimplementing it in user space seems like
> a much worse idea still.)

We don't fake inheritance when linking any other file either.  And
creating a file in a /tmp without any ACL and then linking it into the
filesystem already is very common today.

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Andreas Gruenbacher]

On Wed, Apr 16, 2014 at 04:14:02AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 15, 2014 at 09:31:02PM +0200, Andreas Gruenbacher wrote:
> > from how O_TMPFILE is documented right now [*], creating such a file and
> > then linking it into the namespace is one of the obvious use cases.
> 
> Btw, I think the man page is wrong - given that the tmpfile is not
> visible in the namespace it is obviously not created in the directory.
> The directory passed in is just a handle for the filesystem it should be
> created in.

I don't agree.  If the file is created with O_TMPFILE | O_EXCL, it is clear
that the file will never be linked into the namespace.  Even then, there are
operations which are affected by the inode permissions and label of the
anonymous file, and those should still behave reasonably.  In this context,
I would expect them to behave as if the file was actually created in the
specified directory, not in the file system root or "nowhere" with no clearly
defined permissions and security label.

If the file is created without O_EXCL, then it is clear that the file can
later become part of the namespace.  In that case, I would also expect the
file to be initialized in the context of the specified directory.  The file
can then be filled with data, permissions and other attributes can be changed
if desired, and then the anonymous file can be linked into that directory.

> Michael, should I send you an update for this, or do you want to do it
> yourself as you can probably come up with better language anyway?
> 
> > The
> > intent seems to be to make it seem like the file was created and populated
> > atomically, possibly with inherited permissions and all.
> > I think this
> > behavior require that the O_TMPFILE file inherits from the directory it
> > was "created" in.
> > 
> > Adding code to achieve the effect of create-time inheritance at link
> > time, only for O_TMPFILE files or files without any links, doesn't seem 
> > reasonable to me: it would duplicate create code in the link code path,
> > and it would make it harder to override inherited permissions or labels.
> 
> Inheriting any ACL on creating an anonymous file seems utterly wrong.

Why?

> Inheriting on link seems somewhat more sensible and not too bad in terms
> of code, but very confusing in terms of semantics.  I think the best
> method is to make sure it simply does not inherit any ACL and document
> that clearly.

Again, this approach would make it almost impossible to use O_TMPFILE as a
way to atomically create and initialize a file with permission and security
label inheritance.  This would severely limit the usefulness of O_TMPFILE.

> > (Trying to fake inheritance by reimplementing it in user space seems like
> > a much worse idea still.)
> 
> We don't fake inheritance when linking any other file either.  And
> creating a file in a /tmp without any ACL and then linking it into the
> filesystem already is very common today.

Well, creating a file in /tmp and moving it somewhere else often doesn't even
work because of file system boundaries, and sometimes it simply is the wrong
thing to do:

When the intent is to "create a new file in a directory", such as when saving to
a new file in an editor, the result should be as if the file was actually
created in that directory.  Creating the file in /tmp and then moving it to
where it should end up is simply wrong.

When the intent is to "move a file from here to there", the file should keep
all its attributes, including permissions and security label. It should make no
difference in result whether the file could actually be moved or if it had to
be copied across file system boundaries.

Thanks,
Andreas

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2 1/2] xfs: fix tmpfile/selinux deadlock and initialize security/acl

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 07:29:29PM +0200, Andreas Gruenbacher wrote:
> > Btw, I think the man page is wrong - given that the tmpfile is not
> > visible in the namespace it is obviously not created in the directory.
> > The directory passed in is just a handle for the filesystem it should be
> > created in.
> 
> I don't agree.  If the file is created with O_TMPFILE | O_EXCL, it is clear
> that the file will never be linked into the namespace.  Even then, there are
> operations which are affected by the inode permissions and label of the
> anonymous file, and those should still behave reasonably.  In this context,
> I would expect them to behave as if the file was actually created in the
> specified directory, not in the file system root or "nowhere" with no clearly
> defined permissions and security label.

So you want to define the files as being in a directory, but not
actually visible?  That's defintively a new and strange state to be in.

> > Inheriting any ACL on creating an anonymous file seems utterly wrong.
> 
> Why?

Because it has no parent to inherit it from.

--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html