fs/crypt ioctl and attr

fs/crypt ioctl and attr

[Anand Jain]

Hello,

  As I understand the usage of

     EXT4_IOC_SET(GET)_ENCRYPTION_POLICY

  and as the policy is set per inode and stored as an attribute,
  any reason that - it should be a selinux attribute in specific ?
  and setfattr / getfattr wasn't used instead of the above ioctl ?

  IMO, if its attribute its better to have it read/writable
  using the generic tools such as set(get)fattr. ?

Thanks, Anand

Re: fs/crypt ioctl and attr

[Eric Biggers]

Hi Anand,

On Tue, Feb 07, 2017 at 07:19:00AM +0800, Anand Jain wrote:
> 
> Hello,
> 
>  As I understand the usage of
> 
>     EXT4_IOC_SET(GET)_ENCRYPTION_POLICY
> 
>  and as the policy is set per inode and stored as an attribute,
>  any reason that - it should be a selinux attribute in specific ?
>  and setfattr / getfattr wasn't used instead of the above ioctl ?
> 
>  IMO, if its attribute its better to have it read/writable
>  using the generic tools such as set(get)fattr. ?
> 
> Thanks, Anand

The xattr which stores the fscrypt_context, which is the on-disk equivalent of a
fscrypt_policy, is not exposed through the extended attributes syscalls such as
getxattr() and setxattr().  This is by design, for various reasons.

First, there are specific rules around when and how userspace may enable
encryption.  For example, encryption can only be enabled on an empty directory,
it's not supported to "unencrypt" an encrypted file or directory in-place,
filesystems may require setting an inode flag when the fscrypt_context is
created, filesystems may require that a superblock flag has been enabled first,
and only specific encryption modes and flags are currently supported and
therefore other ones should be rejected.

Also, it's helpful to decouple the structure passed into the API from the
on-disk representation, for various reasons but currently mainly because the
on-disk xattr includes a nonce, which is generated by the filesystem using
get_random_bytes().  It's not a good idea to force userspace to generate the
nonce because then every program using the API has to get it right, instead of
just the filesystem.

Eric

Re: fs/crypt ioctl and attr

[Anand Jain]

Hi Eric,

>>  As I understand the usage of
>>
>>     EXT4_IOC_SET(GET)_ENCRYPTION_POLICY
>>
>>  and as the policy is set per inode and stored as an attribute,
>>  any reason that - it should be a selinux attribute in specific ?
>>  and setfattr / getfattr wasn't used instead of the above ioctl ?
>>
>>  IMO, if its attribute its better to have it read/writable
>>  using the generic tools such as set(get)fattr. ?
>>
>> Thanks, Anand
>
> The xattr which stores the fscrypt_context, which is the on-disk equivalent of a
> fscrypt_policy, is not exposed through the extended attributes syscalls such as
> getxattr() and setxattr().  This is by design, for various reasons.
>
> First, there are specific rules around when and how userspace may enable
> encryption.  For example, encryption can only be enabled on an empty directory,
> it's not supported to "unencrypt" an encrypted file or directory in-place,
> filesystems may require setting an inode flag when the fscrypt_context is
> created, filesystems may require that a superblock flag has been enabled first,
> and only specific encryption modes and flags are currently supported and
> therefore other ones should be rejected.

  These reasons would equally apply to ioctl interface as well.
  I don't see this being limiting factor for not using set/getfattr.

> Also, it's helpful to decouple the structure passed into the API from the
> on-disk representation, for various reasons but currently mainly because the
> on-disk xattr includes a nonce, which is generated by the filesystem using
> get_random_bytes().  It's not a good idea to force userspace to generate the
> nonce because then every program using the API has to get it right, instead of
> just the filesystem.

  Again. I don't see these as the limiting factor for not using
  set/getfattr. And about the iv/nonce you mentioned, I didn't
  say to set IV/nonce value from the user land, but it could have
  been done the same way the attribute inheritance is working now
  with in the fs/crypto kernel.

  BTRFS does not have ioctl for the crypto policy set, it did it
  through xattr, I would like to retain that part even when fs/crypto
  is used, unless there is any security reason. (But definitely not at
  the cost of being incompatible with rest of the FSs, I hope to fix
  that part as well).


  Here, I am just trying to ensure not to over-do in the name of
  security.
  And by letting encryption meta-data easily usable through extended
  attributes I hope to encourage tools to leverage on the FS encryption.


  Still my question is unanswered, I was specific to any security
  or other reasons that set/getfattr interface wasn't used to
  read/write the policy information. And also why SElinux attributes
  instead of normal extended attributes.


