Re: [f2fs-dev] [PATCH] fscrypto: fix to null-terminate encrypted filename in fname_encrypt

[Chao Yu]

Hi Ted,

On 2016/8/30 3:08, Theodore Ts'o wrote:
> On Mon, Aug 29, 2016 at 10:55:47PM +0800, Chao Yu wrote:
>> Hi Ted, Jaegeuk,
>>
>> Since encryption functionality in ext4/f2fs was exported to vfs as fscrypot
>> module, more filesystems can use it, I'm not sure, maybe other fs will traverse
>> encrypted filename directly.
>>
>> So, could we set this null character in fname_encrypt in advance in order to
>> avoid hitting random characters behind target filename when traversing it?
> 
> The encrypted filename is only used by the file system; it's not
> anything which is visible outside of the file system --- if it does,
> such as passing it to the security subsystem, it's a bug.
> 
> Secondly, remember that the encrypted filename is a binary blob, and
> may contain hex 00 as part of the encrypted filename.  So ***any***
> code that tries to use NULL termination for the encrypted filename by
> definition is a bug.  In other words, you must use memcpy, and not
> strcpy.  If you use strcpy, even if you did add a NUL character to the
> end of the encrypted filename (which is a bit of a misnomer because it
> is a binary blob, not an ASCII string, so NUL is really not
> technically correct), there will be encrypted filenames where strcpy
> will stop early, because there is a 0x00 byte in the encrypted
> filename.
> 
> Hence, other file systems MUST NOT traverse the encrypted filename
> directly, because treating it as a NUL-terminated string when it is
> really a binary blob of bits that can include a 0x00 byte is by
> definition a BUG.

Thanks for your detailed explain. :)

I just be misguided by comments of following code:

int fscrypt_fname_alloc_buffer(struct inode *inode,
				u32 ilen, struct fscrypt_str *crypto_str)
{
	unsigned int olen = fscrypt_fname_encrypted_size(inode, ilen);

	crypto_str->len = olen;
	if (olen < FS_FNAME_CRYPTO_DIGEST_SIZE * 2)
		olen = FS_FNAME_CRYPTO_DIGEST_SIZE * 2;
	/*
	 * Allocated buffer can hold one more character to null-terminate the
	 * string
	 */
	crypto_str->name = kmalloc(olen + 1, GFP_NOFS);
	if (!(crypto_str->name))
		return -ENOMEM;
	return 0;
}
EXPORT_SYMBOL(fscrypt_fname_alloc_buffer);

Thanks,

> 
> Cheers,
> 
> 						- Ted
> 
> .
>