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
