2021-09-21 16:33 GMT+09:00, Ralph Boehme <slow@xxxxxxxxx>:
> 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@xxxxxxxxx>
>> Cc: Steve French <smfrench@xxxxxxxxx>
>> Acked-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
>> ---
>> 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
>
>