Thanks, Anand

Re: fs/crypt ioctl and attr

[Eric Biggers]

Hi Anand,

On Tue, Feb 07, 2017 at 01:55:03PM +0800, Anand Jain wrote:
> 
>  These reasons would equally apply to ioctl interface as well.
>  I don't see this being limiting factor for not using set/getfattr.
> 
>  Again. I don't see these as the limiting factor for not using
>  set/getfattr. And about the iv/nonce you mentioned, I didn't
>  say to set IV/nonce value from the user land, but it could have
>  been done the same way the attribute inheritance is working now
>  with in the fs/crypto kernel.
> 
>  BTRFS does not have ioctl for the crypto policy set, it did it
>  through xattr, I would like to retain that part even when fs/crypto
>  is used, unless there is any security reason. (But definitely not at
>  the cost of being incompatible with rest of the FSs, I hope to fix
>  that part as well).
> 
> 
>  Here, I am just trying to ensure not to over-do in the name of
>  security.
>  And by letting encryption meta-data easily usable through extended
>  attributes I hope to encourage tools to leverage on the FS encryption.
> 
> 
>  Still my question is unanswered, I was specific to any security
>  or other reasons that set/getfattr interface wasn't used to
>  read/write the policy information. And also why SElinux attributes
>  instead of normal extended attributes.
> 

I think you're partially correct because setxattr() and getxattr() can actually
do more than just setting/getting the on-disk bytes, depending on what xattr
handers the filesystem provides.  (I had assumed you were referring to
"non-magic" xattrs, like the ones in the "user." namespace.)  For example, many
filesystems have a handler for the POSIX ACL extended attributes
("system.posix_acl_access" and "system.posix_acl_default"), and these have
special behavior.  BTRFS also provides a handler for the "btrfs." namespace and
allows setting/getting "btrfs.compression", also with special behavior.

This could have been done for the fscrypt API, I think, provided that an
extended attribute in the "system." namespace, or in a dedicated namespace like
"fscrypt.", was used.  This would have allow taking advantage of the setxattr()
infrastructure and may have helped prevent some of the bugs I've had to fix in
FS_IOC_SET_ENCRYPTION_POLICY.

However, the issue which I was getting at is that encryption might just diverge
from the standard xattr interface too much for it to be appropriate.  For
example you would not be able to use removexattr() to remove the encryption
xattr.  Nor would the "replace" semantics of setxattr() work.  Nor would
'getfattr --dump' and 'setfattr --restore' work, because the encryption xattrs
would be included in the xattr dump but would not, in general, be possible to
restore.  Also, if the API requires a binary structure, then you kind of need a
custom program to use it anyway, and in that situation an ioctl() is just as
easy (if not easier) to use.

Anyway, if your intent is to change the fscrypt API, note that this almost
certainly can't be done because people are already using it.  It would however
be technically possible to add a new API and deprecate the old one, if there was
a strong reason that made it worthwhile.  Right now I am not convinced that
switching to an xattr interface is a good enough reason.

As for why fscrypt uses "SELinux attributes", it doesn't.  The xattrs have a
special name which does not collide with any public namespace.

(Also note that I was not personally involved in the original design of the
fscrypt API at all, so others may know more than me about the design decisions.)

Eric

Re: fs/crypt ioctl and attr

[Anand Jain]

Hi Eric,

> However, the issue which I was getting at is that encryption might just diverge
> from the standard xattr interface too much for it to be appropriate.  For
> example you would not be able to use removexattr() to remove the encryption
> xattr.  Nor would the "replace" semantics of setxattr() work.  Nor would
> 'getfattr --dump' and 'setfattr --restore' work,

>  because the encryption xattrs
> would be included in the xattr dump but would not, in general, be possible to
> restore.

  yeah. I wonder if this is something we could foresee to work in the
  long run for the encrypt attribute. (assuming there is no threat
  due to this).


>  Also, if the API requires a binary structure, then you kind of need a
> custom program to use it anyway, and in that situation an ioctl() is just as
> easy (if not easier) to use.

  btrfs used string which is also in line with /proc/crypto. and a
  separate RO attribute for IV.


