On Sun, 2021-07-11 at 17:53 -0500, Eric Biggers wrote:
> 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.
>
Oh, I misunderstood the meaning of the last parameter to
fscrypt_fname_encrypt. I had thought that was the length of the target
buffer, but it's not -- it's the length of the resulting filename (which
we need to precompute). I'll fix that up.
> 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.
>
That might make more sense.
> > +
> > + /* 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.
>
Will fix, thanks.
--
Jeff Layton <jlayton@xxxxxxxxxx>
