From 9eef19a24853bbb0ce800f92d4e372b7e5977fbf Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 5 Oct 2012 15:48:07 -0700 Subject: [PATCH 1/2] Modify ensure_canon_entry_valid() into ensure_canon_entry_valid_on_set() - makes the logic clearer. (cherry picked from commit 47ebc8fbc93ee1eb9640d9ca30275fcfc3b50026) Conflicts: source3/smbd/posix_acls.c Signed-off-by: Michael Adam --- source3/smbd/posix_acls.c | 311 +++++++++++++++++++++------------------------ 1 file changed, 148 insertions(+), 163 deletions(-) diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c index 4e93fef..b64faff 100644 --- a/source3/smbd/posix_acls.c +++ b/source3/smbd/posix_acls.c @@ -1404,40 +1404,38 @@ static bool ensure_canon_entry_valid_on_get(connection_struct *conn, } /**************************************************************************** - A well formed POSIX file or default ACL has at least 3 entries, a + A well formed POSIX file or default ACL has at least 3 entries, a SMB_ACL_USER_OBJ, SMB_ACL_GROUP_OBJ, SMB_ACL_OTHER_OBJ. In addition, the owner must always have at least read access. - When using this call on get_acl, the pst struct is valid and contains - the mode of the file. When using this call on set_acl, the pst struct has + When using this call on set_acl, the pst struct has been modified to have a mode containing the default for this file or directory type. ****************************************************************************/ -static bool ensure_canon_entry_valid(connection_struct *conn, +static bool ensure_canon_entry_valid_on_set(connection_struct *conn, canon_ace **pp_ace, bool is_default_acl, const struct share_params *params, const bool is_directory, const struct dom_sid *pfile_owner_sid, const struct dom_sid *pfile_grp_sid, - const SMB_STRUCT_STAT *pst, - bool setting_acl) + const SMB_STRUCT_STAT *pst) { canon_ace *pace; canon_ace *pace_user = NULL; canon_ace *pace_group = NULL; canon_ace *pace_other = NULL; + bool got_duplicate_user = false; + bool got_duplicate_group = false; for (pace = *pp_ace; pace; pace = pace->next) { if (pace->type == SMB_ACL_USER_OBJ) { - if (setting_acl) { - /* - * Ensure we have default parameters for the - * user (owner) even on default ACLs. - */ - apply_default_perms(params, is_directory, pace, S_IRUSR); - } + /* + * Ensure we have default parameters for the + * user (owner) even on default ACLs. + */ + apply_default_perms(params, is_directory, pace, S_IRUSR); pace_user = pace; } else if (pace->type == SMB_ACL_GROUP_OBJ) { @@ -1446,7 +1444,7 @@ static bool ensure_canon_entry_valid(connection_struct *conn, * Ensure create mask/force create mode is respected on set. */ - if (setting_acl && !is_default_acl) { + if (!is_default_acl) { apply_default_perms(params, is_directory, pace, S_IRGRP); } pace_group = pace; @@ -1457,7 +1455,7 @@ static bool ensure_canon_entry_valid(connection_struct *conn, * Ensure create mask/force create mode is respected on set. */ - if (setting_acl && !is_default_acl) { + if (!is_default_acl) { apply_default_perms(params, is_directory, pace, S_IROTH); } pace_other = pace; @@ -1468,16 +1466,18 @@ static bool ensure_canon_entry_valid(connection_struct *conn, * Ensure create mask/force create mode is respected on set. */ - if (setting_acl && !is_default_acl) { + if (!is_default_acl) { apply_default_perms(params, is_directory, pace, S_IRGRP); } } } if (!pace_user) { + canon_ace *pace_iter; + if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n")); - return False; + DEBUG(0,("talloc fail.\n")); + return false; } ZERO_STRUCTP(pace); @@ -1491,50 +1491,45 @@ static bool ensure_canon_entry_valid(connection_struct *conn, surprises for the user. */ pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRUSR, S_IWUSR, S_IXUSR); - if (setting_acl) { - /* See if the owning user is in any of the other groups in - the ACE, or if there's a matching user entry (by uid - or in the case of ID_TYPE_BOTH by SID). - If so, OR in the permissions from that entry. */ + /* See if the owning user is in any of the other groups in + the ACE, or if there's a matching user entry (by uid + or in the case of ID_TYPE_BOTH by SID). + If so, OR in the permissions from that entry. */ - canon_ace *pace_iter; - for (pace_iter = *pp_ace; pace_iter; pace_iter = pace_iter->next) { - if (pace_iter->type == SMB_ACL_USER && - pace_iter->unix_ug.id == pace->unix_ug.id) { + for (pace_iter = *pp_ace; pace_iter; pace_iter = pace_iter->next) { + if (pace_iter->type == SMB_ACL_USER && + pace_iter->unix_ug.id == pace->unix_ug.id) { + pace->perms |= pace_iter->perms; + } else if (pace_iter->type == SMB_ACL_GROUP_OBJ || pace_iter->type == SMB_ACL_GROUP) { + if (dom_sid_equal(&pace->trustee, &pace_iter->trustee)) { + pace->perms |= pace_iter->perms; + } else if (uid_entry_in_group(conn, pace, pace_iter)) { pace->perms |= pace_iter->perms; - } else if (pace_iter->type == SMB_ACL_GROUP_OBJ || pace_iter->type == SMB_ACL_GROUP) { - if (dom_sid_equal(&pace->trustee, &pace_iter->trustee)) { - pace->perms |= pace_iter->perms; - } else if (uid_entry_in_group(conn, pace, pace_iter)) { - pace->perms |= pace_iter->perms; - } } } + } - if (pace->perms == 0) { - /* If we only got an "everyone" perm, just use that. */ - if (pace_other) - pace->perms = pace_other->perms; - } - - /* - * Ensure we have default parameters for the - * user (owner) even on default ACLs. - */ - apply_default_perms(params, is_directory, pace, S_IRUSR); - } else { - pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRUSR, S_IWUSR, S_IXUSR); + if (pace->perms == 0) { + /* If we only got an "everyone" perm, just use that. */ + if (pace_other) + pace->perms = pace_other->perms; } + /* + * Ensure we have default parameters for the + * user (owner) even on default ACLs. + */ + apply_default_perms(params, is_directory, pace, S_IRUSR); + DLIST_ADD(*pp_ace, pace); pace_user = pace; } if (!pace_group) { if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n")); - return False; + DEBUG(0,("talloc fail.\n")); + return false; } ZERO_STRUCTP(pace); @@ -1544,17 +1539,15 @@ static bool ensure_canon_entry_valid(connection_struct *conn, pace->unix_ug.id = pst->st_ex_gid; pace->trustee = *pfile_grp_sid; pace->attr = ALLOW_ACE; - if (setting_acl) { - /* If we only got an "everyone" perm, just use that. */ - if (pace_other) - pace->perms = pace_other->perms; - else - pace->perms = 0; - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IRGRP); - } + + /* If we only got an "everyone" perm, just use that. */ + if (pace_other) { + pace->perms = pace_other->perms; } else { - pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IRGRP, S_IWGRP, S_IXGRP); + pace->perms = 0; + } + if (!is_default_acl) { + apply_default_perms(params, is_directory, pace, S_IRGRP); } DLIST_ADD(*pp_ace, pace); @@ -1563,8 +1556,8 @@ static bool ensure_canon_entry_valid(connection_struct *conn, if (!pace_other) { if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: malloc fail.\n")); - return False; + DEBUG(0,("talloc fail.\n")); + return false; } ZERO_STRUCTP(pace); @@ -1574,126 +1567,118 @@ static bool ensure_canon_entry_valid(connection_struct *conn, pace->unix_ug.id = -1; pace->trustee = global_sid_World; pace->attr = ALLOW_ACE; - if (setting_acl) { - pace->perms = 0; - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IROTH); - } - } else - pace->perms = unix_perms_to_acl_perms(pst->st_ex_mode, S_IROTH, S_IWOTH, S_IXOTH); + pace->perms = 0; + if (!is_default_acl) { + apply_default_perms(params, is_directory, pace, S_IROTH); + } DLIST_ADD(*pp_ace, pace); pace_other = pace; } - if (setting_acl) { - /* Ensure when setting a POSIX ACL, that the uid for a - SMB_ACL_USER_OBJ ACE (the owner ACE entry) has a duplicate - permission entry as an SMB_ACL_USER, and a gid for a - SMB_ACL_GROUP_OBJ ACE (the primary group ACE entry) also has - a duplicate permission entry as an SMB_ACL_GROUP. If not, - then if the ownership or group ownership of this file or - directory gets changed, the user or group can lose their - access. */ - bool got_duplicate_user = false; - bool got_duplicate_group = false; - - for (pace = *pp_ace; pace; pace = pace->next) { - if (pace->type == SMB_ACL_USER && - pace->unix_ug.id == pace_user->unix_ug.id) { - /* Already got one. */ - got_duplicate_user = true; - } else if (pace->type == SMB_ACL_GROUP && - pace->unix_ug.id == pace_group->unix_ug.id) { - /* Already got one. */ - got_duplicate_group = true; - } else if ((pace->type == SMB_ACL_GROUP) - && (dom_sid_equal(&pace->trustee, &pace_user->trustee))) { - /* If the SID owning the file appears - * in a group entry, then we have - * enough duplication, they will still - * have access */ - got_duplicate_user = true; - } - } - - /* If the SID is equal for the user and group that we need - to add the duplicate for, add only the group */ - if (!got_duplicate_user && !got_duplicate_group - && dom_sid_equal(&pace_group->trustee, - &pace_user->trustee)) { - /* Add a duplicate SMB_ACL_GROUP entry, this - * will cover the owning SID as well, as it - * will always be mapped to both a uid and - * gid. */ - - if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n")); - return false; - } - - ZERO_STRUCTP(pace); - pace->type = SMB_ACL_GROUP;; - pace->owner_type = GID_ACE; - pace->unix_ug.type = ID_TYPE_GID; - pace->unix_ug.id = pace_group->unix_ug.id; - pace->trustee = pace_group->trustee; - pace->attr = pace_group->attr; - pace->perms = pace_group->perms; - - DLIST_ADD(*pp_ace, pace); + /* Ensure when setting a POSIX ACL, that the uid for a + SMB_ACL_USER_OBJ ACE (the owner ACE entry) has a duplicate + permission entry as an SMB_ACL_USER, and a gid for a + SMB_ACL_GROUP_OBJ ACE (the primary group ACE entry) also has + a duplicate permission entry as an SMB_ACL_GROUP. If not, + then if the ownership or group ownership of this file or + directory gets changed, the user or group can lose their + access. */ - /* We're done here, make sure the - statements below are not executed. */ + for (pace = *pp_ace; pace; pace = pace->next) { + if (pace->type == SMB_ACL_USER && + pace->unix_ug.id == pace_user->unix_ug.id) { + /* Already got one. */ got_duplicate_user = true; + } else if (pace->type == SMB_ACL_GROUP && + pace->unix_ug.id == pace_user->unix_ug.id) { + /* Already got one. */ got_duplicate_group = true; + } else if ((pace->type == SMB_ACL_GROUP) + && (dom_sid_equal(&pace->trustee, &pace_user->trustee))) { + /* If the SID owning the file appears + * in a group entry, then we have + * enough duplication, they will still + * have access */ + got_duplicate_user = true; } + } - if (!got_duplicate_user) { - /* Add a duplicate SMB_ACL_USER entry. */ - if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n")); - return false; - } + /* If the SID is equal for the user and group that we need + to add the duplicate for, add only the group */ + if (!got_duplicate_user && !got_duplicate_group + && dom_sid_equal(&pace_group->trustee, + &pace_user->trustee)) { + /* Add a duplicate SMB_ACL_GROUP entry, this + * will cover the owning SID as well, as it + * will always be mapped to both a uid and + * gid. */ - ZERO_STRUCTP(pace); - pace->type = SMB_ACL_USER;; - pace->owner_type = UID_ACE; - pace->unix_ug.type = ID_TYPE_UID; - pace->unix_ug.id = pace_user->unix_ug.id; - pace->trustee = pace_user->trustee; - pace->attr = pace_user->attr; - pace->perms = pace_user->perms; + if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { + DEBUG(0,("talloc fail.\n")); + return false; + } - DLIST_ADD(*pp_ace, pace); + ZERO_STRUCTP(pace); + pace->type = SMB_ACL_GROUP;; + pace->owner_type = GID_ACE; + pace->unix_ug.type = ID_TYPE_GID; + pace->unix_ug.id = pace_group->unix_ug.id; + pace->trustee = pace_group->trustee; + pace->attr = pace_group->attr; + pace->perms = pace_group->perms; - got_duplicate_user = true; + DLIST_ADD(*pp_ace, pace); + + /* We're done here, make sure the + statements below are not executed. */ + got_duplicate_user = true; + got_duplicate_group = true; + } + + if (!got_duplicate_user) { + /* Add a duplicate SMB_ACL_USER entry. */ + if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { + DEBUG(0,("talloc fail.\n")); + return false; } - if (!got_duplicate_group) { - /* Add a duplicate SMB_ACL_GROUP entry. */ - if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { - DEBUG(0,("ensure_canon_entry_valid: talloc fail.\n")); - return false; - } + ZERO_STRUCTP(pace); + pace->type = SMB_ACL_USER;; + pace->owner_type = UID_ACE; + pace->unix_ug.type = ID_TYPE_UID; + pace->unix_ug.id = pace_user->unix_ug.id; + pace->trustee = pace_user->trustee; + pace->attr = pace_user->attr; + pace->perms = pace_user->perms; - ZERO_STRUCTP(pace); - pace->type = SMB_ACL_GROUP;; - pace->owner_type = GID_ACE; - pace->unix_ug.type = ID_TYPE_GID; - pace->unix_ug.id = pace_group->unix_ug.id; - pace->trustee = pace_group->trustee; - pace->attr = pace_group->attr; - pace->perms = pace_group->perms; + DLIST_ADD(*pp_ace, pace); - DLIST_ADD(*pp_ace, pace); + got_duplicate_user = true; + } - got_duplicate_group = true; + if (!got_duplicate_group) { + /* Add a duplicate SMB_ACL_GROUP entry. */ + if ((pace = talloc(talloc_tos(), canon_ace)) == NULL) { + DEBUG(0,("talloc fail.\n")); + return false; } + ZERO_STRUCTP(pace); + pace->type = SMB_ACL_GROUP;; + pace->owner_type = GID_ACE; + pace->unix_ug.type = ID_TYPE_GID; + pace->unix_ug.id = pace_group->unix_ug.id; + pace->trustee = pace_group->trustee; + pace->attr = pace_group->attr; + pace->perms = pace_group->perms; + + DLIST_ADD(*pp_ace, pace); + + got_duplicate_group = true; } - return True; + return true; } /**************************************************************************** @@ -2197,7 +2182,7 @@ static bool create_canon_ace_lists(files_struct *fsp, * the file ACL. If we don't have them, check if any SMB_ACL_USER/SMB_ACL_GROUP * entries can be converted to *_OBJ. Don't do this for the default * ACL, we will create them separately for this if needed inside - * ensure_canon_entry_valid(). + * ensure_canon_entry_valid_on_set(). */ if (file_ace) { check_owning_objs(file_ace, pfile_owner_sid, pfile_grp_sid); @@ -2599,8 +2584,8 @@ static bool unpack_canon_ace(files_struct *fsp, print_canon_ace_list( "file ace - before valid", file_ace); - if (!ensure_canon_entry_valid(fsp->conn, &file_ace, false, fsp->conn->params, - fsp->is_directory, pfile_owner_sid, pfile_grp_sid, pst, True)) { + if (!ensure_canon_entry_valid_on_set(fsp->conn, &file_ace, false, fsp->conn->params, + fsp->is_directory, pfile_owner_sid, pfile_grp_sid, pst)) { free_canon_ace_list(file_ace); free_canon_ace_list(dir_ace); return False; @@ -2608,8 +2593,8 @@ static bool unpack_canon_ace(files_struct *fsp, print_canon_ace_list( "dir ace - before valid", dir_ace); - if (dir_ace && !ensure_canon_entry_valid(fsp->conn, &dir_ace, true, fsp->conn->params, - fsp->is_directory, pfile_owner_sid, pfile_grp_sid, pst, True)) { + if (dir_ace && !ensure_canon_entry_valid_on_set(fsp->conn, &dir_ace, true, fsp->conn->params, + fsp->is_directory, pfile_owner_sid, pfile_grp_sid, pst)) { free_canon_ace_list(file_ace); free_canon_ace_list(dir_ace); return False; -- 1.7.9.5 From e0ae3f2965355d6b228bb4860333df0e6d69462d Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Wed, 5 Dec 2012 15:04:01 +0100 Subject: [PATCH 2/2] s3:smbd: don't apply create/directory mask and modes in apply_default_perms() The mask/mode parameters should only apply to a situation with only pure posix permissions. Once we are dealing with ACLs and inheritance, we need to do it correctly. Signed-off-by: Michael Adam Conflicts: source3/smbd/posix_acls.c --- source3/smbd/posix_acls.c | 89 ++++++--------------------------------------- 1 file changed, 11 insertions(+), 78 deletions(-) diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c index b64faff..e7e118d 100644 --- a/source3/smbd/posix_acls.c +++ b/source3/smbd/posix_acls.c @@ -1235,48 +1235,19 @@ NTSTATUS unpack_nt_owners(struct connection_struct *conn, return NT_STATUS_OK; } -/**************************************************************************** - Ensure the enforced permissions for this share apply. -****************************************************************************/ -static void apply_default_perms(const struct share_params *params, - const bool is_directory, canon_ace *pace, - mode_t type) +static void trim_ace_perms(canon_ace *pace) { - mode_t and_bits = (mode_t)0; - mode_t or_bits = (mode_t)0; - - /* Get the initial bits to apply. */ + pace->perms = pace->perms & (S_IXUSR|S_IWUSR|S_IRUSR); +} +static void ensure_minimal_owner_ace_perms(const bool is_directory, + canon_ace *pace) +{ + pace->perms |= S_IRUSR; if (is_directory) { - and_bits = lp_dir_mask(params->service); - or_bits = lp_force_dir_mode(params->service); - } else { - and_bits = lp_create_mask(params->service); - or_bits = lp_force_create_mode(params->service); - } - - /* Now bounce them into the S_USR space. */ - switch(type) { - case S_IRUSR: - /* Ensure owner has read access. */ - pace->perms |= S_IRUSR; - if (is_directory) - pace->perms |= (S_IWUSR|S_IXUSR); - and_bits = unix_perms_to_acl_perms(and_bits, S_IRUSR, S_IWUSR, S_IXUSR); - or_bits = unix_perms_to_acl_perms(or_bits, S_IRUSR, S_IWUSR, S_IXUSR); - break; - case S_IRGRP: - and_bits = unix_perms_to_acl_perms(and_bits, S_IRGRP, S_IWGRP, S_IXGRP); - or_bits = unix_perms_to_acl_perms(or_bits, S_IRGRP, S_IWGRP, S_IXGRP); - break; - case S_IROTH: - and_bits = unix_perms_to_acl_perms(and_bits, S_IROTH, S_IWOTH, S_IXOTH); - or_bits = unix_perms_to_acl_perms(or_bits, S_IROTH, S_IWOTH, S_IXOTH); - break; + pace->perms |= (S_IWUSR|S_IXUSR); } - - pace->perms = ((pace->perms & and_bits)|or_bits); } /**************************************************************************** @@ -1429,46 +1400,14 @@ static bool ensure_canon_entry_valid_on_set(connection_struct *conn, bool got_duplicate_group = false; for (pace = *pp_ace; pace; pace = pace->next) { + trim_ace_perms(pace); if (pace->type == SMB_ACL_USER_OBJ) { - - /* - * Ensure we have default parameters for the - * user (owner) even on default ACLs. - */ - apply_default_perms(params, is_directory, pace, S_IRUSR); + ensure_minimal_owner_ace_perms(is_directory, pace); pace_user = pace; - } else if (pace->type == SMB_ACL_GROUP_OBJ) { - - /* - * Ensure create mask/force create mode is respected on set. - */ - - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IRGRP); - } pace_group = pace; - } else if (pace->type == SMB_ACL_OTHER) { - - /* - * Ensure create mask/force create mode is respected on set. - */ - - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IROTH); - } pace_other = pace; - - } else if (pace->type == SMB_ACL_USER || pace->type == SMB_ACL_GROUP) { - - /* - * Ensure create mask/force create mode is respected on set. - */ - - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IRGRP); - } } } @@ -1520,7 +1459,7 @@ static bool ensure_canon_entry_valid_on_set(connection_struct *conn, * Ensure we have default parameters for the * user (owner) even on default ACLs. */ - apply_default_perms(params, is_directory, pace, S_IRUSR); + ensure_minimal_owner_ace_perms(is_directory, pace); DLIST_ADD(*pp_ace, pace); pace_user = pace; @@ -1546,9 +1485,6 @@ static bool ensure_canon_entry_valid_on_set(connection_struct *conn, } else { pace->perms = 0; } - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IRGRP); - } DLIST_ADD(*pp_ace, pace); pace_group = pace; @@ -1568,9 +1504,6 @@ static bool ensure_canon_entry_valid_on_set(connection_struct *conn, pace->trustee = global_sid_World; pace->attr = ALLOW_ACE; pace->perms = 0; - if (!is_default_acl) { - apply_default_perms(params, is_directory, pace, S_IROTH); - } DLIST_ADD(*pp_ace, pace); pace_other = pace; -- 1.7.9.5