> Anyway, if your intent is to change the fscrypt API, note that this almost
> certainly can't be done because people are already using it.  It would however
> be technically possible to add a new API and deprecate the old one, if there was
> a strong reason that made it worthwhile.  Right now I am not convinced that
> switching to an xattr interface is a good enough reason.

  IMO attributes are fundamental of sharing file-info to the rest of the
  stake holders in the OS and that's a strong enough reason.


> As for why fscrypt uses "SELinux attributes", it doesn't.  The xattrs have a
> special name which does not collide with any public namespace.

   Ok. Thanks. I am updated.

> (Also note that I was not personally involved in the original design of the
> fscrypt API at all, so others may know more than me about the design decisions.)

  Sure. Hope I could confirm it here, that is - if its ok to entirely use
  setfattr / getfattr to read and write fs/crypto policy.

Thanks, Anand


> Eric
>

Re: fs/crypt ioctl and attr

[Theodore Ts'o]

On Tue, Feb 07, 2017 at 05:52:15PM +0800, Anand Jain wrote:
> 
>  Sure. Hope I could confirm it here, that is - if its ok to entirely use
>  setfattr / getfattr to read and write fs/crypto policy.

The controls on when it is legal to set a crypto policy on a file are
different than for a normal extended attribute.  Furthermore, what is
stored on disk is a combination of the fscrypt policy --- which is a
fixed part of the userspace API --- and the some per-file
cryptoggraphic nonces which are generated by the kernel, and which is
subject to change in the future depending on the changes in the key
derivation algorithm, the cipher used, etc.

For this reason using the standard xattr interface is **strongly**
frowned upon.  Using setfattr / getfattr for something that *isn't*
the standard user extended attribute has been a mess, and is
considered a mistake that shouldn't be repeated.

This is why I chose to use a separate ioctl instead of trying to
bastardize the setfattr / getfattr interface for something which is
fundamentally *not* about setting an extended attribute.

There was discussion on linux-fsdevel a few months back where someone
else wanted to perpetrate a similar abuse of the the set/get extended
attribute interface, and other folks (I think it was Christoph and
Dave Chinner) who argued very strongly that it wsa a bad, Bad, BAD,
*BAD* idea.

Cheers,

						- Ted

Re: fs/crypt ioctl and attr

[Darrick J. Wong]

On Tue, Feb 07, 2017 at 09:41:00PM -0500, Theodore Ts'o wrote:
> On Tue, Feb 07, 2017 at 05:52:15PM +0800, Anand Jain wrote:
> > 
> >  Sure. Hope I could confirm it here, that is - if its ok to entirely use
> >  setfattr / getfattr to read and write fs/crypto policy.
> 
> The controls on when it is legal to set a crypto policy on a file are
> different than for a normal extended attribute.  Furthermore, what is
> stored on disk is a combination of the fscrypt policy --- which is a
> fixed part of the userspace API --- and the some per-file
> cryptoggraphic nonces which are generated by the kernel, and which is
> subject to change in the future depending on the changes in the key
> derivation algorithm, the cipher used, etc.
> 
> For this reason using the standard xattr interface is **strongly**
> frowned upon.  Using setfattr / getfattr for something that *isn't*
> the standard user extended attribute has been a mess, and is
> considered a mistake that shouldn't be repeated.
> 
> This is why I chose to use a separate ioctl instead of trying to
> bastardize the setfattr / getfattr interface for something which is
> fundamentally *not* about setting an extended attribute.
> 
> There was discussion on linux-fsdevel a few months back where someone
> else wanted to perpetrate a similar abuse of the the set/get extended
> attribute interface, and other folks (I think it was Christoph and
> Dave Chinner) who argued very strongly that it wsa a bad, Bad, BAD,
> *BAD* idea.

As one of 'those people', I think I'm well qualified to say that a fake
xattr having side effects that doesn't behave in quite the same ways as
regular xattrs is setting up other programs (the ones that listxattr all
the attrs and getxattrs them) for all kinds of weird yuckiness.  And by
'weird yuckiness' I mean "this one magic xattr returned EOPNOTSUPP and
the whole backup program exploded".

Now granted you can argue that we're just shifting that into an ioctl,
but ioctls are already weird and yucky. :)

--D

> 
> Cheers,
> 
> 						- Ted

Re: fs/crypt ioctl and attr

