Possibly incorrect handling of SeBackupPrivilege and SeRestorePrivilege

[Richard Sharpe]

On Wed, Feb 29, 2012 at 7:13 PM, Jeremy Allison <jra at samba.org> wrote:
> On Wed, Feb 29, 2012 at 04:15:57PM -0800, Richard Sharpe wrote:
>> On Wed, Feb 29, 2012 at 9:19 AM, Jeremy Allison <jra at samba.org> wrote:
>> > On Wed, Feb 29, 2012 at 08:36:21AM -0800, Richard Sharpe wrote:
>> >> 2012/2/29 Richard Sharpe <realrichardsharpe at gmail.com>:
>> >> > Hi,
>> >> >
>> >> > I believe that the actual Windows semantics around SeBackupPrivilege
>> >> > and SeRestorePrivilege is that if the requester opens a file with the
>> >> > BACKUP INTENT (FILE_OPEN_FOR_BACKUP_INTENT) flag in CreateOptions and
>> >> > they have those privileges and they have the correct access mode
>> >> > specified then they get to open the file if the ACL does not give them
>> >> > access.
>> >> >
>> >> > In looking at se_access_check we do not take into account
>> >> > FILE_OPEN_FOR_BACKUP_INTENT when checking those two privilege bits,
>> >> > which is wrong, I believe.
>> >> >
>> >> > The good news is that Samba works. The bad news is that Samba will
>> >> > give access in cases where Windows would not.
>> >>
>> >> If I get some agreement that there is a problem here I will file a bug
>> >> in bugzilla and create a patch. It is a small patch. I would pass the
>> >> CreateOptions (flag) along in the places where se_access_check is
>> >> eventually called and pass it into se_access_check ...
>> >
>> > I have some patches pending that implement the "correct"
>> > SeBackup and SeRestore semantics. But I'd still like to
>> > see your change to compare - if you can post it to the
>> > list !
>> >
>> > I'm not sure we should change se_access_check, but
>> > we might want to wrap it in cases where the user has
>> > privilege.
>>
>> Hi Jeremy,
>>
>> OK, I guess that the model is that any privileges that can be dealt
>> with in se_access_check with just the requests access mode should be
>> done there, and then we can deal with additional ones in callers above
>> se_access_check. Since se_access_check returns the access bits not
>> granted, this should be possible.
>>
>> I will code up a less invasive change than I first suggested and we
>> can compare notes.
>
> Ok, I just (with Volker's review) pushed a changeset to
> master that implements SeBackup in trans2 findfirst/findnext
> - please take a look and let me know what you think.

Yeah, I saw it come through on the mailing list. I had forgotten about
trans2 findfirst/findnext requiring the same treatment :-)

Anyway, I will look at it soon.

On the normal open path, it seems like we might have to pass
CreateOptions into samba3/smbd/open.c:smbd_check_access_rights and let
it check whether or not we can remove all the remaining bits if the
user has requested FILE_OPEN_FOR_BACKUP_INTENT and has the correct
privilege, but we would have to hoist the checks for SeBackupPrivilege
and SeRestorePrivilege out of se_access_check.

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)