On Apr 18, 2017, at 3:06 PM, Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
> Subject: [PATCH] fscrypt: use 32 bytes of encrypted filename
> If we use only 16 bytes, due to how CBC works, if the names have the
> same beginning, their last ciphertext block (16 bytes) may be identical.
What is missing from the patch summary is: use the encrypted filename for
_what_?
>
> It happens when two file names share the first 16k bytes and both have
"16k bytes" for the file names? Presumably you don't mean "16KB", but
rather "16n" or "multiple of 16 bytes" would be more clear.
> length withn 16 * n + 13 and 16 * n + 16.
>
> Instead use 32 bytes to build the filenames from encrypted data when
> directory is scrambled.
>
> The drawback is the scrambled filenames change after applying the patch.
> Consider an encrypted directory with:
>
> ls -il
> total 8
> 1177380 -rw-r--r--. 1 root root 0 Apr 18 12:10
> system@framework@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 1177379 -rw-r--r--. 1 root root 0 Apr 18 12:10
> system@framework@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
> Once the key is invalidated, without the patch, ls -li produces:
> 1177379 -rw-r--r--. 1 root root 0 Apr 18 12:10
> _a1Psh01n8FdhC8s9pUywlAyFzlz7n6C3
> 1177379 -rw-r--r--. 1 root root 0 Apr 18 12:10
> _wJS,0akq14ehC8s9pUywlAyFzlz7n6C3
>
> Both files show with the same inode.
>
> After the patch, the names are different, but the inode information is
> now correct:
> ls -li
> 1177380 -rw-r--r--. 1 root root 0 Apr 18 12:10
> _a1Psh01n8FtxbeglW8BqhuthSUxMqh6cFKwz2nSJDXCIXMXOvfqLcD
> 1177379 -rw-r--r--. 1 root root 0 Apr 18 12:10
> _wJS,0akq14eJcuQks7f2Vsg,zE0Jdz98FKwz2nSJDXCIXMXOvfqLcD
It isn't clear to me what the difference is here. In the first case
(without patch) the last 21 characters of the filename are the same,
but the first 11 characters are still different, so it isn't clear why
that isn't enough to distinguish the files, and why checking the whole
filename isn't enough to properly determine the inode number?
In the second case (with patch) the last 22 characters are also the same.
> Tested only on ext4.
>
> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> ---
> Script to reproduce the error:
>
> BASE_DIR="~/"
> DIR="${BASE_DIR}/tmp"
> # Create directory.
> mkdir -p "${DIR}"
> echo foobar | e4crypt add_key "${DIR}"
> # Fill directory.
> cd "${DIR}"
> touch system@framework@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> touch system@framework@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> cd ..
> # Check files have different inode.
> ls -il "${DIR}"
> # Invalidate key
> KEY="$(keyctl show | grep $(e4crypt get_policy "${DIR}" | cut -d ':' -f 2) | sed -ne 's/\(.*\) --al.*/\1/p')"
> sync
> keyctl invalidate "${KEY}"
> echo 3 > /proc/sys/vm/drop_caches
> # Once the key is invalidated, both files have the same inode:
> ls -il "${DIR}"
> if [ $(ls -i1 "${DIR}" | cut -d ' ' -f 1 | uniq | wc -l) -eq 1 ] ; then
> echo same inode!
> fi
> # if we try to remove the directory, we will get an error
> # : Structure needs cleaning
> # rm -rf "${DIR}"
>
> fs/crypto/fname.c | 20 +++++++++++++-------
> fs/ext4/namei.c | 4 ++--
> 2 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
> index 80bb956e14e5..71ddc3eaa62d 100644
> --- a/fs/crypto/fname.c
> +++ b/fs/crypto/fname.c
> @@ -274,7 +274,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[FS_FNAME_CRYPTO_DIGEST_SIZE + sizeof(u64)];
>
> if (fscrypt_is_dot_dotdot(&qname)) {
> oname->name[0] = '.';
> @@ -295,14 +295,19 @@ int fscrypt_fname_disk_to_usr(struct inode *inode,
> return 0;
> }
> if (hash) {
> - memcpy(buf, &hash, 4);
> - memcpy(buf + 4, &minor_hash, 4);
> + memcpy(buf, &hash, sizeof(u32));
> + memcpy(buf + 4, &minor_hash, sizeof(u32));
Should "4" be replaced with something here? sizeof(u32), or something else?
> } else {
> memset(buf, 0, 8);
Likewise, should "8" be replaced with sizeof(u64) as it is with other changes?
> }
> - memcpy(buf + 8, iname->name + iname->len - 16, 16);
> + memcpy(buf + sizeof(u64),
> + 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 + digest_encode(
> + buf,
> + FS_FNAME_CRYPTO_DIGEST_SIZE + sizeof(u64),
> + oname->name + 1);
> return 0;
> }
> EXPORT_SYMBOL(fscrypt_fname_disk_to_usr);
> @@ -375,10 +380,11 @@ int fscrypt_setup_filename(struct inode *dir, const struct qstr *iname,
> */
> if (iname->name[0] == '_')
> bigname = 1;
> - if ((bigname && (iname->len != 33)) || (!bigname && (iname->len > 43)))
> + if ((bigname && iname->len != 55) || (!bigname && (iname->len > 43)))
The "55" constant is no better than "33" in terms of clarity, nor is "43".
Having a (computed) constant for this would be more helpful, for example
2 * sizeof(buf) + 1
or whatever.
> return -ENOENT;
>
> - fname->crypto_buf.name = kmalloc(32, GFP_KERNEL);
> + fname->crypto_buf.name = kmalloc(
> + FS_FNAME_CRYPTO_DIGEST_SIZE + sizeof(u64), GFP_KERNEL);
> if (fname->crypto_buf.name == NULL)
> return -ENOMEM;
>
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index c4a389a6027b..14b2a2335a32 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1257,8 +1257,8 @@ static inline int ext4_match(struct ext4_filename *fname,
> int ret;
> if (de->name_len < 16)
> return 0;
> - ret = memcmp(de->name + de->name_len - 16,
> - fname->crypto_buf.name + 8, 16);
> + ret = memcmp(de->name + de->name_len - 32,
> + fname->crypto_buf.name + 8, 32);
> return (ret == 0) ? 1 : 0;
This could just be:
return (ret == 0);
> }
> name = fname->crypto_buf.name;
> len = fname->crypto_buf.len;
> }
> #endif
> if (de->name_len != len)
> return 0;
> return (memcmp(de->name, name, len) == 0) ? 1 : 0;
And similarly:
return (memcmp(de->name, name, len) == 0);
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
|