[Theodore Ts'o]

On Tue, Feb 07, 2017 at 08:36:13PM -0800, Darrick J. Wong wrote:
> As one of 'those people', I think I'm well qualified to say that a fake
> xattr having side effects that doesn't behave in quite the same ways as
> regular xattrs is setting up other programs (the ones that listxattr all
> the attrs and getxattrs them) for all kinds of weird yuckiness.  And by
> 'weird yuckiness' I mean "this one magic xattr returned EOPNOTSUPP and
> the whole backup program exploded".
> 
> Now granted you can argue that we're just shifting that into an ioctl,
> but ioctls are already weird and yucky. :)

If we were going to do anything at all, it would be to move it to a
syscall.  But given that it took ***years*** for the glibc developers
to get around to adding getrandom(2) to glibc, we'd be stuck using the
syscall(3) interface and having to deal with different syscall numbers
for different architectures (in case we're compiling on a system which
hadn't update the kernel headers in /usr/include), and so at least in
the short term it would actually be worse than using ioctl's.

For similar reasons I don't think there's going to be that much
interest in adding a syscall to replace XFS_IOC_GOINGDOWN.  We'll
probably go with FS_IOC_GOINGDOWN, just as we have with
FS_IOC_SETFLAGS, etc.

Speaking of XFS_IOC_GOINGDOWN, I'm sure the name was coined many, many
years agoo, back when concerns such as "avoiding a hostile working
environment" were much of an issue.  Even so, I note that *someone*
decided that the name that should be exposed to customer (in the
xfs_io man page), was "shutdown", and not "goingdown".  So is there
any objection if we use the name FS_IOC_SHUTDOWN moving forward?

Some might accuse us of being overly concerned about political
correctness, but I have to admit I did have a slight twinge when I
checked in the ext4 shutdown changes into our internal kernel.  I was
worried that there might be some folks who might have found the name
at least a tiny bit offensive.  (Not that anyone complained, but I'd
much rather be conservative in what we send, and liberal in what we
accept.)

	     	     	      	  - Ted

Re: fs/crypt ioctl and attr

[Darrick J. Wong]

On Wed, Feb 08, 2017 at 09:21:27AM -0500, Theodore Ts'o wrote:
> On Tue, Feb 07, 2017 at 08:36:13PM -0800, Darrick J. Wong wrote:
> > As one of 'those people', I think I'm well qualified to say that a fake
> > xattr having side effects that doesn't behave in quite the same ways as
> > regular xattrs is setting up other programs (the ones that listxattr all
> > the attrs and getxattrs them) for all kinds of weird yuckiness.  And by
> > 'weird yuckiness' I mean "this one magic xattr returned EOPNOTSUPP and
> > the whole backup program exploded".
> > 
> > Now granted you can argue that we're just shifting that into an ioctl,
> > but ioctls are already weird and yucky. :)
> 
> If we were going to do anything at all, it would be to move it to a
> syscall.  But given that it took ***years*** for the glibc developers
> to get around to adding getrandom(2) to glibc, we'd be stuck using the
> syscall(3) interface and having to deal with different syscall numbers
> for different architectures (in case we're compiling on a system which
> hadn't update the kernel headers in /usr/include), and so at least in
> the short term it would actually be worse than using ioctl's.
> 
> For similar reasons I don't think there's going to be that much
> interest in adding a syscall to replace XFS_IOC_GOINGDOWN.  We'll
> probably go with FS_IOC_GOINGDOWN, just as we have with
> FS_IOC_SETFLAGS, etc.
> 
> Speaking of XFS_IOC_GOINGDOWN, I'm sure the name was coined many, many
> years agoo, back when concerns such as "avoiding a hostile working
> environment" were much of an issue.  Even so, I note that *someone*
> decided that the name that should be exposed to customer (in the
> xfs_io man page), was "shutdown", and not "goingdown".  So is there
> any objection if we use the name FS_IOC_SHUTDOWN moving forward?

None here.  I think Dave asked for GOINGDOWN -> SHUTDOWN too.

> Some might accuse us of being overly concerned about political
> correctness, but I have to admit I did have a slight twinge when I
> checked in the ext4 shutdown changes into our internal kernel.  I was
> worried that there might be some folks who might have found the name
> at least a tiny bit offensive.  (Not that anyone complained, but I'd
> much rather be conservative in what we send, and liberal in what we
> accept.)

Yes, and I further argue that io control command (ioctl) names should be
imperative.  One doesn't yell 'going down!' to turn off the light
switches. :P

--D

> 
> 	     	     	      	  - Ted
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>