Bug 4308 - Excel save operation corrupts file ACLs
Summary: Excel save operation corrupts file ACLs
Status: RESOLVED FIXED
Alias: None
Product: Samba 3.0
Classification: Unclassified
Component: File Services (show other bugs)
Version: 3.0.23d
Hardware: All All
: P3 normal
Target Milestone: none
Assignee: Jeremy Allison
QA Contact: Samba QA Contact
URL:
Keywords:
: 4788 (view as bug list)
Depends on: 2346
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-23 13:53 UTC by SATOH Fumiyasu
Modified: 2009-02-18 06:33 UTC (History)
14 users (show)

See Also:


Attachments
Level 10 debug of "Excel save operation corrupts file ACLs" (454.26 KB, text/plain)
2007-06-25 09:06 UTC, Christoph Peus
no flags Details
Backport of the patch for 3.0.25c (10.83 KB, patch)
2007-07-12 16:53 UTC, Jeremy Allison
no flags Details
Samba ACL fix backport from 3.0.26 trunk. (18.15 KB, patch)
2007-07-13 06:48 UTC, Carwyn Edwards
no flags Details
Patch to 3.0.26a: Fix or improvement to the bug by adding a couple of security context switches. (1.03 KB, patch)
2007-10-30 10:52 UTC, Michael Adam
no flags Details
wireshark trace of excel save (166.69 KB, application/octet-stream)
2008-11-13 12:56 UTC, Bill Marshall
no flags Details
port 445 trace of 3 client connections opening & saving excel files (160.56 KB, application/x-gzip)
2008-11-19 15:16 UTC, Bill Marshall
no flags Details
3 level 10 traces of excel open & saves against 3.2.4 samba server (630.00 KB, application/octet-stream)
2008-11-19 15:17 UTC, Bill Marshall
no flags Details
Patch (2.09 KB, patch)
2009-01-22 12:31 UTC, Jeremy Allison
no flags Details
Patch (1.86 KB, patch)
2009-01-22 12:42 UTC, Jeremy Allison
no flags Details
trace of excel save from 2 clients to a win2k server (443.14 KB, application/octet-stream)
2009-01-22 15:57 UTC, Bill Marshall
no flags Details
Second part of patch for 3.3.0 and master (10.89 KB, patch)
2009-01-22 16:16 UTC, Jeremy Allison
no flags Details
Second part of patch for 3-2-test. (10.85 KB, patch)
2009-01-22 16:17 UTC, Jeremy Allison
no flags Details
Second part of patch for 3.0.x. (10.57 KB, patch)
2009-01-22 16:30 UTC, Jeremy Allison
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description SATOH Fumiyasu 2006-12-23 13:53:09 UTC
I'm facing a problem same as Bug 2346 with Samba 3.0.23d. Please reopen Bug 2346.
Comment 1 Gerald (Jerry) Carter (dead mail address) 2007-01-11 10:53:07 UTC
Please add you own details.  Often symptoms can appear the same but
are actually different bugs.
Comment 2 Christoph Peus 2007-06-25 09:06:18 UTC
Created attachment 2780 [details]
Level 10 debug of  "Excel save operation corrupts file ACLs"

Switched debug level to 10 immediately before the save operation, switched back to 2 afterwards. The saved file is called "Kasse2007.xls".
Comment 3 Christoph Peus 2007-06-25 09:21:26 UTC
The same here. The symptoms are essentially as described in bugzilla 2346:
(Excel file is write protected after writing if the user who is writing the file is not the owner of the file.)

Permissions before:
-rwxrw----  1 kloehmann  tedi 161792 2007-06-25 15:44 Kasse2007.xls

Permissions after:
-r--rwx---+ 1 marions    tedi 161792 2007-06-25 15:44 Kasse2007.xls

Obviously the ownership changes as expected, but the new owner has no write permission anymore.

I've attached a level 10 debug log of the save operation. 

Details:
samba 3.0.24 with ACL support
filesystem: xfs with ACL support
kernel: 2.6.21.5, 2.6.19.x (tested both)
smb.conf:

