| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Dec 2020 23:32:41 +0530
From: Shyam Prasad N <nspmangalore@...il.com>
To: Boris Protopopov <pboris@...zon.com>
Cc: Steven French <sfrench@...ba.org>,
CIFS <linux-cifs@...r.kernel.org>,
samba-technical <samba-technical@...ts.samba.org>,
LKML <linux-kernel@...r.kernel.org>, samjonas@...zon.com
Subject: Re: [PATCH 2/2] Add SMB 2 support for getting and setting SACLs
Minor comments inline...
On Fri, Dec 18, 2020 at 2:30 AM Boris Protopopov <pboris@...zon.com> wrote:
>
> Fix passing of the additional security info via version
> operations. Force new open when getting SACL and avoid
> reuse of files that were previously open without
> sufficient privileges to access SACLs.
>
> Signed-off-by: Boris Protopopov <pboris@...zon.com>
> ---
>
> After further testing, I found that the security info was not being
> passed correctly to opts->get_acl and opts->get_acl_by_fid(). Also,
> it turned out that files open for read were being used to fetch
> SACL without proper privileges. This patch fixes these issues, and
> is meant to be squashed (comments dropped) with the earlier patch.
>
> fs/cifs/cifsacl.c | 10 +++++-----
> fs/cifs/smb2ops.c | 4 ++--
> fs/cifs/smb2pdu.c | 4 +++-
> fs/cifs/xattr.c | 10 ++++------
> 4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 353394d9ada8..6baa121952ce 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -1245,7 +1245,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
> int rc = 0;
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> struct smb_version_operations *ops;
> - const u32 unused = 0;
> + const u32 info = 0;
>
> cifs_dbg(NOISY, "converting ACL to mode for %s\n", path);
>
> @@ -1255,9 +1255,9 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
> ops = tlink_tcon(tlink)->ses->server->ops;
>
> if (pfid && (ops->get_acl_by_fid))
> - pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, unused);
> + pntsd = ops->get_acl_by_fid(cifs_sb, pfid, &acllen, info);
> else if (ops->get_acl)
> - pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, unused);
> + pntsd = ops->get_acl(cifs_sb, inode, path, &acllen, info);
> else {
> cifs_put_tlink(tlink);
> return -EOPNOTSUPP;
> @@ -1295,7 +1295,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode,
> struct tcon_link *tlink = cifs_sb_tlink(cifs_sb);
> struct smb_version_operations *ops;
> bool mode_from_sid, id_from_sid;
> - const u32 unused = 0;
> + const u32 info = 0;
>
> if (IS_ERR(tlink))
> return PTR_ERR(tlink);
> @@ -1311,7 +1311,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode,
> return -EOPNOTSUPP;
> }
>
> - pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, unused);
> + pntsd = ops->get_acl(cifs_sb, inode, path, &secdesclen, info);
> if (IS_ERR(pntsd)) {
> rc = PTR_ERR(pntsd);
> cifs_dbg(VFS, "%s: error %d getting sec desc\n", __func__, rc);
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index d28a29728fb1..f5e198860c16 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -3315,9 +3315,9 @@ get_smb2_acl(struct cifs_sb_info *cifs_sb,
> struct cifs_ntsd *pntsd = NULL;
> struct cifsFileInfo *open_file = NULL;
>
> - if (inode)
> + if (inode && !(info & SACL_SECINFO))
> open_file = find_readable_file(CIFS_I(inode), true);
> - if (!open_file)
> + if (!open_file || (info & SACL_SECINFO))
> return get_smb2_acl_by_path(cifs_sb, path, pacllen, info);
>
> pntsd = get_smb2_acl_by_fid(cifs_sb, &open_file->fid, pacllen, info);
Why not have an if (info & SACL_SECINFO) return
get_smb2_acl_by_path... right at the beginning?
Looks cleaner that way IMO.
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 0aeb63694306..b207e1eb6803 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -3472,8 +3472,10 @@ SMB311_posix_query_info(const unsigned int xid, struct cifs_tcon *tcon,
> int
> SMB2_query_acl(const unsigned int xid, struct cifs_tcon *tcon,
> u64 persistent_fid, u64 volatile_fid,
> - void **data, u32 *plen, u32 additional_info)
> + void **data, u32 *plen, u32 extra_info)
> {
> + __u32 additional_info = OWNER_SECINFO | GROUP_SECINFO | DACL_SECINFO |
> + extra_info;
I still prefer having these flags set by the caller. That way, the
caller has the flexibility to query only the subset needed.
> *plen = 0;
>
> return query_info(xid, tcon, persistent_fid, volatile_fid,
> diff --git a/fs/cifs/xattr.c b/fs/cifs/xattr.c
> index 9318a2acf4ee..6b658a1172ef 100644
> --- a/fs/cifs/xattr.c
> +++ b/fs/cifs/xattr.c
> @@ -340,21 +340,19 @@ static int cifs_xattr_get(const struct xattr_handler *handler,
> * fetch owner, DACL, and SACL if asked for full descriptor,
> * fetch owner and DACL otherwise
> */
> - u32 acllen, additional_info = 0;
> + u32 acllen, extra_info;
> struct cifs_ntsd *pacl;
>
> if (pTcon->ses->server->ops->get_acl == NULL)
> goto out; /* rc already EOPNOTSUPP */
>
> if (handler->flags == XATTR_CIFS_NTSD_FULL) {
> - additional_info = OWNER_SECINFO | GROUP_SECINFO |
> - DACL_SECINFO | SACL_SECINFO;
> + extra_info = SACL_SECINFO;
> } else {
> - additional_info = OWNER_SECINFO | GROUP_SECINFO |
> - DACL_SECINFO;
> + extra_info = 0;
> }
> pacl = pTcon->ses->server->ops->get_acl(cifs_sb,
> - inode, full_path, &acllen, additional_info);
> + inode, full_path, &acllen, extra_info);
> if (IS_ERR(pacl)) {
> rc = PTR_ERR(pacl);
> cifs_dbg(VFS, "%s: error %zd getting sec desc\n",
> --
> 2.18.4
>
--
-Shyam
Powered by blists - more mailing lists