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

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

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch fixes to add null character at the end of encrypted filename
in fname_encrypt, in order to avoid incorrectly traversing random data
located after target filename. The call stack is as below:

- f2fs_add_link
 - __f2fs_add_link
  - fscrypt_setup_filename
   - fscrypt_fname_alloc_buffer		allocate buffer for @fname
   - fname_encrypt			didn't set null character for @fname
  - f2fs_add_regular_entry		init qstr with @fname
   - init_inode_metadata
    - f2fs_init_security
     - security_inode_init_security
      - selinux_inode_init_security
       - selinux_determine_inode_label
        - security_transition_sid
	 - security_compute_sid
	  - filename_compute_type
	   - hashtab_search
	    - filenametr_hash		traverse @fname as one which has null character

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/crypto/fname.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/crypto/fname.c b/fs/crypto/fname.c
index 5d6d491..5c356c0 100644
--- a/fs/crypto/fname.c
+++ b/fs/crypto/fname.c
@@ -110,6 +110,7 @@ static int fname_encrypt(struct inode *inode,
 				"%s: Error (error code %d)\n", __func__, res);
 
 	oname->len = ciphertext_len;
+	oname->name[oname->len] = 0;
 	return res;
 }
 
-- 
2.7.2

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

[Theodore Ts'o]

On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
> 
> This patch fixes to add null character at the end of encrypted filename
> in fname_encrypt, in order to avoid incorrectly traversing random data
> located after target filename. The call stack is as below:
> 
> - f2fs_add_link
>  - __f2fs_add_link
>   - fscrypt_setup_filename
>    - fscrypt_fname_alloc_buffer		allocate buffer for @fname
>    - fname_encrypt			didn't set null character for @fname
>   - f2fs_add_regular_entry		init qstr with @fname
>    - init_inode_metadata
>     - f2fs_init_security
>      - security_inode_init_security
>       - selinux_inode_init_security
>        - selinux_determine_inode_label
>         - security_transition_sid
> 	 - security_compute_sid
> 	  - filename_compute_type
> 	   - hashtab_search
> 	    - filenametr_hash		traverse @fname as one which has null character

The problem is not in fname_encrypt(), but rather that
security_inode_init_security() should be given the _unencrypted_
filename.

In ext4 security_inode_init_security() is called with the qstr from
the dentry, not the encrypted qstr --- in fact we call
security_inode_init_security before we call fname_encrypt.

SELinux needs the unencrypted filename in order to decide which
SELinux rules / labels should apply.

					- Ted

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

[Chao Yu]

Hi Ted,

On 2016/8/28 13:13, Theodore Ts'o wrote:
> On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> This patch fixes to add null character at the end of encrypted filename
>> in fname_encrypt, in order to avoid incorrectly traversing random data
>> located after target filename. The call stack is as below:
>>
>> - f2fs_add_link
>>  - __f2fs_add_link
>>   - fscrypt_setup_filename
>>    - fscrypt_fname_alloc_buffer		allocate buffer for @fname
>>    - fname_encrypt			didn't set null character for @fname
>>   - f2fs_add_regular_entry		init qstr with @fname
>>    - init_inode_metadata
>>     - f2fs_init_security
>>      - security_inode_init_security
>>       - selinux_inode_init_security
>>        - selinux_determine_inode_label
>>         - security_transition_sid
>> 	 - security_compute_sid
>> 	  - filename_compute_type
>> 	   - hashtab_search
>> 	    - filenametr_hash		traverse @fname as one which has null character
> 
> The problem is not in fname_encrypt(), but rather that
> security_inode_init_security() should be given the _unencrypted_
> filename.
> 
> In ext4 security_inode_init_security() is called with the qstr from
> the dentry, not the encrypted qstr --- in fact we call
> security_inode_init_security before we call fname_encrypt.
> 
> SELinux needs the unencrypted filename in order to decide which
> SELinux rules / labels should apply.

You're right, I missed this mistake. So actually, this is a bug of f2fs.
Let me figure out the fixing patch.

Thanks for your review! :)

Thanks,

> 
> 					- Ted
> 

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

[Chao Yu]

Hi Ted, Jaegeuk,

On 2016/8/28 14:16, Chao Yu wrote:
> Hi Ted,
> 
> On 2016/8/28 13:13, Theodore Ts'o wrote:
>> On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
>>> From: Chao Yu <yuchao0@huawei.com>
>>>
>>> This patch fixes to add null character at the end of encrypted filename

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?

