fscrypt context consistency check fixes
Subsystems (5)
cover fscrypt context consistency check fixes
From: Eric Biggers <[email protected]>
This patchset fixes fscrypt_has_permitted_context() to correctly handle cases where one or both encryption keys are missing, then updates the ->lookup() methods for ext4, f2fs, and ubifs to be consistent in how they handle the encryption context consistency check.
Eric Biggers (4):
fscrypt: fix context consistency check when key(s) unavailable
ext4: remove "nokey" check from ext4_lookup()
f2fs: sync f2fs_lookup() with ext4_lookup()
ubifs: check for consistent encryption contexts in ubifs_lookup()
fs/crypto/policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------ fs/ext4/namei.c | 9 +----- fs/f2fs/namei.c | 7 +++-- fs/ubifs/dir.c | 11 +++++++ 4 files changed, 84 insertions(+), 30 deletions(-)
Signature · 1 line
2.12.2.715.g7642488e1d-goog
PATCH 1/4 fscrypt: fix context consistency check when key(s) unavailable
From: Eric Biggers <[email protected]>
To mitigate some types of offline attacks, filesystem encryption is designed to enforce that all files in an encrypted directory tree use the same encryption policy (i.e. the same encryption context excluding the nonce). However, the fscrypt_has_permitted_context() function which enforces this relies on comparing struct fscrypt_info's, which are only available when we have the encryption keys. This can cause two incorrect behaviors:
1. If we have the parent directory's key but not the child's key, or vice versa, then fscrypt_has_permitted_context() returned false, causing applications to see EPERM or ENOKEY. This is incorrect if the encryption contexts are in fact consistent. Although we'd normally have either both keys or neither key in that case since the master_key_descriptors would be the same, this is not guaranteed because keys can be added or removed from keyrings at any time.
2. If we have neither the parent's key nor the child's key, then fscrypt_has_permitted_context() returned true, causing applications to see no error (or else an error for some other reason). This is incorrect if the encryption contexts are in fact inconsistent, since in that case we should deny access.
To fix this, retrieve and compare the fscrypt_contexts if we are unable to set up both fscrypt_infos.
While this slightly hurts performance when accessing an encrypted directory tree without the key, this isn't a case we really need to be optimizing for; access *with* the key is much more important. Furthermore, the performance hit is barely noticeable given that we are already retrieving the fscrypt_context and doing two keyring searches in fscrypt_get_encryption_info(). If we ever actually wanted to optimize this case we might start by caching the fscrypt_contexts.
--- fs/crypto/policy.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 68 insertions(+), 19 deletions(-)
fs/crypto/policy.c +68 -19
@@ -143,27 +143,61 @@ int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg) }EXPORT_SYMBOL (fscrypt_ioctl_get_policy); +/** +* fscrypt_has_permitted_context() - is a file's encryption policy permitted +* within its directory? +* +* @parent: inode for parent directory +* @child: inode for file being looked up, opened, or linked into @parent +* +* Filesystems must call this before permitting access to an inode in a +* situation where the parent directory is encrypted (either before allowing +* ->lookup() to succeed, or for a regular file before allowing it to be opened) +* and before any operation that involves linking an inode into an encrypted +* directory, including link, rename, and cross rename. It enforces the +* constraint that within a given encrypted directory tree, all files use the +* same encryption policy. The pre-access check is needed to detect potentially +* malicious offline violations of this constraint, while the link and rename +* checks are needed to prevent online violations of this constraint. +* +* Return: 1 if permitted, 0 if forbidden. If forbidden, the caller must fail +* the filesystem operation with EPERM. +*/ int fscrypt_has_permitted_context (struct inode *parent,struct inode *child) { -struct fscrypt_info *parent_ci, *child_ci; +const struct fscrypt_operations *cops = parent->i_sb->s_cop; +const struct fscrypt_info *parent_ci, *child_ci; +struct fscrypt_context parent_ctx, child_ctx;int res; -if ((parent ==NULL ) || (child ==NULL )) { -printk (KERN_ERR "parent %p child %p\n" , parent, child); -BUG_ON (1 ); - } -/* No restrictions on file types which are never encrypted */ if (!S_ISREG (child->i_mode) && !S_ISDIR (child->i_mode) && !S_ISLNK (child->i_mode))return 1 ; -/* no restrictions if the parent directory is not encrypted */ -if (!parent->i_sb->s_cop->is_encrypted (parent)) +/* No restrictions if the parent directory is unencrypted */ +if (!cops->is_encrypted (parent))return 1 ; -/* if the child directory is not encrypted, this is always a problem */ -if (!parent->i_sb->s_cop->is_encrypted (child)) + +/* Encrypted directories must not contain unencrypted files */ +if (!cops->is_encrypted (child))return 0 ; + +/* +* Both parent and child are encrypted, so verify they use the same +* encryption policy. Compare the fscrypt_info structs if the keys are +* available, otherwise retrieve and compare the fscrypt_contexts. +* +* Note that the fscrypt_context retrieval will be required frequently +* when accessing an encrypted directory tree without the key. +* Performance-wise this is not a big deal because we already don't +* really optimize for file access without the key (to the extent that +* such access is even possible), given that any attempted access +* already causes a fscrypt_context retrieval and keyring search. +* +* In any case, if an unexpected error occurs, fall back to "forbidden". +*/ + res =fscrypt_get_encryption_info (parent);if (res)return 0 ;
@@ -172,17 +206,32 @@ int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)return 0 ; parent_ci = parent->i_crypt_info; child_ci = child->i_crypt_info; -if (!parent_ci && !child_ci) -return 1 ; -if (!parent_ci || !child_ci) + +if (parent_ci && child_ci) { +return memcmp (parent_ci->ci_master_key, child_ci->ci_master_key, +FS_KEY_DESCRIPTOR_SIZE ) ==0 && + (parent_ci->ci_data_mode == child_ci->ci_data_mode) && + (parent_ci->ci_filename_mode == + child_ci->ci_filename_mode) && + (parent_ci->ci_flags == child_ci->ci_flags); + } + + res = cops->get_context (parent, &parent_ctx,sizeof (parent_ctx)); +if (res !=sizeof (parent_ctx)) +return 0 ; + + res = cops->get_context (child, &child_ctx,sizeof (child_ctx)); +if (res !=sizeof (child_ctx))return 0 ; -return (memcmp (parent_ci->ci_master_key, - child_ci->ci_master_key, -FS_KEY_DESCRIPTOR_SIZE ) ==0 && - (parent_ci->ci_data_mode == child_ci->ci_data_mode) && - (parent_ci->ci_filename_mode == child_ci->ci_filename_mode) && - (parent_ci->ci_flags == child_ci->ci_flags)); +return memcmp (parent_ctx.master_key_descriptor, + child_ctx.master_key_descriptor, +FS_KEY_DESCRIPTOR_SIZE ) ==0 && + (parent_ctx.contents_encryption_mode == + child_ctx.contents_encryption_mode) && + (parent_ctx.filenames_encryption_mode == + child_ctx.filenames_encryption_mode) && + (parent_ctx.flags == child_ctx.flags); }EXPORT_SYMBOL (fscrypt_has_permitted_context);
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [1/4] fscrypt: fix context consistency check when key(s) unavailable
Quoted 35 lines · Eric Biggers
From: Eric Biggers <[email protected]>
To mitigate some types of offline attacks, filesystem encryption is designed to enforce that all files in an encrypted directory tree use the same encryption policy (i.e. the same encryption context excluding the nonce). However, the
fscrypt_has_permitted_context()function which enforces this relies on comparing struct fscrypt_info's, which are only available when we have the encryption keys. This can cause two incorrect behaviors:1. If we have the parent directory's key but not the child's key, or vice versa, then
fscrypt_has_permitted_context()returned false, causing applications to see EPERM or ENOKEY. This is incorrect if the encryption contexts are in fact consistent. Although we'd normally have either both keys or neither key in that case since themaster_key_descriptorswould be the same, this is not guaranteed because keys can be added or removed from keyrings at any time.2. If we have neither the parent's key nor the child's key, then
fscrypt_has_permitted_context()returned true, causing applications to see no error (or else an error for some other reason). This is incorrect if the encryption contexts are in fact inconsistent, since in that case we should deny access.To fix this, retrieve and compare the
fscrypt_contextsif we are unable to set up bothfscrypt_infos.While this slightly hurts performance when accessing an encrypted directory tree without the key, this isn't a case we really need to be optimizing for; access *with* the key is much more important. Furthermore, the performance hit is barely noticeable given that we are already retrieving the
fscrypt_contextand doing two keyring searches infscrypt_get_encryption_info(). If we ever actually wanted to optimize this case we might start by caching thefscrypt_contexts.
Quoted 1 line
Signed-off-by Eric Biggers <[email protected]>
Thanks, applied.
- Ted
Re: [1/4] fscrypt: fix context consistency check when key(s) unavailable
Quoted 36 lines · Theodore Ts'o
Quoted 35 lines · Eric Biggers
From: Eric Biggers <[email protected]>
To mitigate some types of offline attacks, filesystem encryption is designed to enforce that all files in an encrypted directory tree use the same encryption policy (i.e. the same encryption context excluding the nonce). However, the
fscrypt_has_permitted_context()function which enforces this relies on comparing struct fscrypt_info's, which are only available when we have the encryption keys. This can cause two incorrect behaviors:1. If we have the parent directory's key but not the child's key, or vice versa, then
fscrypt_has_permitted_context()returned false, causing applications to see EPERM or ENOKEY. This is incorrect if the encryption contexts are in fact consistent. Although we'd normally have either both keys or neither key in that case since themaster_key_descriptorswould be the same, this is not guaranteed because keys can be added or removed from keyrings at any time.2. If we have neither the parent's key nor the child's key, then
fscrypt_has_permitted_context()returned true, causing applications to see no error (or else an error for some other reason). This is incorrect if the encryption contexts are in fact inconsistent, since in that case we should deny access.To fix this, retrieve and compare the
fscrypt_contextsif we are unable to set up bothfscrypt_infos.While this slightly hurts performance when accessing an encrypted directory tree without the key, this isn't a case we really need to be optimizing for; access *with* the key is much more important. Furthermore, the performance hit is barely noticeable given that we are already retrieving the
fscrypt_contextand doing two keyring searches infscrypt_get_encryption_info(). If we ever actually wanted to optimize this case we might start by caching thefscrypt_contexts.
Quoted 5 lines
Quoted 1 line
Signed-off-by Eric Biggers <[email protected]>Thanks, applied.
- Ted
Ted, can you add Cc stable to this commit? (Just this one; the others in the series aren't as important.) I think this fix is more important than I originally thought, because it's actually pretty easy to end up in a situation where a directory has its key but a file in it does not, due to the file's inode having been evicted from the inode cache but not the directory's inode. This makes the file undeletable.
- Eric
Theodore Ts'o
maintainerRe: [1/4] fscrypt: fix context consistency check when key(s) unavailable
Quoted 6 lines · Eric Biggers
Ted, can you add Cc stable to this commit? (Just this one; the others in the series aren't as important.) I think this fix is more important than I originally thought, because it's actually pretty easy to end up in a situation where a directory has its key but a file in it does not, due to the file's inode having been evicted from the inode cache but not the directory's inode. This makes the file undeletable.
Done. This meant I had to do a rewind on the fscrypt tree, so if someone has built something on top of fscrypt since Sunday you may need to take special precautions. This was just a change to the commit description so git rebase should take of the problem relatively easily.
- Ted
PATCH 2/4 ext4: remove "nokey" check from ext4_lookup()
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
Now that fscrypt_has_permitted_context() correctly handles the case where we have the key for the parent directory but not the child, we don't need to try to work around this in ext4_lookup().
--- fs/ext4/namei.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
fs/ext4/namei.c +1 -8
@@ -1616,16 +1616,9 @@ static struct dentry *ext4_lookup(struct inode *dir, struct dentry *dentry, unsiif (!IS_ERR (inode) &&ext4_encrypted_inode (dir) && (S_ISDIR (inode->i_mode) ||S_ISLNK (inode->i_mode)) && !fscrypt_has_permitted_context (dir, inode)) { -int nokey =ext4_encrypted_inode (inode) && - !fscrypt_has_encryption_key (inode); -if (nokey) { -iput (inode); -return ERR_PTR (-ENOKEY ); - }ext4_warning (inode->i_sb,"Inconsistent encryption contexts: %lu/%lu" , - (unsigned long ) dir->i_ino, - (unsigned long ) inode->i_ino); + dir->i_ino, inode->i_ino);iput (inode);return ERR_PTR (-EPERM ); }
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [2/4] ext4: remove "nokey" check from ext4_lookup()
Quoted 7 lines · Eric Biggers
From: Eric Biggers <[email protected]>
Now that
fscrypt_has_permitted_context()correctly handles the case where we have the key for the parent directory but not the child, we don't need to try to work around this inext4_lookup().Signed-off-by Eric Biggers <[email protected]>
Thanks, applied.
- Ted
PATCH 3/4 f2fs: sync f2fs_lookup() with ext4_lookup()
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
As for ext4, now that fscrypt_has_permitted_context() correctly handles the case where we have the key for the parent directory but not the child, f2fs_lookup() no longer has to work around it. Also add the same warning message that ext4 uses.
--- fs/f2fs/namei.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
fs/f2fs/namei.c +4 -3
@@ -324,9 +324,10 @@ static struct dentry *f2fs_lookup(struct inode *dir, struct dentry *dentry,if (f2fs_encrypted_inode (dir) && (S_ISDIR (inode->i_mode) ||S_ISLNK (inode->i_mode)) && !fscrypt_has_permitted_context (dir, inode)) { -bool nokey =f2fs_encrypted_inode (inode) && - !fscrypt_has_encryption_key (inode); - err = nokey ? -ENOKEY : -EPERM ; +f2fs_msg (inode->i_sb,KERN_WARNING , +"Inconsistent encryption contexts: %lu/%lu" , + dir->i_ino, inode->i_ino); + err = -EPERM ; goto err_out; }return d_splice_alias (inode, dentry);
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [3/4] f2fs: sync f2fs_lookup() with ext4_lookup()
Quoted 8 lines · Eric Biggers
From: Eric Biggers <[email protected]>
As for ext4, now that
fscrypt_has_permitted_context()correctly handles the case where we have the key for the parent directory but not the child,f2fs_lookup()no longer has to work around it. Also add the same warning message that ext4 uses.Signed-off-by Eric Biggers <[email protected]>
Thanks, applied.
- Ted
PATCH 4/4 ubifs: check for consistent encryption contexts in ubifs_lookup()
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
As ext4 and f2fs do, ubifs should check for consistent encryption contexts during ->lookup() in an encrypted directory. This protects certain users of filesystem encryption against certain types of offline attacks.
--- fs/ubifs/dir.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
fs/ubifs/dir.c +11 -0
@@ -285,6 +285,15 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry, goto out_dent; } +if (ubifs_crypt_is_encrypted (dir) && + (S_ISDIR (inode->i_mode) ||S_ISLNK (inode->i_mode)) && + !fscrypt_has_permitted_context (dir, inode)) { +ubifs_warn (c,"Inconsistent encryption contexts: %lu/%lu" , + dir->i_ino, inode->i_ino); + err = -EPERM ; + goto out_inode; + } + done:kfree (dent);fscrypt_free_filename (&nm);
@@ -295,6 +304,8 @@ static struct dentry *ubifs_lookup(struct inode *dir, struct dentry *dentry,d_add (dentry, inode);return NULL ; +out_inode: +iput (inode); out_dent:kfree (dent); out_fname:
Signature · 1 line
2.12.2.715.g7642488e1d-goog
Re: [4/4] ubifs: check for consistent encryption contexts in ubifs_lookup()
Quoted 8 lines · Eric Biggers
From: Eric Biggers <[email protected]>
As ext4 and f2fs do, ubifs should check for consistent encryption contexts during ->lookup() in an encrypted directory. This protects certain users of filesystem encryption against certain types of offline attacks.
Signed-off-by Eric Biggers <[email protected]>
Thanks, applied.
- Ted