| Message ID | 20170419001005.GA143911@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
Hi Eric, On 04/18, Eric Biggers wrote: > On Tue, Apr 18, 2017 at 04:01:36PM -0700, Eric Biggers wrote: > > > > Strangely, f2fs and ubifs do not use the bytes from the filename at all when > > trying to find a specific directory entry in this case. So this patch doesn't > > really affect them. This seems unreliable; perhaps we should introduce a > > function like "fscrypt_name_matches()" which all the filesystems could call? > > Can any of the f2fs and ubifs developers explain why they don't look at any > > bytes from the filename? > > The fscrypt_setup_filename sets fname->hash in the bigname case, but doesn't give fname->disk_name. If it's not such the bigname case, we check its name since fname->hash is zero. > Just to give some ideas, here's an untested patch which does this and also > updates F2FS to start checking the filename. UBIFS seemed more difficult so I > didn't touch it yet. > > Of course, this would need to be split into a few different patches if we > actually wanted to go with it. > > --- ... > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 8d5c62b07b28..07b80d78a9f6 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -111,8 +111,6 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, > struct f2fs_dir_entry *de; > unsigned long bit_pos = 0; > int max_len = 0; > - struct fscrypt_str de_name = FSTR_INIT(NULL, 0); > - struct fscrypt_str *name = &fname->disk_name; > > if (max_slots) > *max_slots = 0; > @@ -130,17 +128,10 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, > continue; > } > > - /* encrypted case */ > - de_name.name = d->filename[bit_pos]; > - de_name.len = le16_to_cpu(de->name_len); > - > - /* show encrypted name */ > - if (fname->hash) { > - if (de->hash_code == cpu_to_le32(fname->hash)) > - goto found; > - } else if (de_name.len == name->len && > - de->hash_code == namehash && > - !memcmp(de_name.name, name->name, name->len)) > + if ((fname->hash == 0 || > + fname->hash == le32_to_cpu(de->hash_code)) && > + fscrypt_name_matches(fname, d->filename[bit_pos], > + le16_to_cpu(de->name_len))) BTW, this slips checking namehash? Thanks, > goto found; > > if (max_slots && max_len > *max_slots) > diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h > index 3511ca798804..4034976bea93 100644 > --- a/include/linux/fscrypt_notsupp.h > +++ b/include/linux/fscrypt_notsupp.h > @@ -147,6 +147,24 @@ static inline int fscrypt_fname_usr_to_disk(struct inode *inode, > return -EOPNOTSUPP; > } > > +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, > + const struct fscrypt_str *disk_name, > + const struct fscrypt_str *crypto_buf, > + const char *de_name, u32 de_name_len) > +{ > + /* Encryption support disabled; use standard comparison. */ > + if (de_name_len != disk_name->len) > + return false; > + return !memcmp(de_name, disk_name->name, disk_name->len); > +} > + > +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, > + const char *de_name, u32 de_name_len) > +{ > + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, > + &fname->crypto_buf, de_name, de_name_len); > +} > + > /* bio.c */ > static inline void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, > struct bio *bio) > diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h > index a140f47e9b27..e3c9aa7209a1 100644 > --- a/include/linux/fscrypt_supp.h > +++ b/include/linux/fscrypt_supp.h > @@ -57,6 +57,63 @@ extern int fscrypt_fname_disk_to_usr(struct inode *, u32, u32, > extern int fscrypt_fname_usr_to_disk(struct inode *, const struct qstr *, > struct fscrypt_str *); > > +/* > + * Number of bytes of ciphertext from the end of the filename which the > + * filesystem includes when encoding long encrypted filenames for presentation > + * to userspace without the key. > + */ > +#define FS_FNAME_CRYPTO_DIGEST_SIZE (2 * FS_CRYPTO_BLOCK_SIZE) > + > +/** > + * fscrypt_match_dirent() - does the directory entry match the name being looked up? > + * > + * This is like fscrypt_name_matches(), but for filesystems which don't use the > + * fscrypt_name structure. (We probably should make all filesystems do the same > + * thing...) > + */ > +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, > + const struct fscrypt_str *disk_name, > + const struct fscrypt_str *crypto_buf, > + const char *de_name, u32 de_name_len) > +{ > + if (unlikely(!disk_name->name)) { > + if (WARN_ON_ONCE(usr_fname->name[0] != '_')) > + return false; > + if (de_name_len < FS_FNAME_CRYPTO_DIGEST_SIZE) > + return false; > + return !memcmp(de_name + de_name_len - FS_FNAME_CRYPTO_DIGEST_SIZE, > + crypto_buf->name + 8, > + FS_FNAME_CRYPTO_DIGEST_SIZE); > + } > + > + if (de_name_len != disk_name->len) > + return false; > + return !memcmp(de_name, disk_name->name, disk_name->len); > +} > + > +/** > + * fscrypt_name_matches() - does the directory entry match the name being looked up? > + * @fname: the name being looked up > + * @de_name: the name from the directory entry > + * @de_name_len: the length of @de_name in bytes > + * > + * Normally @fname->disk_name will be set, and in that case we simply compare > + * that to the directory entry. The only exception is that if we don't have the > + * key for an encrypted directory and a filename in it is very long, then the > + * filename presented to userspace will only have the last > + * FS_FNAME_CRYPTO_DIGEST_SIZE bytes of ciphertext encoded in it, so we can only > + * compare that portion. Note that despite this limit, due to the use of > + * CBC-CTS encryption there should not be any collisions. > + * > + * Return: true if the name matches, otherwise false. > + */ > +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, > + const char *de_name, u32 de_name_len) > +{ > + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, > + &fname->crypto_buf, de_name, de_name_len); > +} > + > /* bio.c */ > extern void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *, struct bio *); > extern void fscrypt_pullback_bio_page(struct page **, bool); > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most > engaging tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
On Tue, Apr 18, 2017 at 06:42:09PM -0700, Jaegeuk Kim wrote: > Hi Eric, > > On 04/18, Eric Biggers wrote: > > On Tue, Apr 18, 2017 at 04:01:36PM -0700, Eric Biggers wrote: > > > > > > Strangely, f2fs and ubifs do not use the bytes from the filename at all when > > > trying to find a specific directory entry in this case. So this patch doesn't > > > really affect them. This seems unreliable; perhaps we should introduce a > > > function like "fscrypt_name_matches()" which all the filesystems could call? > > > Can any of the f2fs and ubifs developers explain why they don't look at any > > > bytes from the filename? > > > > > The fscrypt_setup_filename sets fname->hash in the bigname case, but doesn't > give fname->disk_name. If it's not such the bigname case, we check its name > since fname->hash is zero. > Yes, that's what it does now. The question is, in the "bigname" case why doesn't f2fs check the 16 bytes of ciphertext in fname->crypto_buf too? f2fs doesn't even use 'minor_hash'; it can't possibly be the case that there are never any collisions of a 32-bit hash in a directory, can it? I actually tested it, and it definitely happens if you put a lot of files in an encrypted directory on f2fs. An example with 100000 files: # seq -f "edir/abcdefghijklmnopqrstuvwxyz012345%.0f" 100000 | xargs touch # find edir -type f | xargs stat -c %i | sort | uniq | wc -l 100000 # sync # echo 3 > /proc/sys/vm/drop_caches # keyctl new_session # find edir -type f | xargs stat -c %i | sort | uniq | wc -l 99999 So when I tried accessing the encrypted directory without the key, two dentries showed the same inode, due to a hash collision. Actually, checking the last 16 bytes of ciphertext currently wouldn't even help for those filenames since it's all the same, as they share a long common prefix: # ls -1 edir | head -n 4 _++09VCAAAAgsQQf6Q5YgLgoO4f3PPSfb _++1UWDAAAAgsQQf6Q5YgLgoO4f3PPSfb _++2HAAAAAAgsQQf6Q5YgLgoO4f3PPSfb _++4UxBAAAAgsQQf6Q5YgLgoO4f3PPSfb But that's the bug, since the last two AES blocks are swapped when using ciphertext stealing. We should at least be using the second-to-last block in which case we'd see: # ls -1 edir | head -n 4 _++09VCAAAAw9VONwQEXOVv3RR,kOAKwB _++1UWDAAAAAHDi7c3QZxbiltjOo1m0,F _++2HAAAAAAAfd1Vx0oC31SmhzYpaYfwz _++4UxBAAAAwZxcWjzORdAef50FB9sKY4 (In either case there are still a few A's at the beginning since f2fs doesn't set 'minor_hash'. That's okay, but only if collisions are ruled out by other means.) > > - /* encrypted case */ > > - de_name.name = d->filename[bit_pos]; > > - de_name.len = le16_to_cpu(de->name_len); > > - > > - /* show encrypted name */ > > - if (fname->hash) { > > - if (de->hash_code == cpu_to_le32(fname->hash)) > > - goto found; > > - } else if (de_name.len == name->len && > > - de->hash_code == namehash && > > - !memcmp(de_name.name, name->name, name->len)) > > + if ((fname->hash == 0 || > > + fname->hash == le32_to_cpu(de->hash_code)) && > > + fscrypt_name_matches(fname, d->filename[bit_pos], > > + le16_to_cpu(de->name_len))) > > BTW, this slips checking namehash? > Yes that's a mistake. Actually it seems that 'namehash' is the same as 'fname->hash' when 'fname->hash' is nonzero, so the code should just be: if (de->hash_code == namehash && fscrypt_name_matches(fname, d->filename[bit_pos], le16_to_cpu(de->name_len))) goto found; - Eric
On Tue, Apr 18, 2017 at 5:10 PM, Eric Biggers <ebiggers3@gmail.com> wrote: > On Tue, Apr 18, 2017 at 04:01:36PM -0700, Eric Biggers wrote: >> >> Strangely, f2fs and ubifs do not use the bytes from the filename at all when >> trying to find a specific directory entry in this case. So this patch doesn't >> really affect them. This seems unreliable; perhaps we should introduce a >> function like "fscrypt_name_matches()" which all the filesystems could call? >> Can any of the f2fs and ubifs developers explain why they don't look at any >> bytes from the filename? >> > > Just to give some ideas, here's an untested patch which does this and also > updates F2FS to start checking the filename. UBIFS seemed more difficult so I > didn't touch it yet. Verified your better patch - modified to work on 4.9 - is working with ext4, More comment inline. > > Of course, this would need to be split into a few different patches if we > actually wanted to go with it. > > --- > > diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c > index 37b49894c762..1fc19a265924 100644 > --- a/fs/crypto/fname.c > +++ b/fs/crypto/fname.c > @@ -160,12 +160,14 @@ static const char *lookup_table = > "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; > > /** > - * digest_encode() - > + * base64_encode() - I noticed there are another implementation of base64 in the kernel, ceph_armor (although it uses the regular '/' instead of ',' for the 64th character). Looking at RFC 3548 (https://tools.ietf.org/html/rfc3548#page-6) "Base 64 Encoding with URL and Filename Safe Alphabet", the 63th and 64th character should be '-_' instead of '+,'. Rename base64_filename_encode to be precise. > * > - * Encodes the input digest using characters from the set [a-zA-Z0-9_+]. > - * The encoded string is roughly 4/3 times the size of the input string. > + * Encode the input data using characters from the set [A-Za-z0-9+,]. > + * > + * Return: the length of the encoded string. This will be 4/3 times the size of > + * the input string, rounded up. > */ > -static int digest_encode(const char *src, int len, char *dst) > +static int base64_encode(const char *src, int len, char *dst) > { > int i = 0, bits = 0, ac = 0; > char *cp = dst; > @@ -185,7 +187,9 @@ static int digest_encode(const char *src, int len, char *dst) > return cp - dst; > } > > -static int digest_decode(const char *src, int len, char *dst) > +#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) > + > +static int base64_decode(const char *src, int len, char *dst) > { > int i = 0, bits = 0, ac = 0; > const char *p; > @@ -274,7 +278,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, > struct fscrypt_str *oname) > { > const struct qstr qname = FSTR_TO_QSTR(iname); > - char buf[24]; > + char buf[8 + FS_FNAME_CRYPTO_DIGEST_SIZE]; Given buf is now internal to fscrypto, we should define a structure: struct fscrypto_bigname { u32 hash; u32 minor_hash; u8 digest[FS_FNAME_CRYPTO_DIGEST_SIZE]; } > > if (fscrypt_is_dot_dotdot(&qname)) { > oname->name[0] = '.'; > @@ -289,20 +293,35 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, > if (inode->i_crypt_info) > return fname_decrypt(inode, iname, oname); > > + /* Key is unavailable. Encode the name without decrypting it. */ > + > if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) { > - oname->len = digest_encode(iname->name, iname->len, > + /* Short name: base64-encode the ciphertext directly */ > + oname->len = base64_encode(iname->name, iname->len, > oname->name); > return 0; > } > + > + /* > + * Long name. We can't simply base64-encode the full ciphertext, since > + * the resulting length may exceed NAME_MAX. Instead, base64-encode a > + * filesystem-provided cookie ('hash' and 'minor_hash') followed by the > + * last two ciphertext blocks. It's assumed this is sufficient to > + * identify the directory entry on ->lookup(). It's not actually > + * guaranteed, but with CBC or CBC-CTS mode encryption, it's good enough > + * since the unused blocks will affect the used ones. > + */ > + > if (hash) { > memcpy(buf, &hash, 4); > memcpy(buf + 4, &minor_hash, 4); > } else { > memset(buf, 0, 8); > } > - memcpy(buf + 8, iname->name + iname->len - 16, 16); > + memcpy(buf + 8, iname->name + iname->len - FS_FNAME_CRYPTO_DIGEST_SIZE, > + FS_FNAME_CRYPTO_DIGEST_SIZE); > oname->name[0] = '_'; > - oname->len = 1 + digest_encode(buf, 24, oname->name + 1); > + oname->len = 1 + base64_encode(buf, sizeof(buf), oname->name + 1); > return 0; > } > EXPORT_SYMBOL(fscrypt_fname_disk_to_usr); > @@ -373,17 +392,21 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname, > * We don't have the key and we are doing a lookup; decode the > * user-supplied name > */ > - if (iname->name[0] == '_') > + if (iname->name[0] == '_') { > bigname = 1; > - if ((bigname && (iname->len != 33)) || (!bigname && (iname->len > 43))) > + if (iname->len != > + BASE64_CHARS(1 + 8 + FS_FNAME_CRYPTO_DIGEST_SIZE)) becomes 1 + sizeof(struct fscrypto_bigname) > + return -ENOENT; > + } else if (iname->len > BASE64_CHARS(FS_FNAME_CRYPTO_DIGEST_SIZE)) > return -ENOENT; > > - fname->crypto_buf.name = kmalloc(32, GFP_KERNEL); > + fname->crypto_buf.name = kmalloc(8 + FS_FNAME_CRYPTO_DIGEST_SIZE, max(32, sizeof(struct fscrypto_bigname)) to be precise, > + GFP_KERNEL); > if (fname->crypto_buf.name == NULL) > return -ENOMEM; > > - ret = digest_decode(iname->name + bigname, iname->len - bigname, > - fname->crypto_buf.name); > + ret = base64_decode(iname->name + bigname, iname->len - bigname, > + fname->crypto_buf.name); > if (ret < 0) { > ret = -ENOENT; > goto errout; > diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h > index e39696e64494..f1f15b84e02f 100644 > --- a/fs/crypto/fscrypt_private.h > +++ b/fs/crypto/fscrypt_private.h > @@ -13,8 +13,6 @@ > > #include <linux/fscrypt_supp.h> > > -#define FS_FNAME_CRYPTO_DIGEST_SIZE 32 > - > /* Encryption parameters */ > #define FS_XTS_TWEAK_SIZE 16 > #define FS_AES_128_ECB_KEY_SIZE 16 > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 07e5e1405771..d1618835de12 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -1237,37 +1237,23 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block) > } > > /* > - * NOTE! unlike strncmp, ext4_match returns 1 for success, 0 for failure. > + * Determine whether the filename being looked up matches the given dir_entry. > * > - * `len <= EXT4_NAME_LEN' is guaranteed by caller. > - * `de != NULL' is guaranteed by caller. > + * Return: true if the entry matches, otherwise false. > */ > -static inline int ext4_match(struct ext4_filename *fname, > - struct ext4_dir_entry_2 *de) > +static inline bool ext4_match(const struct ext4_filename *fname, > + const struct ext4_dir_entry_2 *de) > { > - const void *name = fname_name(fname); > - u32 len = fname_len(fname); > + const struct fscrypt_str *crypto_buf = NULL; > > if (!de->inode) > return 0; > > #ifdef CONFIG_EXT4_FS_ENCRYPTION > - if (unlikely(!name)) { > - if (fname->usr_fname->name[0] == '_') { > - int ret; > - if (de->name_len < 16) > - return 0; > - ret = memcmp(de->name + de->name_len - 16, > - fname->crypto_buf.name + 8, 16); > - return (ret == 0) ? 1 : 0; > - } > - name = fname->crypto_buf.name; > - len = fname->crypto_buf.len; > - } > + crypto_buf = &fname->crypto_buf; > #endif > - if (de->name_len != len) > - return 0; > - return (memcmp(de->name, name, len) == 0) ? 1 : 0; > + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, > + crypto_buf, de->name, de->name_len); > } > > /* > @@ -1289,12 +1275,7 @@ int ext4_search_dir(struct buffer_head *bh, char *search_buf, int buf_size, > /* this code is executed quadratically often */ > /* do minimal checking `by hand' */ > if ((char *) de + de->name_len <= dlimit) { > - res = ext4_match(fname, de); > - if (res < 0) { > - res = -1; > - goto return_result; > - } > - if (res > 0) { > + if (ext4_match(fname, de)) { > /* found a match - just to be sure, do > * a full check */ > if (ext4_check_dir_entry(dir, NULL, de, bh, > @@ -1843,11 +1824,7 @@ int ext4_find_dest_de(struct inode *dir, struct inode *inode, > res = -EFSCORRUPTED; > goto return_result; > } > - /* Provide crypto context and crypto buffer to ext4 match */ > - res = ext4_match(fname, de); > - if (res < 0) > - goto return_result; > - if (res > 0) { > + if (ext4_match(fname, de)) { > res = -EEXIST; > goto return_result; > } > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 8d5c62b07b28..07b80d78a9f6 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -111,8 +111,6 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, > struct f2fs_dir_entry *de; > unsigned long bit_pos = 0; > int max_len = 0; > - struct fscrypt_str de_name = FSTR_INIT(NULL, 0); > - struct fscrypt_str *name = &fname->disk_name; > > if (max_slots) > *max_slots = 0; > @@ -130,17 +128,10 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, > continue; > } > > - /* encrypted case */ > - de_name.name = d->filename[bit_pos]; > - de_name.len = le16_to_cpu(de->name_len); > - > - /* show encrypted name */ > - if (fname->hash) { > - if (de->hash_code == cpu_to_le32(fname->hash)) > - goto found; > - } else if (de_name.len == name->len && > - de->hash_code == namehash && > - !memcmp(de_name.name, name->name, name->len)) > + if ((fname->hash == 0 || > + fname->hash == le32_to_cpu(de->hash_code)) && > + fscrypt_name_matches(fname, d->filename[bit_pos], > + le16_to_cpu(de->name_len))) > goto found; > > if (max_slots && max_len > *max_slots) > diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h > index 3511ca798804..4034976bea93 100644 > --- a/include/linux/fscrypt_notsupp.h > +++ b/include/linux/fscrypt_notsupp.h > @@ -147,6 +147,24 @@ static inline int fscrypt_fname_usr_to_disk(struct inode *inode, > return -EOPNOTSUPP; > } > > +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, > + const struct fscrypt_str *disk_name, > + const struct fscrypt_str *crypto_buf, > + const char *de_name, u32 de_name_len) > +{ > + /* Encryption support disabled; use standard comparison. */ > + if (de_name_len != disk_name->len) > + return false; > + return !memcmp(de_name, disk_name->name, disk_name->len); > +} > + > +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, > + const char *de_name, u32 de_name_len) > +{ > + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, > + &fname->crypto_buf, de_name, de_name_len); > +} > + > /* bio.c */ > static inline void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, > struct bio *bio) > diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h > index a140f47e9b27..e3c9aa7209a1 100644 > --- a/include/linux/fscrypt_supp.h > +++ b/include/linux/fscrypt_supp.h > @@ -57,6 +57,63 @@ extern int fscrypt_fname_disk_to_usr(struct inode *, u32, u32, > extern int fscrypt_fname_usr_to_disk(struct inode *, const struct qstr *, > struct fscrypt_str *); > > +/* > + * Number of bytes of ciphertext from the end of the filename which the > + * filesystem includes when encoding long encrypted filenames for presentation > + * to userspace without the key. > + */ > +#define FS_FNAME_CRYPTO_DIGEST_SIZE (2 * FS_CRYPTO_BLOCK_SIZE) > + > +/** > + * fscrypt_match_dirent() - does the directory entry match the name being looked up? > + * > + * This is like fscrypt_name_matches(), but for filesystems which don't use the > + * fscrypt_name structure. (We probably should make all filesystems do the same > + * thing...) > + */ > +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, > + const struct fscrypt_str *disk_name, > + const struct fscrypt_str *crypto_buf, > + const char *de_name, u32 de_name_len) > +{ > + if (unlikely(!disk_name->name)) { > + if (WARN_ON_ONCE(usr_fname->name[0] != '_')) > + return false; > + if (de_name_len < FS_FNAME_CRYPTO_DIGEST_SIZE) > + return false; > + return !memcmp(de_name + de_name_len - FS_FNAME_CRYPTO_DIGEST_SIZE, > + crypto_buf->name + 8,/buf[ > + FS_FNAME_CRYPTO_DIGEST_SIZE); > + } > + > + if (de_name_len != disk_name->len) > + return false; > + return !memcmp(de_name, disk_name->name, disk_name->len); > +} > + > +/** > + * fscrypt_name_matches() - does the directory entry match the name being looked up? > + * @fname: the name being looked up > + * @de_name: the name from the directory entry > + * @de_name_len: the length of @de_name in bytes > + * > + * Normally @fname->disk_name will be set, and in that case we simply compare > + * that to the directory entry. The only exception is that if we don't have the > + * key for an encrypted directory and a filename in it is very long, then the > + * filename presented to userspace will only have the last > + * FS_FNAME_CRYPTO_DIGEST_SIZE bytes of ciphertext encoded in it, so we can only > + * compare that portion. Note that despite this limit, due to the use of > + * CBC-CTS encryption there should not be any collisions. > + * > + * Return: true if the name matches, otherwise false. > + */ > +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, > + const char *de_name, u32 de_name_len) > +{ > + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, > + &fname->crypto_buf, de_name, de_name_len); > +} > + > /* bio.c */ > extern void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *, struct bio *); > extern void fscrypt_pullback_bio_page(struct page **, bool);
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 37b49894c762..1fc19a265924 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -160,12 +160,14 @@ static const char *lookup_table = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+,"; /** - * digest_encode() - + * base64_encode() - * - * Encodes the input digest using characters from the set [a-zA-Z0-9_+]. - * The encoded string is roughly 4/3 times the size of the input string. + * Encode the input data using characters from the set [A-Za-z0-9+,]. + * + * Return: the length of the encoded string. This will be 4/3 times the size of + * the input string, rounded up. */ -static int digest_encode(const char *src, int len, char *dst) +static int base64_encode(const char *src, int len, char *dst) { int i = 0, bits = 0, ac = 0; char *cp = dst; @@ -185,7 +187,9 @@ static int digest_encode(const char *src, int len, char *dst) return cp - dst; } -static int digest_decode(const char *src, int len, char *dst) +#define BASE64_CHARS(nbytes) DIV_ROUND_UP((nbytes) * 4, 3) + +static int base64_decode(const char *src, int len, char *dst) { int i = 0, bits = 0, ac = 0; const char *p; @@ -274,7 +278,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, struct fscrypt_str *oname) { const struct qstr qname = FSTR_TO_QSTR(iname); - char buf[24]; + char buf[8 + FS_FNAME_CRYPTO_DIGEST_SIZE]; if (fscrypt_is_dot_dotdot(&qname)) { oname->name[0] = '.'; @@ -289,20 +293,35 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, if (inode->i_crypt_info) return fname_decrypt(inode, iname, oname); + /* Key is unavailable. Encode the name without decrypting it. */ + if (iname->len <= FS_FNAME_CRYPTO_DIGEST_SIZE) { - oname->len = digest_encode(iname->name, iname->len, + /* Short name: base64-encode the ciphertext directly */ + oname->len = base64_encode(iname->name, iname->len, oname->name); return 0; } + + /* + * Long name. We can't simply base64-encode the full ciphertext, since + * the resulting length may exceed NAME_MAX. Instead, base64-encode a + * filesystem-provided cookie ('hash' and 'minor_hash') followed by the + * last two ciphertext blocks. It's assumed this is sufficient to + * identify the directory entry on ->lookup(). It's not actually + * guaranteed, but with CBC or CBC-CTS mode encryption, it's good enough + * since the unused blocks will affect the used ones. + */ + if (hash) { memcpy(buf, &hash, 4); memcpy(buf + 4, &minor_hash, 4); } else { memset(buf, 0, 8); } - memcpy(buf + 8, iname->name + iname->len - 16, 16); + memcpy(buf + 8, iname->name + iname->len - FS_FNAME_CRYPTO_DIGEST_SIZE, + FS_FNAME_CRYPTO_DIGEST_SIZE); oname->name[0] = '_'; - oname->len = 1 + digest_encode(buf, 24, oname->name + 1); + oname->len = 1 + base64_encode(buf, sizeof(buf), oname->name + 1); return 0; } EXPORT_SYMBOL(fscrypt_fname_disk_to_usr); @@ -373,17 +392,21 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname, * We don't have the key and we are doing a lookup; decode the * user-supplied name */ - if (iname->name[0] == '_') + if (iname->name[0] == '_') { bigname = 1; - if ((bigname && (iname->len != 33)) || (!bigname && (iname->len > 43))) + if (iname->len != + BASE64_CHARS(1 + 8 + FS_FNAME_CRYPTO_DIGEST_SIZE)) + return -ENOENT; + } else if (iname->len > BASE64_CHARS(FS_FNAME_CRYPTO_DIGEST_SIZE)) return -ENOENT; - fname->crypto_buf.name = kmalloc(32, GFP_KERNEL); + fname->crypto_buf.name = kmalloc(8 + FS_FNAME_CRYPTO_DIGEST_SIZE, + GFP_KERNEL); if (fname->crypto_buf.name == NULL) return -ENOMEM; - ret = digest_decode(iname->name + bigname, iname->len - bigname, - fname->crypto_buf.name); + ret = base64_decode(iname->name + bigname, iname->len - bigname, + fname->crypto_buf.name); if (ret < 0) { ret = -ENOENT; goto errout; diff --git a/fs/crypto/fscrypt_private.h b/fs/crypto/fscrypt_private.h index e39696e64494..f1f15b84e02f 100644 --- a/fs/crypto/fscrypt_private.h +++ b/fs/crypto/fscrypt_private.h @@ -13,8 +13,6 @@ #include <linux/fscrypt_supp.h> -#define FS_FNAME_CRYPTO_DIGEST_SIZE 32 - /* Encryption parameters */ #define FS_XTS_TWEAK_SIZE 16 #define FS_AES_128_ECB_KEY_SIZE 16 diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 07e5e1405771..d1618835de12 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1237,37 +1237,23 @@ static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block) } /* - * NOTE! unlike strncmp, ext4_match returns 1 for success, 0 for failure. + * Determine whether the filename being looked up matches the given dir_entry. * - * `len <= EXT4_NAME_LEN' is guaranteed by caller. - * `de != NULL' is guaranteed by caller. + * Return: true if the entry matches, otherwise false. */ -static inline int ext4_match(struct ext4_filename *fname, - struct ext4_dir_entry_2 *de) +static inline bool ext4_match(const struct ext4_filename *fname, + const struct ext4_dir_entry_2 *de) { - const void *name = fname_name(fname); - u32 len = fname_len(fname); + const struct fscrypt_str *crypto_buf = NULL; if (!de->inode) return 0; #ifdef CONFIG_EXT4_FS_ENCRYPTION - if (unlikely(!name)) { - if (fname->usr_fname->name[0] == '_') { - int ret; - if (de->name_len < 16) - return 0; - ret = memcmp(de->name + de->name_len - 16, - fname->crypto_buf.name + 8, 16); - return (ret == 0) ? 1 : 0; - } - name = fname->crypto_buf.name; - len = fname->crypto_buf.len; - } + crypto_buf = &fname->crypto_buf; #endif - if (de->name_len != len) - return 0; - return (memcmp(de->name, name, len) == 0) ? 1 : 0; + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, + crypto_buf, de->name, de->name_len); } /* @@ -1289,12 +1275,7 @@ int ext4_search_dir(struct buffer_head *bh, char *search_buf, int buf_size, /* this code is executed quadratically often */ /* do minimal checking `by hand' */ if ((char *) de + de->name_len <= dlimit) { - res = ext4_match(fname, de); - if (res < 0) { - res = -1; - goto return_result; - } - if (res > 0) { + if (ext4_match(fname, de)) { /* found a match - just to be sure, do * a full check */ if (ext4_check_dir_entry(dir, NULL, de, bh, @@ -1843,11 +1824,7 @@ int ext4_find_dest_de(struct inode *dir, struct inode *inode, res = -EFSCORRUPTED; goto return_result; } - /* Provide crypto context and crypto buffer to ext4 match */ - res = ext4_match(fname, de); - if (res < 0) - goto return_result; - if (res > 0) { + if (ext4_match(fname, de)) { res = -EEXIST; goto return_result; } diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index 8d5c62b07b28..07b80d78a9f6 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -111,8 +111,6 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, struct f2fs_dir_entry *de; unsigned long bit_pos = 0; int max_len = 0; - struct fscrypt_str de_name = FSTR_INIT(NULL, 0); - struct fscrypt_str *name = &fname->disk_name; if (max_slots) *max_slots = 0; @@ -130,17 +128,10 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, continue; } - /* encrypted case */ - de_name.name = d->filename[bit_pos]; - de_name.len = le16_to_cpu(de->name_len); - - /* show encrypted name */ - if (fname->hash) { - if (de->hash_code == cpu_to_le32(fname->hash)) - goto found; - } else if (de_name.len == name->len && - de->hash_code == namehash && - !memcmp(de_name.name, name->name, name->len)) + if ((fname->hash == 0 || + fname->hash == le32_to_cpu(de->hash_code)) && + fscrypt_name_matches(fname, d->filename[bit_pos], + le16_to_cpu(de->name_len))) goto found; if (max_slots && max_len > *max_slots) diff --git a/include/linux/fscrypt_notsupp.h b/include/linux/fscrypt_notsupp.h index 3511ca798804..4034976bea93 100644 --- a/include/linux/fscrypt_notsupp.h +++ b/include/linux/fscrypt_notsupp.h @@ -147,6 +147,24 @@ static inline int fscrypt_fname_usr_to_disk(struct inode *inode, return -EOPNOTSUPP; } +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, + const struct fscrypt_str *disk_name, + const struct fscrypt_str *crypto_buf, + const char *de_name, u32 de_name_len) +{ + /* Encryption support disabled; use standard comparison. */ + if (de_name_len != disk_name->len) + return false; + return !memcmp(de_name, disk_name->name, disk_name->len); +} + +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, + const char *de_name, u32 de_name_len) +{ + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, + &fname->crypto_buf, de_name, de_name_len); +} + /* bio.c */ static inline void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *ctx, struct bio *bio) diff --git a/include/linux/fscrypt_supp.h b/include/linux/fscrypt_supp.h index a140f47e9b27..e3c9aa7209a1 100644 --- a/include/linux/fscrypt_supp.h +++ b/include/linux/fscrypt_supp.h @@ -57,6 +57,63 @@ extern int fscrypt_fname_disk_to_usr(struct inode *, u32, u32, extern int fscrypt_fname_usr_to_disk(struct inode *, const struct qstr *, struct fscrypt_str *); +/* + * Number of bytes of ciphertext from the end of the filename which the + * filesystem includes when encoding long encrypted filenames for presentation + * to userspace without the key. + */ +#define FS_FNAME_CRYPTO_DIGEST_SIZE (2 * FS_CRYPTO_BLOCK_SIZE) + +/** + * fscrypt_match_dirent() - does the directory entry match the name being looked up? + * + * This is like fscrypt_name_matches(), but for filesystems which don't use the + * fscrypt_name structure. (We probably should make all filesystems do the same + * thing...) + */ +static inline bool fscrypt_match_dirent(const struct qstr *usr_fname, + const struct fscrypt_str *disk_name, + const struct fscrypt_str *crypto_buf, + const char *de_name, u32 de_name_len) +{ + if (unlikely(!disk_name->name)) { + if (WARN_ON_ONCE(usr_fname->name[0] != '_')) + return false; + if (de_name_len < FS_FNAME_CRYPTO_DIGEST_SIZE) + return false; + return !memcmp(de_name + de_name_len - FS_FNAME_CRYPTO_DIGEST_SIZE, + crypto_buf->name + 8, + FS_FNAME_CRYPTO_DIGEST_SIZE); + } + + if (de_name_len != disk_name->len) + return false; + return !memcmp(de_name, disk_name->name, disk_name->len); +} + +/** + * fscrypt_name_matches() - does the directory entry match the name being looked up? + * @fname: the name being looked up + * @de_name: the name from the directory entry + * @de_name_len: the length of @de_name in bytes + * + * Normally @fname->disk_name will be set, and in that case we simply compare + * that to the directory entry. The only exception is that if we don't have the + * key for an encrypted directory and a filename in it is very long, then the + * filename presented to userspace will only have the last + * FS_FNAME_CRYPTO_DIGEST_SIZE bytes of ciphertext encoded in it, so we can only + * compare that portion. Note that despite this limit, due to the use of + * CBC-CTS encryption there should not be any collisions. + * + * Return: true if the name matches, otherwise false. + */ +static inline bool fscrypt_name_matches(const struct fscrypt_name *fname, + const char *de_name, u32 de_name_len) +{ + return fscrypt_match_dirent(fname->usr_fname, &fname->disk_name, + &fname->crypto_buf, de_name, de_name_len); +} + /* bio.c */ extern void fscrypt_decrypt_bio_pages(struct fscrypt_ctx *, struct bio *); extern void fscrypt_pullback_bio_page(struct page **, bool);