summaryrefslogtreecommitdiff
path: root/source3/smbd
diff options
context:
space:
mode:
authorJeremy Allison <jra@samba.org>2009-01-22 10:57:10 -0800
committerJeremy Allison <jra@samba.org>2009-01-22 10:57:10 -0800
commit2d81c9e957a21191c5b4e2b28a4599052c1357a1 (patch)
tree0157cf898b106106f0d26a072d0e0358622cfea7 /source3/smbd
parent3942e53357146c9c5419313efc4c91b85f7e508b (diff)
downloadsamba-2d81c9e957a21191c5b4e2b28a4599052c1357a1.tar.gz
samba-2d81c9e957a21191c5b4e2b28a4599052c1357a1.tar.bz2
samba-2d81c9e957a21191c5b4e2b28a4599052c1357a1.zip
Another attempt to fix bug #4308 - Excel save operation corrupts file ACLs.
Simo is completely correct. We should be doing the chown *first*, and fail the ACL set if this fails. The long standing assumption I made when writing the initial POSIX ACL code was that Windows didn't control who could chown a file in the same was as POSIX. In POSIX only root can do this whereas I wasn't sure who could do this in Windows at the time (I didn't understand the privilege model). So the assumption was that setting the ACL was more important (early tests showed many failed ACL set's due to inability to chown). But now we have privileges in smbd, and we must always fail an ACL set when we can't chown first. The key that Simo noticed is that the CREATOR_OWNER bits in the ACL incoming are relative to the *new* owner, not the old one. This is why the old user owner disappears on ACL set - their access was set via the USER_OBJ in the creator POSIX ACL and when the ownership changes they lose their access. Patch is simple - just ensure we do the chown first before evaluating the incoming ACL re-read the owners. We already have code to do this it just wasn't rigorously being applied. Jeremy.
Diffstat (limited to 'source3/smbd')
-rw-r--r--source3/smbd/posix_acls.c29
1 files changed, 4 insertions, 25 deletions
diff --git a/source3/smbd/posix_acls.c b/source3/smbd/posix_acls.c
index 8fe7a9abe9..951046c562 100644
--- a/source3/smbd/posix_acls.c
+++ b/source3/smbd/posix_acls.c
@@ -3428,7 +3428,6 @@ NTSTATUS set_nt_acl(files_struct *fsp, uint32 security_info_sent, const SEC_DESC
NTSTATUS status;
uid_t orig_uid;
gid_t orig_gid;
- bool need_chown = False;
DEBUG(10,("set_nt_acl: called for file %s\n", fsp->fsp_name ));
@@ -3464,14 +3463,12 @@ NTSTATUS set_nt_acl(files_struct *fsp, uint32 security_info_sent, const SEC_DESC
}
/*
- * Do we need to chown ?
+ * Do we need to chown ? If so this must be done first as the incoming
+ * CREATOR_OWNER acl will be relative to the *new* owner, not the old.
+ * Noticed by Simo.
*/
if (((user != (uid_t)-1) && (orig_uid != user)) || (( grp != (gid_t)-1) && (orig_gid != grp))) {
- need_chown = True;
- }
-
- if (need_chown && (user == (uid_t)-1 || user == current_user.ut.uid)) {
DEBUG(3,("set_nt_acl: chown %s. uid = %u, gid = %u.\n",
fsp->fsp_name, (unsigned int)user, (unsigned int)grp ));
@@ -3511,9 +3508,6 @@ NTSTATUS set_nt_acl(files_struct *fsp, uint32 security_info_sent, const SEC_DESC
orig_mode = sbuf.st_mode;
orig_uid = sbuf.st_uid;
orig_gid = sbuf.st_gid;
-
- /* We did chown already, drop the flag */
- need_chown = False;
}
create_file_sids(&sbuf, &file_owner_sid, &file_grp_sid);
@@ -3664,24 +3658,9 @@ NTSTATUS set_nt_acl(files_struct *fsp, uint32 security_info_sent, const SEC_DESC
}
free_canon_ace_list(file_ace_list);
- free_canon_ace_list(dir_ace_list);
+ free_canon_ace_list(dir_ace_list);
}
- /* Any chown pending? */
- if (need_chown) {
- DEBUG(3,("set_nt_acl: chown %s. uid = %u, gid = %u.\n",
- fsp->fsp_name, (unsigned int)user, (unsigned int)grp ));
-
- if(try_chown( fsp->conn, fsp->fsp_name, user, grp) == -1) {
- DEBUG(3,("set_nt_acl: chown %s, %u, %u failed. Error = %s.\n",
- fsp->fsp_name, (unsigned int)user, (unsigned int)grp, strerror(errno) ));
- if (errno == EPERM) {
- return NT_STATUS_INVALID_OWNER;
- }
- return map_nt_error_from_unix(errno);
- }
- }
-
return NT_STATUS_OK;
}