[global]
        workgroup = UWH
        realm = UNI-WH.DE
        netbios aliases = Fileserver
        server string = Central File/Printserver UWH
        security = ADS
        password server = 10.0.0.4
        log level = 0 vfs:10
        syslog = 0
        log file = /var/log/samba/%m.%U
        max log size = 1024
        name resolve order = host wins
        printcap name = cups
        mangle prefix = 6
        preferred master = No
        local master = No
        wins server = 10.0.0.4
        ldap ssl = no
        idmap uid = 1000-60000
        idmap gid = 1000-60000
        template homedir = /home/%U
        template shell = /bin/bash
        winbind enum users = Yes
        winbind enum groups = Yes
        winbind use default domain = Yes
        comment = Central File/Printserver UWH
        read only = No
        create mask = 0770
        directory mask = 0770
        hosts deny = 10.7.4.
        veto files = /.recycle/

[tedi]
        comment = Tedi O - Platte
        path = /export/admin/tedi
Comment 4 Gerald (Jerry) Carter (dead mail address) 2007-06-25 09:22:33 UTC
Jeremy, Any ideas ?
Comment 5 Jeremy Allison 2007-06-25 14:51:37 UTC
Excel is creating the tmp file 44049100, and then setting a 2 element ACL on it that consists of :

user: marions
group: Domain Users

acl[0]: kloehmann : rw-
acl[1]: tedi : rw-

Note that it's not setting any permissions for the owner or primary group. This is strange. I'd like to see a network sniff of this running against a Windows server. Looks like somehow it's expecting inherited permissions to take care of access for the owner. There are no inherited permissions set for us so we end up with a owner acl as "r--" (which we silently add).

I'm sure this can be fixed with acl inheritence set correctly, but I'm puzzled as to why Excel isn't adding any permissions for the owner.

Jeremy.
Comment 6 Jeremy Allison 2007-06-25 16:53:12 UTC
I need to know what exactly version of Excel (patch level) on exactly what platform you're seeing this. I'm not seeing it with Office 2003 SP2.
Jeremy.
Comment 7 Jeremy Allison 2007-06-25 19:55:12 UTC
I've been doing experiments on this and I never see Excel set an ACL for the file owner. It's very strange. I'd like to see some more details from your setup (who is in what group etc.) as well as more details on what versions of office are being used.
Jeremy.
Comment 8 Jeremy Allison 2007-06-25 20:39:18 UTC
I've noticed these bugs are being reported against 3.0.23d. Can you please test 
with the latest 3.0.25 release ? There have been some changes here and I can't reproduce the problem here.

The interesting thing is that Excel is initially sending an ACL with the "security information" field set to 0x7 == (OWNER_SECURITY_INFORMATION|GROUP_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION)
- in other words it's arbitrarily trying to set the owner and group SID. This should also fail against a Windows server, else it's a massive quota hole (possibly security too). I'm going to do some more experiments....

Jeremy.

Comment 9 Christoph Peus 2007-06-26 04:19:03 UTC
Platform: Windows XP Pro SP2 
Excel 2002 10.6823.6825 SP3  (Office XP)

group memberships:
uid=7025(marions) gid=10645(domänen-benutzer) groups=1001(nawi),7001(tedi),10645(domänen-benutzer),10672(mitarbeiter)

uid=50253(kloehmann) gid=10645(domänen-benutzer) groups=1001(nawi),7001(tedi),10645(domänen-benutzer),10672(mitarbeiter)

BTW: There's something wrong with the mapping of gid 1001 to "nawi", but I don't know how to solve this. I've asked for a solution here: http://lists.samba.org/archive/samba/2007-March/130568.html

Testing this with 3.0.25 is difficult for me, because up to now there's no samba 3.0.25 package availabe for our distribution (gentoo), and I would like to avoid  installing a manually configured vanilla samba on this machine. (I will do it though, if it's unavoidable.)
Thanks for your support!

