Re: [PATCH v3] ksmbd: remove follow symlinks support

[Namjae Jeon <linkinjeon@kernel.org>]

2021-09-21 16:33 GMT+09:00, Ralph Boehme <slow@samba.org>:
> Hi Namjae,
>
> excellent! One issue below.
>
> Am 21.09.21 um 07:19 schrieb Namjae Jeon:
>> Use  LOOKUP_NO_SYMLINKS flags for default lookup to prohibit the middle
>> of
>> symlink component lookup and remove follow symlinks parameter support.
>> We re-implement it as reparse point later.
>>
>> Test result:
>> smbclient -Ulinkinjeon%1234 //172.30.1.42/share -c
>> "get hacked/passwd passwd"
>> NT_STATUS_OBJECT_NAME_NOT_FOUND opening remote file \hacked\passwd
>>
>> Cc: Ralph Böhme <slow@samba.org>
>> Cc: Steve French <smfrench@gmail.com>
>> Acked-by: Ronnie Sahlberg <lsahlber@redhat.com>
>> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org>
>> ---
>>   v2:
>>    - reorganize path lookup in smb2_open to call only one
>>      ksmbd_vfs_kern_path().
>>   v3:
>>    - combine "ksmbd: use LOOKUP_NO_SYMLINKS flags for default lookup"
>>      patch into this patch.
>>
>>   fs/ksmbd/smb2pdu.c | 43 ++++++++++---------------------------------
>>   fs/ksmbd/vfs.c     | 32 +++++++++-----------------------
>>   2 files changed, 19 insertions(+), 56 deletions(-)
>>
>> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
>> index 4399c399284b..baf7ce31d557 100644
>> --- a/fs/ksmbd/smb2pdu.c
>> +++ b/fs/ksmbd/smb2pdu.c
>> @@ -2660,13 +2660,9 @@ int smb2_open(struct ksmbd_work *work)
>>   		goto err_out1;
>>   	}
>>
>> -	if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
>> -		/*
>> -		 * On delete request, instead of following up, need to
>> -		 * look the current entity
>> -		 */
>> -		rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
>> -		if (!rc) {
>> +	rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
>> +	if (!rc) {
>> +		if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
>>   			/*
>>   			 * If file exists with under flags, return access
>>   			 * denied error.
>> @@ -2685,25 +2681,10 @@ int smb2_open(struct ksmbd_work *work)
>>   				path_put(&path);
>>   				goto err_out;
>>   			}
>> -		}
>> -	} else {
>> -		if (test_share_config_flag(work->tcon->share_conf,
>> -					   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
>> -			/*
>> -			 * Use LOOKUP_FOLLOW to follow the path of
>> -			 * symlink in path buildup
>> -			 */
>> -			rc = ksmbd_vfs_kern_path(name, LOOKUP_FOLLOW, &path, 1);
>> -			if (rc) { /* Case for broken link ?*/
>> -				rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
>> -			}
>> -		} else {
>> -			rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
>> -			if (!rc && d_is_symlink(path.dentry)) {
>> -				rc = -EACCES;
>> -				path_put(&path);
>> -				goto err_out;
>> -			}
>> +		} else if (d_is_symlink(path.dentry)) {
>> +			rc = -EACCES;
>> +			path_put(&path);
>> +			goto err_out;
>
> I wonder whether the d_is_symlink() check should be done *inside*
> ksmbd_vfs_kern_path()? The idea being having one function where the
> required semantics are implemented without bleeding logic stuff in the
> callers. ksmbd_vfs_kern_path() could simply return -ELOOP if *any* path
> component is a symlink.
>
> Then of course the question is how to handle this in some callers to
> make the decision what how to present the directory entry to the caller.
>
> For example in ksmbd_vfs_readdir_name() I'm not sure if we return file
> metadata from the link target.
Okay, I will check it, And Hyunchul is checking LOOKUP_BENEATH flags
now. If it work properly, such symlink check may not needed.
Thanks!
>
> Thanks!
> -slow
>
> --
> Ralph Boehme, Samba Team                 https://samba.org/
> SerNet Samba Team Lead      https://sernet.de/en/team-samba
>
>