Thanks,

>>> in fname_encrypt, in order to avoid incorrectly traversing random data
>>> located after target filename. The call stack is as below:
>>>
>>> - f2fs_add_link
>>>  - __f2fs_add_link
>>>   - fscrypt_setup_filename
>>>    - fscrypt_fname_alloc_buffer		allocate buffer for @fname
>>>    - fname_encrypt			didn't set null character for @fname
>>>   - f2fs_add_regular_entry		init qstr with @fname
>>>    - init_inode_metadata
>>>     - f2fs_init_security
>>>      - security_inode_init_security
>>>       - selinux_inode_init_security
>>>        - selinux_determine_inode_label
>>>         - security_transition_sid
>>> 	 - security_compute_sid
>>> 	  - filename_compute_type
>>> 	   - hashtab_search
>>> 	    - filenametr_hash		traverse @fname as one which has null character
>>
>> The problem is not in fname_encrypt(), but rather that
>> security_inode_init_security() should be given the _unencrypted_
>> filename.
>>
>> In ext4 security_inode_init_security() is called with the qstr from
>> the dentry, not the encrypted qstr --- in fact we call
>> security_inode_init_security before we call fname_encrypt.
>>
>> SELinux needs the unencrypted filename in order to decide which
>> SELinux rules / labels should apply.
> 
> You're right, I missed this mistake. So actually, this is a bug of f2fs.
> Let me figure out the fixing patch.
> 
> Thanks for your review! :)
> 
> Thanks,
> 
>>
>> 					- Ted
>>
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

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

[Jaegeuk Kim]

On Mon, Aug 29, 2016 at 10:55:47PM +0800, Chao Yu wrote:
> Hi Ted, Jaegeuk,
> 
> On 2016/8/28 14:16, Chao Yu wrote:
> > Hi Ted,
> > 
> > On 2016/8/28 13:13, Theodore Ts'o wrote:
> >> On Sun, Aug 28, 2016 at 09:13:28AM +0800, Chao Yu wrote:
> >>> From: Chao Yu <yuchao0@huawei.com>
> >>>
> >>> This patch fixes to add null character at the end of encrypted filename
> 
> 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?

When taking a look at fscrypt_fname_alloc_buffer(),

	/*  
	 * Allocated buffer can hold one more character to null-terminate the
	 * string
	 */
	crypto_str->name = kmalloc(olen + 1, GFP_NOFS);

So, there'd be an alternative way which calls kzalloc() here.

Thanks,

> 
> Thanks,
> 
> >>> in fname_encrypt, in order to avoid incorrectly traversing random data
> >>> located after target filename. The call stack is as below:
> >>>
> >>> - f2fs_add_link
> >>>  - __f2fs_add_link
> >>>   - fscrypt_setup_filename
> >>>    - fscrypt_fname_alloc_buffer		allocate buffer for @fname
> >>>    - fname_encrypt			didn't set null character for @fname
> >>>   - f2fs_add_regular_entry		init qstr with @fname
> >>>    - init_inode_metadata
> >>>     - f2fs_init_security
> >>>      - security_inode_init_security
> >>>       - selinux_inode_init_security
> >>>        - selinux_determine_inode_label
> >>>         - security_transition_sid
> >>> 	 - security_compute_sid
> >>> 	  - filename_compute_type
> >>> 	   - hashtab_search
> >>> 	    - filenametr_hash		traverse @fname as one which has null character
> >>
> >> The problem is not in fname_encrypt(), but rather that
> >> security_inode_init_security() should be given the _unencrypted_
> >> filename.
> >>
> >> In ext4 security_inode_init_security() is called with the qstr from
> >> the dentry, not the encrypted qstr --- in fact we call
> >> security_inode_init_security before we call fname_encrypt.
> >>
> >> SELinux needs the unencrypted filename in order to decide which
> >> SELinux rules / labels should apply.
> > 
> > You're right, I missed this mistake. So actually, this is a bug of f2fs.
> > Let me figure out the fixing patch.
> > 
> > Thanks for your review! :)
> > 
> > Thanks,
> > 
> >>
> >> 					- Ted
> >>
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Linux-f2fs-devel mailing list
> > Linux-f2fs-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> > 

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

[Theodore Ts'o]

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.

Cheers,

						- Ted

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
> 
> .
>