Christoph
Comment 10 Christoph Peus 2007-06-26 07:13:20 UTC
I don't know if it matters... anyhow - the directories permissions:

drwxrws---  2 lindemeier tedi    148 2007-06-25 15:44 .
Comment 11 Jeremy Allison 2007-06-28 20:12:08 UTC
I'm working on some new code to fix this for 3.0.26. I now know more about Excel and ACLs than I ever wanted to know :-).

As a short term fix setting : "map acl inherit = yes" should fix this.

Jeremy.
Comment 12 Jeremy Allison 2007-06-29 16:55:37 UTC
I committed a fix for this - svn rev. 23663
in SAMBA_3_0 and SAMBA_3_0_26. Should be in 3.0.26.
Based on information in :

http://www.codeproject.com/win32/accessctrl2.asp

Search for "Q. How does Inheritance come into this?"
for details.

Please test this if you can.
Jeremy.
Comment 13 Carwyn Edwards 2007-07-06 08:31:50 UTC
I've just back ported the posix_acl.c fixes in SAMBA_3_0_26 to 3.0.25 and it does indeed fix the problem.

samba-3.0.25a-3.fc7 on Fedora 7.
Comment 14 Jeremy Allison 2007-07-06 12:12:03 UTC
Thanks - can you attach your patch for 3.0.25 as an attachment here ? I'll then commit it to the SAMBA_3_0_25 repository in case we have to do a 3.0.25c.

Thanks,

Jeremy.
Comment 15 Jeremy Allison 2007-07-12 16:53:14 UTC
Created attachment 2814 [details]
Backport of the patch for 3.0.25c

