| Message ID | 20210625135834.12934-16-jlayton@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v7,01/24] vfs: export new_inode_pseudo | expand |
On Fri, Jun 25, 2021 at 09:58:25AM -0400, Jeff Layton wrote: > +/* > + * We want to encrypt filenames when creating them, but the encrypted > + * versions of those names may have illegal characters in them. To mitigate > + * that, we base64 encode them, but that gives us a result that can exceed > + * NAME_MAX. > + * > + * Follow a similar scheme to fscrypt itself, and cap the filename to a > + * smaller size. If the cleartext name is longer than the value below, then > + * sha256 hash the remaining bytes. > + * > + * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255) > + */ > +#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE) Shouldn't this say "If the ciphertext name is longer than the value below", not "If the cleartext name is longer than the value below"? It would also be helpful if the above comment mentioned that when the hashing is done, the real encrypted name is stored separately. > +#if IS_ENABLED(CONFIG_FS_ENCRYPTION) > +static int encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf) > +{ > + u32 len; > + int elen; > + int ret; > + u8 *cryptbuf; > + > + WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)); > + > + /* > + * convert cleartext dentry name to ciphertext > + * if result is longer than CEPH_NOKEY_NAME_MAX, > + * sha256 the remaining bytes > + * > + * See: fscrypt_setup_filename > + */ > + if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len)) > + return -ENAMETOOLONG; > + > + /* If we have to hash the end, then we need a full-length buffer */ > + if (len > CEPH_NOHASH_NAME_MAX) > + len = NAME_MAX; > + > + cryptbuf = kmalloc(len, GFP_KERNEL); > + if (!cryptbuf) > + return -ENOMEM; > + > + ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len); > + if (ret) { > + kfree(cryptbuf); > + return ret; > + } > + > + /* hash the end if the name is long enough */ > + if (len > CEPH_NOHASH_NAME_MAX) { > + u8 hash[SHA256_DIGEST_SIZE]; > + u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX; > + > + /* hash the extra bytes and overwrite crypttext beyond that point with it */ > + sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash); > + memcpy(extra, hash, SHA256_DIGEST_SIZE); > + len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE; > + } When the ciphertext name is longer than CEPH_NOHASH_NAME_MAX, why is the filename being padded all the way to NAME_MAX? That can produce a totally different ciphertext from that produced by get_fscrypt_altname() in the next patch. The logical thing to do would be to do the encryption in the same way as get_fscrypt_altname(), and then replace any bytes beyond CEPH_NOHASH_NAME_MAX with their hash. > + > + /* base64 encode the encrypted name */ > + elen = fscrypt_base64_encode(cryptbuf, len, buf); > + kfree(cryptbuf); > + dout("base64-encoded ciphertext name = %.*s\n", len, buf); > + return elen; The argument to dout() should be elen, not len. - Eric
On Sun, 2021-07-11 at 17:53 -0500, Eric Biggers wrote: > On Fri, Jun 25, 2021 at 09:58:25AM -0400, Jeff Layton wrote: > > +/* > > + * We want to encrypt filenames when creating them, but the encrypted > > + * versions of those names may have illegal characters in them. To mitigate > > + * that, we base64 encode them, but that gives us a result that can exceed > > + * NAME_MAX. > > + * > > + * Follow a similar scheme to fscrypt itself, and cap the filename to a > > + * smaller size. If the cleartext name is longer than the value below, then > > + * sha256 hash the remaining bytes. > > + * > > + * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255) > > + */ > > +#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE) > > Shouldn't this say "If the ciphertext name is longer than the value below", not > "If the cleartext name is longer than the value below"? > > It would also be helpful if the above comment mentioned that when the hashing is > done, the real encrypted name is stored separately. > > > +#if IS_ENABLED(CONFIG_FS_ENCRYPTION) > > +static int encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf) > > +{ > > + u32 len; > > + int elen; > > + int ret; > > + u8 *cryptbuf; > > + > > + WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)); > > + > > + /* > > + * convert cleartext dentry name to ciphertext > > + * if result is longer than CEPH_NOKEY_NAME_MAX, > > + * sha256 the remaining bytes > > + * > > + * See: fscrypt_setup_filename > > + */ > > + if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len)) > > + return -ENAMETOOLONG; > > + > > + /* If we have to hash the end, then we need a full-length buffer */ > > + if (len > CEPH_NOHASH_NAME_MAX) > > + len = NAME_MAX; > > + > > + cryptbuf = kmalloc(len, GFP_KERNEL); > > + if (!cryptbuf) > > + return -ENOMEM; > > + > > + ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len); > > + if (ret) { > > + kfree(cryptbuf); > > + return ret; > > + } > > + > > + /* hash the end if the name is long enough */ > > + if (len > CEPH_NOHASH_NAME_MAX) { > > + u8 hash[SHA256_DIGEST_SIZE]; > > + u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX; > > + > > + /* hash the extra bytes and overwrite crypttext beyond that point with it */ > > + sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash); > > + memcpy(extra, hash, SHA256_DIGEST_SIZE); > > + len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE; > > + } > > When the ciphertext name is longer than CEPH_NOHASH_NAME_MAX, why is the > filename being padded all the way to NAME_MAX? That can produce a totally > different ciphertext from that produced by get_fscrypt_altname() in the next > patch. > Oh, I misunderstood the meaning of the last parameter to fscrypt_fname_encrypt. I had thought that was the length of the target buffer, but it's not -- it's the length of the resulting filename (which we need to precompute). I'll fix that up. > The logical thing to do would be to do the encryption in the same way as > get_fscrypt_altname(), and then replace any bytes beyond CEPH_NOHASH_NAME_MAX > with their hash. > That might make more sense. > > + > > + /* base64 encode the encrypted name */ > > + elen = fscrypt_base64_encode(cryptbuf, len, buf); > > + kfree(cryptbuf); > > + dout("base64-encoded ciphertext name = %.*s\n", len, buf); > > + return elen; > > The argument to dout() should be elen, not len. > Will fix, thanks. -- Jeff Layton <jlayton@kernel.org>
diff --git a/fs/ceph/crypto.h b/fs/ceph/crypto.h index bdf1ba47db16..60cc92f25fed 100644 --- a/fs/ceph/crypto.h +++ b/fs/ceph/crypto.h @@ -6,6 +6,7 @@ #ifndef _CEPH_CRYPTO_H #define _CEPH_CRYPTO_H +#include <crypto/sha2.h> #include <linux/fscrypt.h> #define CEPH_XATTR_NAME_ENCRYPTION_CONTEXT "encryption.ctx" @@ -27,6 +28,20 @@ static inline u32 ceph_fscrypt_auth_size(u32 ctxsize) return offsetof(struct ceph_fscrypt_auth, cfa_blob) + ctxsize; } +/* + * We want to encrypt filenames when creating them, but the encrypted + * versions of those names may have illegal characters in them. To mitigate + * that, we base64 encode them, but that gives us a result that can exceed + * NAME_MAX. + * + * Follow a similar scheme to fscrypt itself, and cap the filename to a + * smaller size. If the cleartext name is longer than the value below, then + * sha256 hash the remaining bytes. + * + * 189 bytes => 252 bytes base64-encoded, which is <= NAME_MAX (255) + */ +#define CEPH_NOHASH_NAME_MAX (189 - SHA256_DIGEST_SIZE) + void ceph_fscrypt_set_ops(struct super_block *sb); void ceph_fscrypt_free_dummy_policy(struct ceph_fs_client *fsc); diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 5b7872a61ad7..2692468d2dc4 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -13,6 +13,7 @@ #include <linux/ktime.h> #include "super.h" +#include "crypto.h" #include "mds_client.h" #include <linux/ceph/ceph_features.h> @@ -2383,18 +2384,85 @@ static inline u64 __get_oldest_tid(struct ceph_mds_client *mdsc) return mdsc->oldest_tid; } -/* - * Build a dentry's path. Allocate on heap; caller must kfree. Based - * on build_path_from_dentry in fs/cifs/dir.c. +#if IS_ENABLED(CONFIG_FS_ENCRYPTION) +static int encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf) +{ + u32 len; + int elen; + int ret; + u8 *cryptbuf; + + WARN_ON_ONCE(!fscrypt_has_encryption_key(parent)); + + /* + * convert cleartext dentry name to ciphertext + * if result is longer than CEPH_NOKEY_NAME_MAX, + * sha256 the remaining bytes + * + * See: fscrypt_setup_filename + */ + if (!fscrypt_fname_encrypted_size(parent, dentry->d_name.len, NAME_MAX, &len)) + return -ENAMETOOLONG; + + /* If we have to hash the end, then we need a full-length buffer */ + if (len > CEPH_NOHASH_NAME_MAX) + len = NAME_MAX; + + cryptbuf = kmalloc(len, GFP_KERNEL); + if (!cryptbuf) + return -ENOMEM; + + ret = fscrypt_fname_encrypt(parent, &dentry->d_name, cryptbuf, len); + if (ret) { + kfree(cryptbuf); + return ret; + } + + /* hash the end if the name is long enough */ + if (len > CEPH_NOHASH_NAME_MAX) { + u8 hash[SHA256_DIGEST_SIZE]; + u8 *extra = cryptbuf + CEPH_NOHASH_NAME_MAX; + + /* hash the extra bytes and overwrite crypttext beyond that point with it */ + sha256(extra, len - CEPH_NOHASH_NAME_MAX, hash); + memcpy(extra, hash, SHA256_DIGEST_SIZE); + len = CEPH_NOHASH_NAME_MAX + SHA256_DIGEST_SIZE; + } + + /* base64 encode the encrypted name */ + elen = fscrypt_base64_encode(cryptbuf, len, buf); + kfree(cryptbuf); + dout("base64-encoded ciphertext name = %.*s\n", len, buf); + return elen; +} +#else +static int encode_encrypted_fname(const struct inode *parent, struct dentry *dentry, char *buf) +{ + return -EOPNOTSUPP; +} +#endif + +/** + * ceph_mdsc_build_path - build a path string to a given dentry + * @dentry: dentry to which path should be built + * @plen: returned length of string + * @pbase: returned base inode number + * @for_wire: is this path going to be sent to the MDS? + * + * Build a string that represents the path to the dentry. This is mostly called + * for two different purposes: * - * If @stop_on_nosnap, generate path relative to the first non-snapped - * inode. + * 1) we need to build a path string to send to the MDS (for_wire == true) + * 2) we need a path string for local presentation (e.g. debugfs) (for_wire == false) + * + * The path is built in reverse, starting with the dentry. Walk back up toward + * the root, building the path until the first non-snapped inode is reached (for_wire) + * or the root inode is reached (!for_wire). * * Encode hidden .snap dirs as a double /, i.e. * foo/.snap/bar -> foo//bar */ -char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, - int stop_on_nosnap) +char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, int for_wire) { struct dentry *cur; struct inode *inode; @@ -2416,30 +2484,65 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, seq = read_seqbegin(&rename_lock); cur = dget(dentry); for (;;) { - struct dentry *temp; + struct dentry *parent; spin_lock(&cur->d_lock); inode = d_inode(cur); if (inode && ceph_snap(inode) == CEPH_SNAPDIR) { dout("build_path path+%d: %p SNAPDIR\n", pos, cur); - } else if (stop_on_nosnap && inode && dentry != cur && - ceph_snap(inode) == CEPH_NOSNAP) { + spin_unlock(&cur->d_lock); + parent = dget_parent(cur); + } else if (for_wire && inode && dentry != cur && ceph_snap(inode) == CEPH_NOSNAP) { spin_unlock(&cur->d_lock); pos++; /* get rid of any prepended '/' */ break; - } else { + } else if (!for_wire || !IS_ENCRYPTED(d_inode(cur->d_parent))) { pos -= cur->d_name.len; if (pos < 0) { spin_unlock(&cur->d_lock); break; } memcpy(path + pos, cur->d_name.name, cur->d_name.len); + spin_unlock(&cur->d_lock); + parent = dget_parent(cur); + } else { + int len, ret; + char buf[FSCRYPT_BASE64_CHARS(NAME_MAX)]; + + /* + * Proactively copy name into buf, in case we need to present + * it as-is. + */ + memcpy(buf, cur->d_name.name, cur->d_name.len); + len = cur->d_name.len; + spin_unlock(&cur->d_lock); + parent = dget_parent(cur); + + ret = __fscrypt_prepare_readdir(d_inode(parent)); + if (ret < 0) { + dput(parent); + dput(cur); + return ERR_PTR(ret); + } + + if (fscrypt_has_encryption_key(d_inode(parent))) { + len = encode_encrypted_fname(d_inode(parent), cur, buf); + if (len < 0) { + dput(parent); + dput(cur); + return ERR_PTR(len); + } + } + pos -= len; + if (pos < 0) { + dput(parent); + break; + } + memcpy(path + pos, buf, len); } - temp = cur; - spin_unlock(&temp->d_lock); - cur = dget_parent(temp); - dput(temp); + dput(cur); + cur = parent; /* Are we at the root? */ if (IS_ROOT(cur)) @@ -2463,8 +2566,7 @@ char *ceph_mdsc_build_path(struct dentry *dentry, int *plen, u64 *pbase, * A rename didn't occur, but somehow we didn't end up where * we thought we would. Throw a warning and try again. */ - pr_warn("build_path did not end path lookup where " - "expected, pos is %d\n", pos); + pr_warn("build_path did not end path lookup where expected (pos = %d)\n", pos); goto retry; } @@ -2484,7 +2586,7 @@ static int build_dentry_path(struct dentry *dentry, struct inode *dir, rcu_read_lock(); if (!dir) dir = d_inode_rcu(dentry->d_parent); - if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP) { + if (dir && parent_locked && ceph_snap(dir) == CEPH_NOSNAP && !IS_ENCRYPTED(dir)) { *pino = ceph_ino(dir); rcu_read_unlock(); *ppath = dentry->d_name.name;
Allow ceph_mdsc_build_path to encrypt and base64 encode the filename when the parent is encrypted and we're sending the path to the MDS. In most cases, we just encrypt the filenames and base64 encode them, but when the name is longer than CEPH_NOHASH_NAME_MAX, we use a similar scheme to fscrypt proper, and hash the remaning bits with sha256. When doing this, we then send along the full crypttext of the name in the new alternate_name field of the MClientRequest. The MDS can then send that along in readdir responses and traces. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/crypto.h | 15 +++++ fs/ceph/mds_client.c | 138 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 135 insertions(+), 18 deletions(-)