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>
![]() |