From: Eric Biggers <ebiggers3@gmail.com> To: linux-fscrypt@vger.kernel.org Cc: "Theodore Y . Ts'o" <tytso@mit.edu>, Jaegeuk Kim <jaegeuk@kernel.org>, linux-f2fs-devel@lists.sourceforge.net, linux-ext4@vger.kernel.org, linux-mtd@lists.infradead.org, Gwendal Grignou <gwendal@chromium.org>, hashimoto@chromium.org, kinaba@chromium.org, Eric Biggers <ebiggers@google.com>, stable@vger.kernel.org Subject: [PATCH 2/6] fscrypt: avoid collisions when presenting long encrypted filenames Date: Mon, 24 Apr 2017 10:00:09 -0700 [thread overview] Message-ID: <20170424170013.85175-3-ebiggers3@gmail.com> (raw) In-Reply-To: <20170424170013.85175-1-ebiggers3@gmail.com> 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> --- fs/crypto/fname.c | 2 +- fs/ext4/namei.c | 4 ++-- fs/f2fs/dir.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) 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; -- 2.12.2.816.g2cccc81164-goog
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers3@gmail.com> To: linux-fscrypt@vger.kernel.org Cc: hashimoto@chromium.org, Gwendal Grignou <gwendal@chromium.org>, "Theodore Y . Ts'o" <tytso@mit.edu>, Eric Biggers <ebiggers@google.com>, stable@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-mtd@lists.infradead.org, Jaegeuk Kim <jaegeuk@kernel.org>, linux-ext4@vger.kernel.org, kinaba@chromium.org Subject: [PATCH 2/6] fscrypt: avoid collisions when presenting long encrypted filenames Date: Mon, 24 Apr 2017 10:00:09 -0700 [thread overview] Message-ID: <20170424170013.85175-3-ebiggers3@gmail.com> (raw) In-Reply-To: <20170424170013.85175-1-ebiggers3@gmail.com> 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> --- fs/crypto/fname.c | 2 +- fs/ext4/namei.c | 4 ++-- fs/f2fs/dir.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) 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; -- 2.12.2.816.g2cccc81164-goog ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
next prev parent reply other threads:[~2017-04-24 17:00 UTC|newest] Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-04-24 17:00 [PATCH 0/6] fscrypt: fixes for presentation of long encrypted filenames Eric Biggers 2017-04-24 17:00 ` Eric Biggers 2017-04-24 17:00 ` [PATCH 1/6] f2fs: check entire encrypted bigname when finding a dentry Eric Biggers 2017-04-25 0:10 ` Jaegeuk Kim 2017-05-03 2:56 ` Eric Biggers 2017-05-03 4:21 ` Jaegeuk Kim 2017-04-30 6:19 ` [1/6] " Theodore Ts'o 2017-04-24 17:00 ` Eric Biggers [this message] 2017-04-24 17:00 ` [PATCH 2/6] fscrypt: avoid collisions when presenting long encrypted filenames Eric Biggers 2017-04-30 6:19 ` [2/6] " Theodore Ts'o 2017-04-24 17:00 ` [PATCH 3/6] fscrypt: introduce helper function for filename matching Eric Biggers 2017-04-24 17:00 ` Eric Biggers 2017-04-28 21:18 ` Eric Biggers 2017-04-30 6:20 ` [3/6] " Theodore Ts'o 2017-04-24 17:00 ` [PATCH 4/6] ext4: switch to using fscrypt_match_name() Eric Biggers 2017-04-24 17:00 ` Eric Biggers 2017-04-30 6:21 ` [4/6] " Theodore Ts'o 2017-04-24 17:00 ` [PATCH 5/6] f2fs: " Eric Biggers 2017-04-24 17:00 ` Eric Biggers 2017-04-25 0:16 ` Jaegeuk Kim 2017-04-25 13:37 ` Richard Weinberger 2017-04-25 17:46 ` Eric Biggers 2017-04-25 17:46 ` Eric Biggers 2017-04-25 19:22 ` Richard Weinberger 2017-04-25 19:22 ` Richard Weinberger 2017-04-25 20:58 ` Eric Biggers 2017-04-25 21:03 ` Richard Weinberger 2017-04-30 6:21 ` [5/6] " Theodore Ts'o 2017-04-24 17:00 ` [PATCH 6/6] ext4: clean up ext4_match() and callers Eric Biggers 2017-04-24 17:00 ` Eric Biggers 2017-04-30 6:22 ` [6/6] " Theodore Ts'o
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170424170013.85175-3-ebiggers3@gmail.com \ --to=ebiggers3@gmail.com \ --cc=ebiggers@google.com \ --cc=gwendal@chromium.org \ --cc=hashimoto@chromium.org \ --cc=jaegeuk@kernel.org \ --cc=kinaba@chromium.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-mtd@lists.infradead.org \ --cc=stable@vger.kernel.org \ --cc=tytso@mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.