[PATCH 1/2] cifs: Fix unix perm bits to cifsacl conversion for "other" bits.

* [PATCH 1/2] cifs: Fix unix perm bits to cifsacl conversion for "other" bits.
@ 2020-11-09 17:42 Shyam Prasad N
  2020-11-09 17:46 ` Shyam Prasad N
  0 siblings, 1 reply; 3+ messages in thread
From: Shyam Prasad N @ 2020-11-09 17:42 UTC (permalink / raw)
  To: Steve French, CIFS

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi Steve,

Here's the patch as per the discussion. I'm trying to maintain a
preferred order of ACEs as much as possible. I had to modify the
reverse conversion logic, since deny ACEs can appear in the middle of
the list now.

Tested thoroughly with many permission modes, conversions and reverse
conversions.

FYI, there's a sticky bit implementation which I've patched on top of
this fix. Will send that for review soon.

-- 
-Shyam

[-- Attachment #2: 0001-cifs-Fix-unix-perm-bits-to-cifsacl-conversion-for-ot.patch --]
[-- Type: application/octet-stream, Size: 16194 bytes --]

From 2088cce13522c59386ee1c7d355d2539ccd79db8 Mon Sep 17 00:00:00 2001
From: Shyam Prasad N <sprasad@microsoft.com>
Date: Mon, 17 Aug 2020 03:23:12 -0700
Subject: [PATCH] cifs: Fix unix perm bits to cifsacl conversion for "other"
 bits.

With the "cifsacl" mount option, the mode bits set on the file/dir
is converted to corresponding ACEs in DACL. However, only the
ALLOWED ACEs were being set for "owner" and "group" SIDs. Since
owner is a subset of group, and group is a subset of
everyone/world SID, in order to properly emulate unix perm groups,
we need to add DENIED ACEs. If we don't do that, "owner" and "group"
SIDs could get more access rights than they should. Which is what
was happening. This fixes it.

We try to keep the "preferred" order of ACEs, i.e. DENYs followed
by ALLOWs. However, for a small subset of cases we cannot
maintain the preferred order. In that case, we'll end up with the
DENY ACE for group after the ALLOW for the owner.

If owner SID == group SID, use the more restrictive
among the two perm bits and convert them to ACEs.

Also, for reverse mapping, i.e. to convert ACL to unix perm bits,
for the "others" bits, we needed to add the masked bits of the
owner and group masks to others mask.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/cifs/cifsacl.c   | 211 +++++++++++++++++++++++++++++---------------
 fs/cifs/cifsproto.h |   4 +-
 fs/cifs/inode.c     |  12 ++-
 3 files changed, 154 insertions(+), 73 deletions(-)

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index 23b21e943652..a828373c1eff 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -557,30 +557,37 @@ static void copy_sec_desc(const struct cifs_ntsd *pntsd,
    bits to set can be: S_IRWXU, S_IRWXG or S_IRWXO ie 00700 or 00070 or 00007
 */
 static void access_flags_to_mode(__le32 ace_flags, int type, umode_t *pmode,
-				 umode_t *pbits_to_set)
+				 umode_t *pdenied, umode_t mask)
 {
 	__u32 flags = le32_to_cpu(ace_flags);
-	/* the order of ACEs is important.  The canonical order is to begin with
-	   DENY entries followed by ALLOW, otherwise an allow entry could be
-	   encountered first, making the subsequent deny entry like "dead code"
-	   which would be superflous since Windows stops when a match is made
-	   for the operation you are trying to perform for your user */
-
-	/* For deny ACEs we change the mask so that subsequent allow access
-	   control entries do not turn on the bits we are denying */
+	/*
+	 * Do not assume "preferred" or "canonical" order.
+	 * The first DENY or ALLOW ACE which matches perfectly is
+	 * the permission to be used. Once allowed or denied, same
+	 * permission in later ACEs do not matter.
+	 */
+
+	/* If not already allowed, deny these bits */
 	if (type == ACCESS_DENIED) {
-		if (flags & GENERIC_ALL)
-			*pbits_to_set &= ~S_IRWXUGO;
-
-		if ((flags & GENERIC_WRITE) ||
-			((flags & FILE_WRITE_RIGHTS) == FILE_WRITE_RIGHTS))
-			*pbits_to_set &= ~S_IWUGO;
-		if ((flags & GENERIC_READ) ||
-			((flags & FILE_READ_RIGHTS) == FILE_READ_RIGHTS))
-			*pbits_to_set &= ~S_IRUGO;
-		if ((flags & GENERIC_EXECUTE) ||
-			((flags & FILE_EXEC_RIGHTS) == FILE_EXEC_RIGHTS))
-			*pbits_to_set &= ~S_IXUGO;
+		if (flags & GENERIC_ALL &&
+				!(*pmode & mask & 0777))
+			*pdenied |= mask & 0777;
+
+		if (((flags & GENERIC_WRITE) ||
+				((flags & FILE_WRITE_RIGHTS) == FILE_WRITE_RIGHTS)) &&
+				!(*pmode & mask & 0222))
+			*pdenied |= mask & 0222;
+
+		if (((flags & GENERIC_READ) ||
+				((flags & FILE_READ_RIGHTS) == FILE_READ_RIGHTS)) &&
+				!(*pmode & mask & 0444))
+			*pdenied |= mask & 0444;
+
+		if (((flags & GENERIC_EXECUTE) ||
+				((flags & FILE_EXEC_RIGHTS) == FILE_EXEC_RIGHTS)) &&
+				!(*pmode & mask & 0111))
+			*pdenied |= mask & 0111;
+
 		return;
 	} else if (type != ACCESS_ALLOWED) {
 		cifs_dbg(VFS, "unknown access control type %d\n", type);
@@ -588,20 +595,27 @@ static void access_flags_to_mode(__le32 ace_flags, int type, umode_t *pmode,
 	}
 	/* else ACCESS_ALLOWED type */
 
-	if (flags & GENERIC_ALL) {
-		*pmode |= (S_IRWXUGO & (*pbits_to_set));
+	if ((flags & GENERIC_ALL) &&
+			!(*pdenied & mask & 0777)) {
+		*pmode |= mask & 0777;
 		cifs_dbg(NOISY, "all perms\n");
 		return;
 	}
-	if ((flags & GENERIC_WRITE) ||
-			((flags & FILE_WRITE_RIGHTS) == FILE_WRITE_RIGHTS))
-		*pmode |= (S_IWUGO & (*pbits_to_set));
-	if ((flags & GENERIC_READ) ||
-			((flags & FILE_READ_RIGHTS) == FILE_READ_RIGHTS))
-		*pmode |= (S_IRUGO & (*pbits_to_set));
-	if ((flags & GENERIC_EXECUTE) ||
-			((flags & FILE_EXEC_RIGHTS) == FILE_EXEC_RIGHTS))
-		*pmode |= (S_IXUGO & (*pbits_to_set));
+
+	if (((flags & GENERIC_WRITE) ||
+			((flags & FILE_WRITE_RIGHTS) == FILE_WRITE_RIGHTS)) &&
+			!(*pdenied & mask & 0222))
+		*pmode |= mask & 0222;
+
+	if (((flags & GENERIC_READ) ||
+			((flags & FILE_READ_RIGHTS) == FILE_READ_RIGHTS)) &&
+			!(*pdenied & mask & 0444))
+		*pmode |= mask & 0444;
+
+	if (((flags & GENERIC_EXECUTE) ||
+			((flags & FILE_EXEC_RIGHTS) == FILE_EXEC_RIGHTS)) &&
+			!(*pdenied & mask & 0111))
+		*pmode |= mask & 0111;
 
 	cifs_dbg(NOISY, "access flags 0x%x mode now %04o\n", flags, *pmode);
 	return;
@@ -638,17 +652,19 @@ static void mode_to_access_flags(umode_t mode, umode_t bits_to_use,
 }
 
 static __u16 fill_ace_for_sid(struct cifs_ace *pntace,
-			const struct cifs_sid *psid, __u64 nmode, umode_t bits)
+			const struct cifs_sid *psid, __u64 nmode, umode_t bits, __u8 access_type)
 {
 	int i;
 	__u16 size = 0;
 	__u32 access_req = 0;
 
-	pntace->type = ACCESS_ALLOWED;
+	pntace->type = access_type;
 	pntace->flags = 0x0;
 	mode_to_access_flags(nmode, bits, &access_req);
-	if (!access_req)
+	if (access_type == ACCESS_ALLOWED && !access_req)
 		access_req = SET_MINIMUM_RIGHTS;
+	else if (access_type == ACCESS_DENIED)
+		access_req &= ~SET_MINIMUM_RIGHTS;
 	pntace->access_req = cpu_to_le32(access_req);
 
 	pntace->sid.revision = psid->revision;
@@ -701,6 +717,10 @@ static void dump_ace(struct cifs_ace *pace, char *end_of_acl)
 }
 #endif
 
+#define ACL_OWNER_MASK 0700
+#define ACL_GROUP_MASK 0770
+#define ACL_EVERYONE_MASK 0777
+
 static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl,
 		       struct cifs_sid *pownersid, struct cifs_sid *pgrpsid,
 		       struct cifs_fattr *fattr, bool mode_from_special_sid)
@@ -716,7 +736,7 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl,
 	if (!pdacl) {
 		/* no DACL in the security descriptor, set
 		   all the permissions for user/group/other */
-		fattr->cf_mode |= S_IRWXUGO;
+		fattr->cf_mode |= 0777;
 		return;
 	}
 
@@ -733,16 +753,14 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl,
 	/* reset rwx permissions for user/group/other.
 	   Also, if num_aces is 0 i.e. DACL has no ACEs,
 	   user/group/other have no permissions */
-	fattr->cf_mode &= ~(S_IRWXUGO);
+	fattr->cf_mode &= ~(0777);
 
 	acl_base = (char *)pdacl;
 	acl_size = sizeof(struct cifs_acl);
 
 	num_aces = le32_to_cpu(pdacl->num_aces);
 	if (num_aces > 0) {
-		umode_t user_mask = S_IRWXU;
-		umode_t group_mask = S_IRWXG;
-		umode_t other_mask = S_IRWXU | S_IRWXG | S_IRWXO;
+		umode_t denied_mode = 0;
 
 		if (num_aces > ULONG_MAX / sizeof(struct cifs_ace *))
 			return;
@@ -768,26 +786,28 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl,
 				fattr->cf_mode |=
 					le32_to_cpu(ppace[i]->sid.sub_auth[2]);
 				break;
-			} else if (compare_sids(&(ppace[i]->sid), pownersid) == 0)
-				access_flags_to_mode(ppace[i]->access_req,
-						     ppace[i]->type,
-						     &fattr->cf_mode,
-						     &user_mask);
-			else if (compare_sids(&(ppace[i]->sid), pgrpsid) == 0)
-				access_flags_to_mode(ppace[i]->access_req,
-						     ppace[i]->type,
-						     &fattr->cf_mode,
-						     &group_mask);
-			else if (compare_sids(&(ppace[i]->sid), &sid_everyone) == 0)
-				access_flags_to_mode(ppace[i]->access_req,
-						     ppace[i]->type,
-						     &fattr->cf_mode,
-						     &other_mask);
-			else if (compare_sids(&(ppace[i]->sid), &sid_authusers) == 0)
-				access_flags_to_mode(ppace[i]->access_req,
-						     ppace[i]->type,
-						     &fattr->cf_mode,
-						     &other_mask);
+			} else {
+				if (compare_sids(&(ppace[i]->sid), pownersid) == 0) {
+					access_flags_to_mode(ppace[i]->access_req,
+							ppace[i]->type,
+							&fattr->cf_mode,
+							&denied_mode,
+							ACL_OWNER_MASK);
+				} else if (compare_sids(&(ppace[i]->sid), pgrpsid) == 0) {
+					access_flags_to_mode(ppace[i]->access_req,
+							ppace[i]->type,
+							&fattr->cf_mode,
+							&denied_mode,
+							ACL_GROUP_MASK);
+				} else if ((compare_sids(&(ppace[i]->sid), &sid_everyone) == 0) ||
+						(compare_sids(&(ppace[i]->sid), &sid_authusers) == 0)) {
+					access_flags_to_mode(ppace[i]->access_req,
+							ppace[i]->type,
+							&fattr->cf_mode,
+							&denied_mode,
+							ACL_EVERYONE_MASK);
+				}
+			}
 
 
 /*			memcpy((void *)(&(cifscred->aces[i])),
@@ -873,11 +893,17 @@ unsigned int setup_special_user_owner_ACE(struct cifs_ace *pntace)
 }
 
 static int set_chmod_dacl(struct cifs_acl *pndacl, struct cifs_sid *pownersid,
-			struct cifs_sid *pgrpsid, __u64 nmode, bool modefromsid)
+			struct cifs_sid *pgrpsid, __u64 *pnmode, bool modefromsid)
 {
 	u16 size = 0;
 	u32 num_aces = 0;
 	struct cifs_acl *pnndacl;
+	__u64 nmode;
+	__u64 user_mode;
+	__u64 group_mode;
+	__u64 other_mode;
+	__u64 deny_user_mode = 0;
+	__u64 deny_group_mode = 0;
 
 	pnndacl = (struct cifs_acl *)((char *)pndacl + sizeof(struct cifs_acl));
 
@@ -887,18 +913,65 @@ static int set_chmod_dacl(struct cifs_acl *pndacl, struct cifs_sid *pownersid,
 
 		size += setup_special_mode_ACE(pntace, nmode);
 		num_aces++;
+		goto set_size;
 	}
 
+	/*
+	 * We'll try to keep the mode as requested by the user.
+	 * But in cases where we cannot meaningfully convert that
+	 * into ACL, return back the updated mode, so that it is
+	 * updated in the inode.
+	 */
+	nmode = *pnmode;
+
+	if (!memcmp(pownersid, pgrpsid, sizeof(struct cifs_sid))) {
+		/*
+		 * Case when owner and group SIDs are the same.
+		 * Set the more restrictive of the two modes.
+		 */
+		user_mode = nmode & (nmode << 3) & 0700;
+		group_mode = nmode & (nmode >> 3) & 0070;
+	} else {
+		user_mode = nmode & 0700;
+		group_mode = nmode & 0070;
+	}
+
+	other_mode = nmode & 0007;
+
+	/* We need DENY ACE when the perm is more restrictive than the next sets. */
+	deny_user_mode = ~(user_mode) & ((group_mode << 3) | (other_mode << 6)) & 0700;
+	deny_group_mode = ~(group_mode) & (other_mode << 3) & 0070;
+
+	*pnmode = user_mode | group_mode | other_mode | (nmode & ~0777);
+
+	if (deny_user_mode) {
+		size += fill_ace_for_sid((struct cifs_ace *)((char *)pnndacl + size),
+				pownersid, deny_user_mode, 0700, ACCESS_DENIED);
+		num_aces++;
+	}
+	/* Group DENY ACE does not conflict with owner ALLOW ACE. Keep in preferred order*/
+	if (deny_group_mode && !(deny_group_mode & (user_mode >> 3))) {
+		size += fill_ace_for_sid((struct cifs_ace *)((char *)pnndacl + size),
+				pgrpsid, deny_group_mode, 0070, ACCESS_DENIED);
+		num_aces++;
+	}
 	size += fill_ace_for_sid((struct cifs_ace *) ((char *)pnndacl + size),
-					pownersid, nmode, S_IRWXU);
+			pownersid, user_mode, 0700, ACCESS_ALLOWED);
 	num_aces++;
+	/* Group DENY ACE conflicts with owner ALLOW ACE. So keep it after. */
+	if (deny_group_mode && (deny_group_mode & (user_mode >> 3))) {
+		size += fill_ace_for_sid((struct cifs_ace *)((char *)pnndacl + size),
+				pgrpsid, deny_group_mode, 0070, ACCESS_DENIED);
+		num_aces++;
+	}
 	size += fill_ace_for_sid((struct cifs_ace *)((char *)pnndacl + size),
-					pgrpsid, nmode, S_IRWXG);
+			pgrpsid, group_mode, 0070, ACCESS_ALLOWED);
 	num_aces++;
 	size += fill_ace_for_sid((struct cifs_ace *)((char *)pnndacl + size),
-					 &sid_everyone, nmode, S_IRWXO);
+			&sid_everyone, other_mode, 0007, ACCESS_ALLOWED);
 	num_aces++;
 
+set_size:
 	pndacl->num_aces = cpu_to_le32(num_aces);
 	pndacl->size = cpu_to_le16(size + sizeof(struct cifs_acl));
 
@@ -1000,7 +1073,7 @@ static int parse_sec_desc(struct cifs_sb_info *cifs_sb,
 
 /* Convert permission bits from mode to equivalent CIFS ACL */
 static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
-	__u32 secdesclen, __u64 nmode, kuid_t uid, kgid_t gid,
+	__u32 secdesclen, __u64 *pnmode, kuid_t uid, kgid_t gid,
 	bool mode_from_sid, bool id_from_sid, int *aclflag)
 {
 	int rc = 0;
@@ -1012,7 +1085,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
 	struct cifs_acl *dacl_ptr = NULL;  /* no need for SACL ptr */
 	struct cifs_acl *ndacl_ptr = NULL; /* no need for SACL ptr */
 
-	if (nmode != NO_CHANGE_64) { /* chmod */
+	if (pnmode && *pnmode != NO_CHANGE_64) { /* chmod */
 		owner_sid_ptr = (struct cifs_sid *)((char *)pntsd +
 				le32_to_cpu(pntsd->osidoffset));
 		group_sid_ptr = (struct cifs_sid *)((char *)pntsd +
@@ -1026,7 +1099,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd,
 		ndacl_ptr->num_aces = 0;
 
 		rc = set_chmod_dacl(ndacl_ptr, owner_sid_ptr, group_sid_ptr,
-				    nmode, mode_from_sid);
+				    pnmode, mode_from_sid);
 		sidsoffset = ndacloffset + le16_to_cpu(ndacl_ptr->size);
 		/* copy sec desc control portion & owner and group sids */
 		copy_sec_desc(pntsd, pnntsd, sidsoffset);
@@ -1281,7 +1354,7 @@ cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb, struct cifs_fattr *fattr,
 
 /* Convert mode bits to an ACL so we can update the ACL on the server */
 int
-id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode,
+id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
 			kuid_t uid, kgid_t gid)
 {
 	int rc = 0;
@@ -1340,7 +1413,7 @@ id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 nmode,
 	else
 		id_from_sid = false;
 
-	rc = build_sec_desc(pntsd, pnntsd, secdesclen, nmode, uid, gid,
+	rc = build_sec_desc(pntsd, pnntsd, secdesclen, pnmode, uid, gid,
 			    mode_from_sid, id_from_sid, &aclflag);
 
 	cifs_dbg(NOISY, "build_sec_desc rc: %d\n", rc);
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 24c6f36177ba..2ed98d4a30c1 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -215,8 +215,8 @@ extern int cifs_acl_to_fattr(struct cifs_sb_info *cifs_sb,
 			      struct cifs_fattr *fattr, struct inode *inode,
 			      bool get_mode_from_special_sid,
 			      const char *path, const struct cifs_fid *pfid);
-extern int id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64,
-					kuid_t, kgid_t);
+extern int id_mode_to_cifs_acl(struct inode *inode, const char *path, __u64 *pnmode,
+					kuid_t uid, kgid_t gid);
 extern struct cifs_ntsd *get_cifs_acl(struct cifs_sb_info *, struct inode *,
 					const char *, u32 *);
 extern struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *,
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 9ee5f304592f..e06dd3ad906f 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -2812,7 +2812,8 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) ||
 	    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MODE_FROM_SID)) {
 		if (uid_valid(uid) || gid_valid(gid)) {
-			rc = id_mode_to_cifs_acl(inode, full_path, NO_CHANGE_64,
+			mode = NO_CHANGE_64;
+			rc = id_mode_to_cifs_acl(inode, full_path, &mode,
 							uid, gid);
 			if (rc) {
 				cifs_dbg(FYI, "%s: Setting id failed with error: %d\n",
@@ -2833,13 +2834,20 @@ cifs_setattr_nounix(struct dentry *direntry, struct iattr *attrs)
 		rc = 0;
 		if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_CIFS_ACL) ||
 		    (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MODE_FROM_SID)) {
-			rc = id_mode_to_cifs_acl(inode, full_path, mode,
+			rc = id_mode_to_cifs_acl(inode, full_path, &mode,
 						INVALID_UID, INVALID_GID);
 			if (rc) {
 				cifs_dbg(FYI, "%s: Setting ACL failed with error: %d\n",
 					 __func__, rc);
 				goto cifs_setattr_exit;
 			}
+
+			/*
+			 * In case of CIFS_MOUNT_CIFS_ACL, we cannot support all modes.
+			 * Pick up the actual mode bits that were set.
+			 */
+			if (mode != attrs->ia_mode)
+				attrs->ia_mode = mode;
 		} else
 		if (((mode & S_IWUGO) == 0) &&
 		    (cifsInode->cifsAttrs & ATTR_READONLY) == 0) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread