Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Andreas Dilger]

On Jul 13, 2017, at 3:58 PM, Eric Biggers  wrote:
> 
> Hi Michael,
> 
> On Thu, Jul 13, 2017 at 03:29:44PM -0700, Michael Halcrow wrote:
>> On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
>>> From: Eric Biggers 
>>> 
>>> Currently, the fscrypt_context (i.e. the encryption xattr) does not
>>> contain a cryptographically secure identifier for the master key's
>>> payload.  Therefore it's not possible to verify that the correct key was
>>> supplied, which is problematic in multi-user scenarios.  To make this
>>> possible, define a new fscrypt_context version (v2) which includes a
>>> key_hash field, and allow userspace to opt-in to it when setting an
>>> encryption policy by setting fscrypt_policy.version to 2.  For now just
>>> zero the new field; a later patch will start setting it for real.
>> 
>> The main concern that comes to mind is potentially blowing past the
>> inline xattr size limit and allocating a new inode block.  The
>> security benefit probably outweighs that concern in this case.
>> 
> 
> The way it adds up now for ext4 is:
> 
> 128 bytes for base inode
> + 32 bytes for i_extra fields
> + 4 bytes for in-inode xattrs header
> + 20 bytes for encryption xattr header + name
> + 28 bytes for encryption xattr value
> --
> = 212 bytes total.
> 
> By adding the 16-byte 'key_hash' field it grows to 228 bytes total.  So it 
> still
> fits in a 256-byte inode, though it's getting closer to the limit.  We could
> save 8 bytes by instead using the design where master_key_descriptor is 
> extended
> to 16 bytes and redefined as a cryptographically secure hash.  But as noted,
> that has some significant disadvantages.
> 
> Also note that we don't really have to worry about leaving space for a SELinux
> xattr anymore because with 256-byte inodes + encryption the SELinux xattr is
> already being written to an external block, given that it requires about 52-62
> bytes (at least when using Android's SELinux policy; different SELinux 
> policies
> may use different values), and 212 + 52 > 256.  So if someone wants both 
> xattrs
> in-inode they need to use 512-byte inodes already.

It is probably time to consider changing to a default of 512-byte inodes for
larger filesystems anyway.  In our testing, this affected performance only by
a couple of percent under normal usage, and avoided a significant performance
drop if the xattrs ever fall out of the inode.

Cheers, Andreas







signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Eric Biggers]

Hi Michael,

On Thu, Jul 13, 2017 at 03:29:44PM -0700, Michael Halcrow wrote:
> On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
> > From: Eric Biggers 
> > 
> > Currently, the fscrypt_context (i.e. the encryption xattr) does not
> > contain a cryptographically secure identifier for the master key's
> > payload.  Therefore it's not possible to verify that the correct key was
> > supplied, which is problematic in multi-user scenarios.  To make this
> > possible, define a new fscrypt_context version (v2) which includes a
> > key_hash field, and allow userspace to opt-in to it when setting an
> > encryption policy by setting fscrypt_policy.version to 2.  For now just
> > zero the new field; a later patch will start setting it for real.
> 
> The main concern that comes to mind is potentially blowing past the
> inline xattr size limit and allocating a new inode block.  The
> security benefit probably outweighs that concern in this case.
> 

The way it adds up now for ext4 is:

128 bytes for base inode
+ 32 bytes for i_extra fields
+ 4 bytes for in-inode xattrs header
+ 20 bytes for encryption xattr header + name
+ 28 bytes for encryption xattr value
--
= 212 bytes total.

By adding the 16-byte 'key_hash' field it grows to 228 bytes total.  So it still
fits in a 256-byte inode, though it's getting closer to the limit.  We could
save 8 bytes by instead using the design where master_key_descriptor is extended
to 16 bytes and redefined as a cryptographically secure hash.  But as noted,
that has some significant disadvantages.

Also note that we don't really have to worry about leaving space for a SELinux
xattr anymore because with 256-byte inodes + encryption the SELinux xattr is
already being written to an external block, given that it requires about 52-62
bytes (at least when using Android's SELinux policy; different SELinux policies
may use different values), and 212 + 52 > 256.  So if someone wants both xattrs
in-inode they need to use 512-byte inodes already.

Eric

Re: [PATCH 1/6] fscrypt: add v2 encryption context and policy

[Michael Halcrow]

