Re: [PATCH] ksmbd: remove follow symlinks support

[Ralph Boehme <slow@xxxxxxxxx>]

Hi Namjae,

Am 20.09.21 um 08:56 schrieb Namjae Jeon:
> This patch remove symlink support that can be vulnerable, and we
> re-implement it as reparse point later.
>
> Cc: Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx>
> Cc: Ralph Böhme <slow@xxxxxxxxx>
> Cc: Steve French <smfrench@xxxxxxxxx>
> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxxx>
> ---
>   fs/ksmbd/smb2pdu.c | 55 ++++++++++------------------------------------
>   fs/ksmbd/vfs.c     | 50 +++++++++--------------------------------
>   2 files changed, 21 insertions(+), 84 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index afc508c2656d..911c5e97678d 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -2661,17 +2661,7 @@ int smb2_open(struct ksmbd_work *work)
>   	}
>   
>   	if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE) {
> -		if (test_share_config_flag(work->tcon->share_conf,
> -					   KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS)) {
> -			/*
> -			 * On delete request, instead of following up, need to
> -			 * look the current entity
> -			 */
> -			rc = ksmbd_vfs_kern_path(name, 0, &path, 1);
> -		} else {
> -			rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> -		}
> -
> +		rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
>   		if (!rc) {
>   			/*
>   			 * If file exists with under flags, return access
> @@ -2693,24 +2683,11 @@ int smb2_open(struct ksmbd_work *work)
>   			}
>   		}
>   	} 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, LOOKUP_NO_SYMLINKS,
> -						 &path, 1);
> -			if (!rc && d_is_symlink(path.dentry)) {
> -				rc = -EACCES;
> -				path_put(&path);
> -				goto err_out;
> -			}
> +		rc = ksmbd_vfs_kern_path(name, LOOKUP_NO_SYMLINKS, &path, 1);
> +		if (!rc && d_is_symlink(path.dentry)) {
> +			rc = -EACCES;
> +			path_put(&path);
> +			goto err_out;

why the the check for d_is_symlink()? Wouldn't ksmbd_vfs_kern_path() 
just return -ELOOP if a path component is a symlink? Hence I guess the 
below if (rc) where we handle EACCESS should be expanded to handle ELOOP.

I guess I would also refactor the

if (req->CreateOptions & FILE_DELETE_ON_CLOSE_LE)

logic in a previous commit to change the codeflow so there's only one 
call to ksmbd_vfs_kern_path().

>   		}
>   	}
>   
> @@ -4795,12 +4772,8 @@ static int smb2_get_info_filesystem(struct ksmbd_work *work,
>   	struct path path;
>   	int rc = 0, len;
>   	int fs_infoclass_size = 0;
> -	int lookup_flags = LOOKUP_NO_SYMLINKS;
>   
> -	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> -		lookup_flags = LOOKUP_FOLLOW;
> -
> -	rc = ksmbd_vfs_kern_path(share->path, lookup_flags, &path, 0);
> +	rc = ksmbd_vfs_kern_path(share->path, LOOKUP_NO_SYMLINKS, &path, 0);
>   	if (rc) {
>   		pr_err("cannot create vfs path\n");
>   		return -EIO;

why doesn't this return rc?

> @@ -5307,7 +5280,7 @@ static int smb2_rename(struct ksmbd_work *work,
>   	char *pathname = NULL;
>   	struct path path;
>   	bool file_present = true;
> -	int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
> +	int rc;
>   
>   	ksmbd_debug(SMB, "setting FILE_RENAME_INFO\n");
>   	pathname = kmalloc(PATH_MAX, GFP_KERNEL);
> @@ -5376,11 +5349,8 @@ static int smb2_rename(struct ksmbd_work *work,
>   		goto out;
>   	}
>   
> -	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> -		lookup_flags = LOOKUP_FOLLOW;
> -
>   	ksmbd_debug(SMB, "new name %s\n", new_name);
> -	rc = ksmbd_vfs_kern_path(new_name, lookup_flags, &path, 1);
> +	rc = ksmbd_vfs_kern_path(new_name, LOOKUP_NO_SYMLINKS, &path, 1);
>   	if (rc)
>   		file_present = false;

I guess this should handle ELOOP?

>   	else
> @@ -5430,7 +5400,7 @@ static int smb2_create_link(struct ksmbd_work *work,
>   	char *link_name = NULL, *target_name = NULL, *pathname = NULL;
>   	struct path path;
>   	bool file_present = true;
> -	int rc, lookup_flags = LOOKUP_NO_SYMLINKS;
> +	int rc;
>   
>   	if (buf_len < sizeof(struct smb2_file_link_info) +
>   			le32_to_cpu(file_info->FileNameLength))
> @@ -5457,11 +5427,8 @@ static int smb2_create_link(struct ksmbd_work *work,
>   		goto out;
>   	}
>   
> -	if (test_share_config_flag(share, KSMBD_SHARE_FLAG_FOLLOW_SYMLINKS))
> -		lookup_flags = LOOKUP_FOLLOW;
> -
>   	ksmbd_debug(SMB, "target name is %s\n", target_name);
> -	rc = ksmbd_vfs_kern_path(link_name, lookup_flags, &path, 0);
> +	rc = ksmbd_vfs_kern_path(link_name, LOOKUP_NO_SYMLINKS, &path, 0);
>   	if (rc)
>   		file_present = false;

same here?

Other then that: lgtm. Oh, besides, guess this needs an accomanying 
change to ksmbd-tools?

Cheers!
-slow

--
Ralph Boehme, Samba Team                 https://samba.org/
SerNet Samba Team Lead      https://sernet.de/en/team-samba

Attachment: OpenPGP_signature
Description: OpenPGP digital signature