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 

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

[Eric Biggers]

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.

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 
---
 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_SIZE64
 
 #define FS_KEY_DERIVATION_NONCE_SIZE   16
+#define FSCRYPT_KEY_HASH_SIZE  16
 
 /**
- * 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_V11
+   /* v2+ */
+
+   /* Cryptographically secure hash of the master key */
+   u8 key_hash[FSCRYPT_KEY_HASH_SIZE];
+};
+
+#define FSCRYPT_CONTEXT_V1 1
+#define FSCRYPT_CONTEXT_V1_SIZEoffsetof(struct fscrypt_context, 
key_hash)
+
+#define FSCRYPT_CONTEXT_V2 2
+#define FSCRYPT_CONTEXT_V2_SIZEsizeof(struct fscrypt_context)
+
+static inline int fscrypt_context_size(const struct fscrypt_context *ctx)
+{
+   switch (ctx->version) {
+   case FSCRYPT_CONTEXT_V1:
+   return FSCRYPT_CONTEXT_V1_SIZE;
+   case FSCRYPT_CONTEXT_V2:
+   return FSCRYPT_CONTEXT_V2_SIZE;
+   }
+   return 0;
+}
+
+static inline bool
+fscrypt_valid_context_format(const struct fscrypt_context *ctx, int size)
+{
+   return size >= 1 && size == fscrypt_context_size(ctx);
+}
 
 /*
- * A pointer to this structure is stored in the file system's in-core
- * representation of an inode.
+ *