Bug 7716 - acl_xattr and acl_tdb modules don't store unmodified copies of security descriptors
acl_xattr and acl_tdb modules don't store unmodified copies of security descr...
Status: RESOLVED FIXED
Product: Samba 3.5
Classification: Unclassified
Component: File services
unspecified
All All
: P3 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
: 7733 7734 (view as bug list)
Depends on:
Blocks: 7812
  Show dependency treegraph
 
Reported: 2010-10-07 16:22 UTC by Jeremy Allison
Modified: 2010-11-24 11:15 UTC (History)
0 users

See Also:


Attachments
git-am patch for 3.5.next (2.21 KB, patch)
2010-10-07 16:29 UTC, Jeremy Allison
vl: review+
Details
ACL jumbo patch for 3.5.7. tgz of git-am format patch list. (9.87 KB, application/octect-stream)
2010-11-01 13:59 UTC, Jeremy Allison
no flags Details
git-am tgz of new jumbo patch. (9.94 KB, application/octet-stream)
2010-11-17 18:12 UTC, Jeremy Allison
vl: review+
Details
git am jumbo patch (10.09 KB, application/octet-stream)
2010-11-18 18:29 UTC, Jeremy Allison
vl: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Allison 2010-10-07 16:22:45 UTC
As pointed out by an OEM, the code within smbd/posix_acl.c, even though passed a const pointer to a security descriptor, still modifies the ACE entries within it (which are not const pointers).

This means ACLs stored in the extended attribute by the acl_xattr module have already been modified by the POSIX acl layer, and are not the original intent of storing the "unmodified" ACL from the client.

Simple patch for 3.5.next to follow.

Jeremy.
Comment 1 Jeremy Allison 2010-10-07 16:29:28 UTC
Created attachment 6000 [details]
git-am patch for 3.5.next

Volker please check and re-assign to Karolin if you're ok with it.
Jeremy.
Comment 2 Volker Lendecke 2010-10-08 01:55:36 UTC
Comment on attachment 6000 [details]
git-am patch for 3.5.next

Looks simple enough.
Comment 3 Jeremy Allison 2010-11-01 13:59:20 UTC
Created attachment 6043 [details]
ACL jumbo patch for 3.5.7. tgz of git-am format patch list.

Karolin, here is the ACL jumbo patch for 3.5.7. I'm going to use this bug as the placeholder for this patch (as requested).

Jeremy.
Comment 4 Jeremy Allison 2010-11-01 14:16:04 UTC
*** Bug 7733 has been marked as a duplicate of this bug. ***
Comment 5 Jeremy Allison 2010-11-01 14:16:30 UTC
*** Bug 7734 has been marked as a duplicate of this bug. ***
Comment 6 Simo Sorce 2010-11-01 15:07:16 UTC
Jeremy please do not mark as patch when it is a tgz.
Comment 7 Karolin Seeger 2010-11-11 05:08:11 UTC
Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 8 Jeremy Allison 2010-11-11 11:28:51 UTC
Karolin - the patches in the ACL jumbo patch for 3.5.7 attachment also need to be applied before we close this one out.

That's the tgz attachment 6043 [details] on this bug.

Thanks,

Jeremy.


Comment 9 Karolin Seeger 2010-11-16 08:26:09 UTC
Volker, is your review flag also valid for the jumbo patch?
Comment 10 Jeremy Allison 2010-11-17 18:12:13 UTC
Created attachment 6070 [details]
git-am tgz of new jumbo patch.

This is a new version of the jumbo patch for 3.5.x that applies on top of the first patch that Karolin has already applied. This contains one extra fix discovered in testing at an OEM.

"commit ce27f98705f564dfb2f5c43fb061da9e89f1f3dc
Author: Jeremy Allison <jra@samba.org>
Date:   Wed Nov 17 15:58:15 2010 -0800

    Fix our privileges code to display privileges with the "high" 32-bit value set.
    
    SeSecurityPrivilege is the first LUID we have added that has a non-zero
    "high" value, ensure our LUID code correctly supports it.
    
    Jeremy."

Volker, please review then re-assign to Karolin once you're ok with it.

Jeremy.
Comment 11 Jeremy Allison 2010-11-18 18:29:03 UTC
Created attachment 6073 [details]
git am jumbo patch

Replace the SeSecurityPrivilege LUID with the correct value (low = 0x8, high = 0), otherwise - same as previous patch.
Jeremy.
Comment 12 Volker Lendecke 2010-11-23 00:54:29 UTC
Comment on attachment 6073 [details]
git am jumbo patch

Jeremy, when you upload a tar.gz file, please do not set "patch" in the type field. This makes bugzilla set the mime-type so that the browser will not download the file but display it as garbage. If you want it to be displayed directly, you could just upload the output of "git format-patch --stdout", which is semantically exactly the same as the .tar.gz, due to missing compression it is just a bit larger. If you insist on putting in individual patch files (why is that required in this case, git format-patch --stdout would also do it), then please set the mime type to application/octet-stream.

Thanks,

Volker
Comment 13 Volker Lendecke 2010-11-23 01:09:29 UTC
Karolin, sorry, I know this is confusing, but final versions of the Jumbo ACL patch keep flowing in on a frequent basis. They all look sane, what I have just acked looks very sane to me. I do not have tested all the test cases that Jeremy's OEM has tested (Jeremy, do you have the complete list somewhere?), so I can't really say this is the final word. But what I can say is that this patch is probably better than what we have in 3.5.6, so I would think it is worth being put into the next 3.5 release.

Volker
Comment 14 Jeremy Allison 2010-11-23 10:26:58 UTC
Sorry about the marking as a patch, I'll try and remember the next time not to do that for a .tgz file.

The difference between the last two versions of the patch are changing the SeSecurityPrivilege LUID from {8,0} to {0,8} (which is correct) so not much changed there.

The tests performed were passing the smbtorture4 RAW-ACL test, and also passing a test using ROBOCOPY at an OEM site (don't have details of the ACLs being copied in the second test, sorry).

Hope that helps clarify things.

Jeremy.
Comment 15 Jeremy Allison 2010-11-23 16:29:27 UTC
NB. Once the jumbo patch is applied we also need the patch in 

bug:https://bugzilla.samba.org/show_bug.cgi?id=7812

to complete the fixes for ACLs.

Jeremy.
Comment 16 Karolin Seeger 2010-11-24 11:14:09 UTC
(In reply to comment #11)
> Created an attachment (id=6073) [details]
> git am jumbo patch
> 
> Replace the SeSecurityPrivilege LUID with the correct value (low = 0x8, high =
> 0), otherwise - same as previous patch.
> Jeremy.
> 

Pushed to v3-5-test.
Closing out bug report.

Thanks!
Comment 17 Karolin Seeger 2010-11-24 11:15:56 UTC
(In reply to comment #15)
> NB. Once the jumbo patch is applied we also need the patch in 
> 
> bug:https://bugzilla.samba.org/show_bug.cgi?id=7812
> 
> to complete the fixes for ACLs.
> 
> Jeremy.
> 

Done.