On Wed, Jul 12, 2017 at 02:00:30PM -0700, Eric Biggers wrote:
> From: Eric Biggers 
> 
> Currently, the fscrypt_context (i.e. the encryption xattr) does not
> contain a cryptographically secure identifier for the master key's
> payload.  Therefore it's not possible to verify that the correct key was
> supplied, which is problematic in multi-user scenarios.  To make this
> possible, define a new fscrypt_context version (v2) which includes a
> key_hash field, and allow userspace to opt-in to it when setting an
> encryption policy by setting fscrypt_policy.version to 2.  For now just
> zero the new field; a later patch will start setting it for real.

The main concern that comes to mind is potentially blowing past the
inline xattr size limit and allocating a new inode block.  The
security benefit probably outweighs that concern in this case.

> Even though we aren't changing the layout of struct fscrypt_policy (i.e.
> the struct used by the ioctls), the new context version still has to be
> "opt-in" because old kernels will not recognize it, and the keyring key
> will now need to be available when setting an encryption policy, which
> is an API change.  We'll also be taking the opportunity to make another
> API change (dropping support for the filesystem-specific key prefixes).
> 
> Previously, the version numbers were 0 in the fscrypt_policy and 1 in
> the fscrypt_context.  Rather than incrementing them to 1 and 2, make
> them both 2 to be consistent with each other.  It's not required that
> these numbers match, but it should make things less confusing.
> 
> An alternative to adding a key_hash field would have been to reuse
> master_key_descriptor.  However, master_key_descriptor is only 8 bytes,
> which is too short to be a cryptographically secure hash.  Thus,
> master_key_descriptor would have needed to be lengthened to 16+ bytes,
> which would have required defining a fscrypt_policy_v2 structure and
> adding a FS_IOC_GET_ENCRYPTION_POLICY_V2 ioctl.  It also would have
> required userspace to start using a specific hash algorithm to create
> the key descriptors, which would have made the API harder to use.
> Perhaps it should have been done that way originally, but at this point
> it seems better to keep the API simpler.
> 
> Signed-off-by: Eric Biggers 

Acked-by: Michael Halcrow 

> ---
>  fs/crypto/fscrypt_private.h| 79 
> ++
>  fs/crypto/keyinfo.c| 14 
>  fs/crypto/policy.c | 67 ++-
>  include/linux/fscrypt_common.h |  2 +-
>  include/uapi/linux/fs.h|  6 
>  5 files changed, 127 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h
> index a1d5021c31ef..ef6909035823 100644
> --- a/fs/crypto/fscrypt_private.h
> +++ b/fs/crypto/fscrypt_private.h
> @@ -25,39 +25,88 @@
>  #define FS_AES_256_XTS_KEY_SIZE  64
>  
>  #define FS_KEY_DERIVATION_NONCE_SIZE 16

I'm seeing tab misalignment from all the other values here.  Maybe
remove the extra tab while you're at it?

> +#define FSCRYPT_KEY_HASH_SIZE16
>  
>  /**
> - * Encryption context for inode
> + * fscrypt_context - the encryption context for an inode
>   *
> - * Protector format:
> - *  1 byte: Protector format (1 = this version)
> - *  1 byte: File contents encryption mode
> - *  1 byte: File names encryption mode
> - *  1 byte: Flags
> - *  8 bytes: Master Key descriptor
> - *  16 bytes: Encryption Key derivation nonce
> + * Filesystems usually store this in an extended attribute.  It identifies 
> the
> + * encryption algorithm and key with which the file is encrypted.
>   */
>  struct fscrypt_context {
> - u8 format;
> + /* v1+ */
> +
> + /* Version of this structure */
> + u8 version;
> +
> + /* Encryption mode for the contents of regular files */
>   u8 contents_encryption_mode;
> +
> + /* Encryption mode for filenames in directories and symlink targets */
>   u8 filenames_encryption_mode;
> +
> + /* Options that affect how encryption is done (e.g. padding amount) */
>   u8 flags;
> +
> + /* Descriptor for this file's master key in the keyring */
>   u8 master_key_descriptor[FS_KEY_DESCRIPTOR_SIZE];
> +
> + /*
> +  * A unique value used in combination with the master key to derive the
> +  * file's actual encryption key
> +  */
>   u8 nonce[FS_KEY_DERIVATION_NONCE_SIZE];
> -} __packed;
>  
> -#define FS_ENCRYPTION_CONTEXT_FORMAT_V1  1
> + /* v2+ */
> +
> + /* Cryptographically secure hash of the master key */
> + u8 key_hash[FSCRYPT_KEY_HASH_SIZE];

Please add a comment not to re-order without macro changes below.

> +};
> +
> +#define FSCRYPT_CONTEXT_V1   1
> +#define FSCRYPT_CONTEXT_V1_SIZE  offsetof(struct fscrypt_context, 
> key_hash)
> +
> +#define FSCRYPT_CONTEXT_V2   2
> +#define