| Message ID | 20170424170013.85175-3-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, Apr 24, 2017 at 10:00:09AM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > When accessing an encrypted directory without the key, userspace must > operate on filenames derived from the ciphertext names, which contain > arbitrary bytes. Since we must support filenames as long as NAME_MAX, > we can't always just base64-encode the ciphertext, since that may make > it too long. Currently, this is solved by presenting long names in an > abbreviated form containing any needed filesystem-specific hashes (e.g. > to identify a directory block), then the last 16 bytes of ciphertext. > This needs to be sufficient to identify the actual name on lookup. > > However, there is a bug. It seems to have been assumed that due to the > use of a CBC (ciphertext block chaining)-based encryption mode, the last > 16 bytes (i.e. the AES block size) of ciphertext would depend on the > full plaintext, preventing collisions. However, we actually use CBC > with ciphertext stealing (CTS), which handles the last two blocks > specially, causing them to appear "flipped". Thus, it's actually the > second-to-last block which depends on the full plaintext. > > This caused long filenames that differ only near the end of their > plaintexts to, when observed without the key, point to the wrong inode > and be undeletable. For example, with ext4: > > # echo pass | e4crypt add_key -p 16 edir/ > # 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 > 2004 > # rm -rf edir/ > rm: cannot remove 'edir/_A7nNFi3rhkEQlJ6P,hdzluhODKOeWx5V': Structure needs cleaning > ... > > To fix this, when presenting long encrypted filenames, encode the > second-to-last block of ciphertext rather than the last 16 bytes. > > Although it would be nice to solve this without depending on a specific > encryption mode, that would mean doing a cryptographic hash like SHA-256 > which would be much less efficient. This way is sufficient for now, and > it's still compatible with encryption modes like HEH which are strong > pseudorandom permutations. Also, changing the presented names is still > allowed at any time because they are only provided to allow applications > to do things like delete encrypted directories. They're not designed to > be used to persistently identify files --- which would be hard to do > anyway, given that they're encrypted after all. > > For ease of backports, this patch only makes the minimal fix to both > ext4 and f2fs. It leaves ubifs as-is, since ubifs doesn't compare the > ciphertext block yet. Follow-on patches will clean things up properly > and make the filesystems use a shared helper function. > > Fixes: 5de0b4d0cd15 ("ext4 crypto: simplify and speed up filename encryption") > Reported-by: Gwendal Grignou <gwendal@chromium.org> > Cc: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> Thanks, applied. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c index 13052b85c393..932881f27f2f 100644 --- a/fs/crypto/fname.c +++ b/fs/crypto/fname.c @@ -300,7 +300,7 @@ int fscrypt_fname_disk_to_usr(struct inode *inode, } else { memset(buf, 0, 8); } - memcpy(buf + 8, iname->name + iname->len - 16, 16); + memcpy(buf + 8, iname->name + ((iname->len - 17) & ~15), 16); oname->name[0] = '_'; oname->len = 1 + digest_encode(buf, 24, oname->name + 1); return 0; diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 6ad612c576fc..e6301b6933fc 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1255,9 +1255,9 @@ static inline int ext4_match(struct ext4_filename *fname, if (unlikely(!name)) { if (fname->usr_fname->name[0] == '_') { int ret; - if (de->name_len < 16) + if (de->name_len <= 32) return 0; - ret = memcmp(de->name + de->name_len - 16, + ret = memcmp(de->name + ((de->name_len - 17) & ~15), fname->crypto_buf.name + 8, 16); return (ret == 0) ? 1 : 0; } diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c index 374e4b8f9b70..5df3596a667a 100644 --- a/fs/f2fs/dir.c +++ b/fs/f2fs/dir.c @@ -139,8 +139,8 @@ struct f2fs_dir_entry *find_target_dentry(struct fscrypt_name *fname, #ifdef CONFIG_F2FS_FS_ENCRYPTION if (unlikely(!name->name)) { if (fname->usr_fname->name[0] == '_') { - if (de_name.len >= 16 && - !memcmp(de_name.name + de_name.len - 16, + if (de_name.len > 32 && + !memcmp(de_name.name + ((de_name.len - 17) & ~15), fname->crypto_buf.name + 8, 16)) goto found; goto not_match;