This should be the patch for 3.0.25c. Please test.
Jeremy.
Comment 16 Carwyn Edwards 2007-07-13 06:45:22 UTC
(In reply to comment #15)
> This should be the patch for 3.0.25c. Please test.
> Jeremy.
> 

Have you missed the i -> j fix you found with valgrind?

--- branches/SAMBA_3_0_26/source/smbd/posix_acls.c	2007/06/29 17:57:05	23663
+++ branches/SAMBA_3_0_26/source/smbd/posix_acls.c	2007/06/30 00:22:59	23664
@@ -3263,7 +3263,7 @@
 
 	/* Finally append any inherited ACEs. */
 	for (j = 0; j < parent_sd->dacl->num_aces; j++) {
-		SEC_ACE *se = &parent_sd->dacl->aces[i];
+		SEC_ACE *se = &parent_sd->dacl->aces[j];
 		uint32 i_flags = se->flags & (SEC_ACE_FLAG_OBJECT_INHERIT|
 					SEC_ACE_FLAG_CONTAINER_INHERIT|
 					SEC_ACE_FLAG_INHERIT_ONLY);


I basically took the changes from posix_acls.c r22651 to r23664 to make the patch. Doing this I also had to modify vfs_default.c though due to the return status changes. Maybe what I should have done though is back out the changes in posix_acls.c r23620 instead though.

I've attached the patch I used. For information only, do not use!

Running with if for a few days now it's certainly fixed the excel problem and no noticed regressions yet.
Comment 17 Carwyn Edwards 2007-07-13 06:48:06 UTC
Created attachment 2818 [details]
Samba ACL fix backport from 3.0.26 trunk.

The backported patch I used, there is almost certainly more than required in this.

Note the chunk at the end that applies to the default_vfs file.
Comment 18 Jeremy Allison 2007-07-13 11:53:46 UTC
Thanks for that note (and pointing out what I'd missed :-). I'm not going to change the VFS interface for 3.0.25c, that's too big a change for a minor letter release, so I'm going to have to wait and see if the failure to return the correct error on owner change is ignored by the client and it continues correctly. I'm waiting on bmarsh @ IBM to confirm that this is fixed with the patch already applied (plus the i -> j fix :-).
Thanks,
Jeremy.
Comment 19 Jeremy Allison 2007-07-16 13:50:40 UTC
*** Bug 4788 has been marked as a duplicate of this bug. ***
Comment 20 Bill Marshall 2007-07-18 10:22:56 UTC
I took the 3.0.25b source and patched it, but we're still having errors in my test environment.

 getfacl  /home/group/xlstest/Book2.xls
getfacl: Removing leading '/' from absolute path names
# file: home/group/xlstest/Book2.xls
# owner: wrm3
# group: domain\040users
user::r-x
user:wrm3:rwx
group::r-x
group:statsp_access:rwx
mask::rwx
other::r--

Do you need a log or trace?
Comment 21 Jeremy Allison 2007-07-18 13:36:11 UTC
Ok I just tried to reproduce your problem. I had a clean 3.0.25b source tree, applied the attachment "Backport of the patch for 3.0.25c" and built and ran it.
Opened an xls file I had only access to via a u:jra:rw ACL entry, changed it and saved. It re-wrote under my name but correctly kept the previous ACL.

Original acl :

# file: ent_dm_nb702_b.xls
# owner: griffin
# group: games
user::rw-
user:jra:rw-
group::rw-
mask::rw-
other::---

Modified ACL after save (from user jra)

# owner: jra
# group: eng
user::rw-
user:griffin:rw-
group::rw-
group:games:rw-
mask::rwx
other::rw-

So this works correctly for me. More info on previous and subsequent ACL's please ? Also your settings in smb.conf.

Comment 22 Bill Marshall 2007-07-23 10:39:03 UTC
I was thinking this was working ok... and then I noticed permission changes on my server for group and other. Now I see in your example:
  mask::rw-
  other::---
becomes
  mask::rwx
  other::rw-
Comment 23 Jeremy Allison 2007-07-24 03:59:04 UTC
The mask will always change to rwx, that's what the Samba POSIX ACL code always sets it to if there are more than the 3 standard entries (and it doesn't affect permissions anyway). The "other" change is due to the fact that this is a new file that's created, and then the "extra" permissions of the old file are added on top. The "other" permission will be controlled by the setting of the "create mask" "force create mode" parameters, as this is a new file being created by Excel. It's not going to blindly copy the "other" permission from the initial file, as it's creating new, not modifying old.
Jeremy.
Comment 24 Bill Marshall 2007-07-26 16:18:28 UTC
Here's what I'm seeing. We generally grant group::x and other::x for traversing purposes. (In this case I didn't grant other::x)

# getfacl .
# file: .
# owner: root
# group: root
user::rwx
group::--x
group:ltest:rwx
mask::rwx
other::---
default:user::rwx
default:group::--x
default:group:ltest:rwx
default:mask::rwx
default:other::---

Create a new file in excel and save it. I don't want all domain users to have access and the ACL will do that for me.

[root@rchs5bld xlstest]# getfacl Book2.xls
# file: Book2.xls
# owner: bmarsh
# group: domain\040users
user::rwx
group::--x
group:ltest:rwx
mask::rwx
other::---

Make a minor change and save the file again. Group:: and other:: now have read

[root@rchs5bld xlstest]# getfacl Book2.xls
# file: Book2.xls
# owner: bmarsh
# group: domain\040users
user::rwx
group::r-x
group:ltest:rwx
mask::rwx
other::r--

Comment 25 Bill Marshall 2007-07-26 16:23:45 UTC
You said - "It's not going to blindly copy the "other" permission from the initial file, as it's creating new, not modifying old."

But... as far as the user is concerned, this is not a new file. If I didn't want Jerry to read the payroll info this morning, if I updated the contents, I still don't want him to read the payroll info.

Under windows I experimented with a similar setup. Domain users have full control on the directory and on the default acl.
K:\bmarsh>xcacls .
K:\bmarsh BUILTIN\Administrators:(OI)(CI)F
          CREATOR OWNER:(OI)(CI)(IO)F
          RCHDNT\Domain Users:(OI)(CI)F
          NT AUTHORITY\SYSTEM:(OI)(CI)F
          BUILTIN\Users:(CI)(special access:)
                            SYNCHRONIZE
                            FILE_WRITE_DATA
                            FILE_APPEND_DATA

          BUILTIN\Users:(OI)(CI)R

But if I remove domain users from my acl (via a copy of the acl from the parent dir and turning off inheritance) it stays this way. Even as I edit the file (and under the covers excel makes new files -- but does the guy in finance care about under the covers?)
K:\bmarsh>xcacls secretfile.xls
K:\bmarsh\secretfile.xls BUILTIN\Administrators:F
                         NT AUTHORITY\SYSTEM:F
Comment 26 Jeremy Allison 2007-07-27 04:52:37 UTC
For new files, restrict the default "other" permissions using "create mask" in the smb.conf.
Jeremy.

Comment 27 Bill Marshall 2007-07-31 17:33:46 UTC
(In reply to comment #26)
> For new files, restrict the default "other" permissions using "create mask" in
> the smb.conf

create mask doesn't work well if I have various subdirectories that have different permissions. For example if I have public and private directories under the same share. Under public\ I want "domain users" to have read and under private I don't.

This worked on samba-3.0.20b

[root@rchs10dc xlstest]# getfacl .
# file: .
# owner: root
# group: root
user::rwx
group::--x
group:ltest:rwx
mask::rwx
other::---
default:user::rwx
default:group::--x
default:group:ltest:rwx
default:mask::rwx
default:other::---

Create new file, save file, update file, save file.

[root@rchs10dc xlstest]# getfacl Book9.xls
# file: Book9.xls
# owner: bmarsh
# group: Domain Users
user::rwx
group::--x
group:ltest:rwx
mask::rwx
other::---

Comment 28 Patrick Selka 2007-09-11 02:09:08 UTC
Same problem here - Samba 3.0.25c SerNet SLES10-64 from www.enterprisesamba.org.
Saving an Excel file that I am not owner of corrupts file ACLs. This is a serious problem for us beacuse we have many .xls files on our network share...
map acl inherit = yes in smb.conf does not resolve the problem.
Is there a way to fix this bug or to get a workaround?
Comment 29 Patrick Selka 2007-09-11 03:32:24 UTC
(In reply to comment #28)
> Same problem here - Samba 3.0.25c SerNet SLES10-64 from
> www.enterprisesamba.org.
> Saving an Excel file that I am not owner of corrupts file ACLs. This is a
> serious problem for us beacuse we have many .xls files on our network share...
> map acl inherit = yes in smb.conf does not resolve the problem.
> Is there a way to fix this bug or to get a workaround?
> 

Here is an example...:

The ACLs before opening and saving it again:
# file: test.xls
# owner: rothhoeft
# group: vk_m_w
user::rwx
user:himrich:r-x
group::---
group:vk_m_w:rwx
group:itadmins:rwx
mask::rwx
other::---


After opening this file as user "selka" (which is member of group "itadmins"):
# file: test.xls
# owner: rothhoeft
# group: vk_m_w
user::r--
user:himrich:r-x
user:rothhoeft:rwx
group::rwx
group:itadmins:rwx
mask::rwx
other::---

The group right "vk_m_w" is removed completely and the owning user right is set to "r--", so it is write protected for the owning user!!!
Comment 30 Patrick Selka 2007-09-13 08:41:50 UTC
With 3.0.26a the bug seems to be resolved for Excel 2000 on Windows 2000.
But with Excel 2003 (latest updates) on Windows XP (latest updates) it is still there.

Testing with the RPMs from SerNet down to 3.0.23c shows the same problems than 3.0.25c.

Please let me know if I can test anything else.
Comment 31 Yannick Bergeron (mail address dead) 2007-09-20 09:44:30 UTC
similar problem

clients are experiencing read-only excel files when actually nobody has it open according to smbstatus

our current solution is to create a copy of the file, delete the original and then renaming the copy to the original name

I'll check on the next client call if the file has the read-only flag.

samba 3.0.25
Microsoft Excel 2002 (10.6501.6714) SP3
Windows XP SP2
map acl inherit = no

what should we try first? map acl inherit = yes? samba 3.0.26a?
Comment 32 Michael Adam 2007-10-30 10:52:56 UTC
Created attachment 2957 [details]
Patch to 3.0.26a: Fix or improvement to the bug by adding a couple of security context switches.

I found a couple of missing become_root/unbecome_root blocks that resulted in broken permissions with LDAP passdb backend.

These fixes have gone upstream in two parts:
r25598 (svn) and 
0f633851537b689b0ac57b041d97115b6158546f (git branch v3-0-test),
016795c550ee0b78fa46c508703fb5e1e40d8f36 (git branch v3-2-test)
Comment 33 Michael Adam 2007-11-18 05:01:43 UTC
Any feedback on the effect of the fixes mentioned in comment #32?

Thanks - Michael
Comment 34 Tim Bell 2007-12-04 15:57:55 UTC
We were seeing similar problems as reported in Comment #29 (i.e. the file becoming read-only for the owner) when using 3.0.25b with the "Backport of the patch for 3.0.25c" patch (id=2814) applied. The patch (id=2957) from Comment #32 seems to have fixed that problem.

Thanks for all the hard work.

Tim.
Comment 35 Michael Adam 2007-12-04 17:04:48 UTC
Thanks for the feedback, Tim.

Any more comments? Can we close the bug?

Michael
Comment 36 Jeremy Allison 2008-01-24 20:24:18 UTC
These patches were not enough. The bug is *really* subtle :-). The good news is I think it's fixed in the 3.0-test and 3.2-test git trees now. Jim, please git-pull and test.
Jeremy.
Comment 37 Jeremy Allison 2008-03-06 12:51:49 UTC
*** Bug 5306 has been marked as a duplicate of this bug. ***
Comment 38 Jeremy Allison 2008-03-06 12:52:26 UTC
Fixed in 3.0.28a and above.
Jeremy.
Comment 39 Jeremy Allison 2008-11-12 20:38:35 UTC
Re-opening. The fix committed here breaks inheritance in the Windows ACL editor. Need some other way to fix this.
Jeremy.
Comment 40 Jeremy Allison 2008-11-12 20:46:37 UTC
Bill Marshall, what I need from you is a capture trace of Excel doing a "save" operation onto a Windows 2000 share. I need the full capture from connect to the share to save operation completing. I'm hoping this will allow me to determine the bits I need to return in get_nt_acl that will trigger the proper client code inheritance path.
Jeremy.
Comment 41 Bill Marshall 2008-11-13 12:56:00 UTC
Created attachment 3733 [details]
wireshark trace of excel save

Excel 2002 file save from an XP client. to a Microsoft Windows 2000 [Version 5.00.2195] server.
Trace includes the "net use" connection. I created a new excel file. Saved it, then added text and saved it again.
Comment 42 Jeremy Allison 2008-11-13 14:39:30 UTC
Thanks Bill, I'm in San Diego for the LISA conference for the rest of this week but I'll look at this asap.

Have you been able to try saving an Excel file onto a 3.2 server with the "temporary" patch in bug #5873 

https://bugzilla.samba.org/attachment.cgi?id=3730

to see if the ACL problem with Excel & Samba re-appears (I'm guessing it will, but need confirmation) ?

Thanks,

Jeremy.
Comment 43 Bill Marshall 2008-11-18 14:35:56 UTC
I pulled samba 3.2.4 and complied it w/ the acl stuff:
   HAVE_SYS_ACL_H
   HAVE_ACL_LIBACL_H
   HAVE_POSIX_ACLS
and tried writing to an excel w/ excel 2002 on my system and excel xp on a co-workers. I would loose access to the file when we went back and forth on writes.

Initial:
getfacl *
# file: Book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:bmarsh:rwx
user:greed:rwx
group::---
mask::rwx
other::---

co-worker Gary saves it

[root@rchs4bld excel]# getfacl *
# file: Book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:bmarsh:rwx
group::---
mask::rwx
other::---

bmarsh saves it (and looses access)
[root@rchs4bld excel]# getfacl *
# file: Book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:greed:rwx
group::---
mask::rwx
other::---

So I'm not sure if I should go ahead w/ the patch.. or if we'll learn anything since it is already busted.
Comment 44 Jeremy Allison 2008-11-18 16:44:43 UTC
Any chance I can get a debug level 10 log of the save that loses the permissions, and also a wireshark trace (a wireshark trace of the same activities against a Windows server would be ideal) ? Does this happen on all versions of excel or just on a specific version ?

This used to work, so I'm interested in what regressed.

Jeremy.
Comment 45 Bill Marshall 2008-11-19 15:10:33 UTC
Jeremy - we did a lot of testing.. bu didn't find anything simple. For example, using ID wrm3 from my PC seemed to work just fine. But using bmarsh from my PC there were problems. (Doesn't make sense) Neither user is an "admin user" on the share. I was signed onto my box as bmarsh both times, I just switched users on the net use. We also tried to narrow this down to a version of excel, but that didn't seem to matter.

The attachment "wireshark trace of excel save" is against a windows server. I'll upload the level 10 logs and the tcpdump from the samba server that goes with the scenario shown below.

Samba version 3.2.4
Username      Group         Machine
-----------------------------------------------------------------
wrm3    domain users  tigger2  (::ffff:9.10.65.57)  XP client, office 2003
greed   domain users  tigger7  (::ffff:9.10.65.91) vista, office 2002 sp3
bmarsh  domain users  bmarshxp (::ffff:9.10.67.194) XP client, office 2002 sp3

The ACL seems to come from the default acl, not from the acl that WAS on the file.

[root@rchs4bld excel]# getfacl .  (get the ACL of the directory)
# file: .
# owner: wrm3
# group: domain\040users
user::rwx
user:lbmarsh:rwx
user:wrm3:rwx
user:bmarsh:rwx
user:greed:rwx
group::---
mask::rwx
other::---
default:user::rwx
default:user:lbmarsh:rwx
default:user:wrm3:rwx
default:user:bmarsh:rwx
default:user:greed:rwx
default:group::---
default:mask::rwx
default:other::---


[root@rchs4bld excel]# getfacl book1.xls  (initial ACL -- missing bmarsh)
# file: book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
group::---
mask::rwx
other::---

opened & saved it as wrm3 on XP office 2003. owner moved to wrm3

[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: wrm3
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:bmarsh:rwx
user:greed:rwx
group::---
mask::rwx
other::---

opened & saved it as bmarsh on XP,  office 2002 sp3. Errored when trying to open the file after the save.



[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: wrm3
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
user:greed:rwx
group::---
mask::rwx
other::---

opened it as greed on vista,  office 2002 sp3

[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
user:bmarsh:rwx		(bmarsh comes back)
group::---
mask::rwx
other::---

remove bmarsh from the default acl

[root@rchs4bld excel]# setfacl -x d:u:bmarsh .
[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
user:bmarsh:rwx
group::---
mask::rwx
other::---

greed opens & saves it again  (bmarsh falls off the ACL)

[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
user:greed:rwx
group::---
mask::rwx
other::---

opened it as bmarsh on XP,  office 2002 sp3

opened it as greed on vista,  office 2002 sp3

[root@rchs4bld excel]# getfacl book1.xls
# file: book1.xls
# owner: greed
# group: domain\040users
user::rwx
user:root:rwx
user:lbmarsh:rwx
user:wrm3:rwx
group::---
mask::rwx
other::---
Comment 46 Bill Marshall 2008-11-19 15:16:17 UTC
Created attachment 3750 [details]
port 445 trace of 3 client connections opening & saving excel files

samba server IP address - 9.10.227.73
Comment 47 Bill Marshall 2008-11-19 15:17:49 UTC
Created attachment 3751 [details]
3 level 10 traces of excel open & saves against 3.2.4 samba server

level 10 traces for jra
Comment 48 Simo Sorce 2009-01-22 09:31:07 UTC
Bill, Jeremy,
I think I know what may be going on.

If you look at lob.bmarshxp at time 2008/11/19 14:52:32, we see that we set an ACL for a file named "41215B00" (This is the temp file used by excel I believe).

Well there set the ACL properly (line 26416):
[2008/11/19 14:52:32, 10] modules/vfs_posixacl.c:posixacl_sys_acl_set_file(89)
  Calling acl_set_file: 41215B00, 0

Unfortunately we never print the Posix ACL in debug statements (we should I think), but because the temp file is created by bmarsh (See open_create at 14:52:31), I *believe* it must have bmarsh as the owner.

Now right after we set the ACL properly for some misterious reason we do a chown. And we chown the file to wm3:

line 26418:
[2008/11/19 14:52:32,  3] smbd/posix_acls.c:set_nt_acl(3670)
  set_nt_acl: chown 41215B00. uid = 1000, gid = 1000.

And I think here is where we loose access because we basically *change* the owner from bmarsh -> wrm3 and therefore we completely loose bmarsh which is present only as owner in the posix ACL.

I think mixing chown and posix ACLs is the real problem here, we should never do that IMO. Or if we really need do, then we should do it *before* we set the ACL, so that we do not loose the previous owner.


In fact in the code, we actually can do chown *before* setting the ACL, but we must be failing this condition (source3/smbd/posix_acls.c, line: 3474):

        if (need_chown && (user == (uid_t)-1 || user == current_user.ut.uid)) {

If we fail this then we do the chown after, which is wrong in this case.

HTH
Comment 49 Jeremy Allison 2009-01-22 12:31:21 UTC
Created attachment 3882 [details]
Patch

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.

Patch for 3.3.0 and 3.2.x is attached.

Jeremy.
Comment 50 Jeremy Allison 2009-01-22 12:42:37 UTC
Created attachment 3883 [details]
Patch

Patch for 3.0.x.
Jeremy.
Comment 51 Bill Marshall 2009-01-22 15:57:03 UTC
Created attachment 3884 [details]
trace of excel save from 2 clients to a win2k server

This is a trace of 2 different users writing an excel file to a windows 2000 server. I did this trace so simo could see any specific packets related to updating the file ownership information.
In the current patch from jra, the file ownership does not change when a different person writes the file.
Comment 52 Jeremy Allison 2009-01-22 16:16:36 UTC
Created attachment 3885 [details]
Second part of patch for 3.3.0 and master

Ok bmarsh pointed out that once the chown is successful, the ACL set also must be. Ensure this is so by always doing the ACL set as root, if the chown succeeded.
Patch for 3.3.0 and master.
Jeremy.
Comment 53 Jeremy Allison 2009-01-22 16:17:36 UTC
Created attachment 3886 [details]
Second part of patch for 3-2-test.

Second part of patch for 3-2-test.
Jeremy.
Comment 54 Jeremy Allison 2009-01-22 16:30:26 UTC
Created attachment 3887 [details]
Second part of patch for 3.0.x.
Comment 55 Bill Marshall 2009-01-26 12:08:02 UTC
I tested with a RHEL 5 test package from simo and we're unable to recreate the error w/ Excel. Also, a while ago I had started testing the ACL functions from a windows client using xcopy /o. xcopy /o also seems to be able to create the ACLs fine.
Comment 56 Jeremy Allison 2009-01-26 21:37:19 UTC
My god, does this mean that this evil bug is *finally* dead ? :-).
Jeremy.
Comment 57 SATOH Fumiyasu 2009-02-18 06:29:14 UTC
We confirmed this bug was gone.
We tested with Samba 3.2.8 on CentOS 5 and
Samba 3.2.8 plus zfsacl.so on Solaris 10 + ZFS (i.e. NFSv4 ACL).

Thank you very much!
Comment 58 Karolin Seeger 2009-02-18 06:33:28 UTC
Closing out bug report.