share-level permissions and SYSTEM_SECURITY to files and dirs failing even though token has SeSecurityPrivilege

[Richard Sharpe-2]

Hi,

in smbd_check_access_rights we first check if the share permissions
allow the mode of access. This fails of the caller asked for
SYSTEM_SECURITY and no further check is made.

However, if the logged in user's token contains SeSecurityPrivilege
and rejected_share_access was only SYSTEM_SECURITY, then I believe
that the requested operation should be allowed to continue to further
checks.

What say ye?

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

[Jeremy Allison]

On Tue, May 15, 2012 at 03:38:37PM -0700, Richard Sharpe wrote:

> Hi,
>
> in smbd_check_access_rights we first check if the share permissions
> allow the mode of access. This fails of the caller asked for
> SYSTEM_SECURITY and no further check is made.
>
> However, if the logged in user's token contains SeSecurityPrivilege
> and rejected_share_access was only SYSTEM_SECURITY, then I believe
> that the requested operation should be allowed to continue to further
> checks.
>
> What say ye?

Errrr. Yeah. Sounds about right to me :-).

Jeremy.

[Richard Sharpe-2]

On Tue, May 15, 2012 at 5:11 PM, Jeremy Allison <[hidden email]> wrote:

> On Tue, May 15, 2012 at 03:38:37PM -0700, Richard Sharpe wrote:
>> Hi,
>>
>> in smbd_check_access_rights we first check if the share permissions
>> allow the mode of access. This fails of the caller asked for
>> SYSTEM_SECURITY and no further check is made.
>>
>> However, if the logged in user's token contains SeSecurityPrivilege
>> and rejected_share_access was only SYSTEM_SECURITY, then I believe
>> that the requested operation should be allowed to continue to further
>> checks.
>>
>> What say ye?
>
> Errrr. Yeah. Sounds about right to me :-).

OK, but in thinking about this some more, I think the correct fix is a
slight restructuring of the current code.

We currently have share_rejected_access, which is checked first up,
and we error out immediately if any bits are rejected at that point.

Rather than duplicate the privilege checks that are in
se_access_check, I would rather use those share_rejected_bits to feed
into rejected_mask and have se_access_check pull those in to see if
privileges override them ...

I guess I should look at how Windows handles this. The algorithm is
available, at least.

The issue came up because xcopy /X with a SACL does really weird
things if it cannot get access to the parent directory with
SYSTEM_SECURITY but can get access to the newly created file with that
access mode. (It stores the SACL but creates an empty DACL :-)

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

[Stefan (metze) Metzmacher]

Hi,

>>> in smbd_check_access_rights we first check if the share permissions
>>> allow the mode of access. This fails of the caller asked for
>>> SYSTEM_SECURITY and no further check is made.
>>>
>>> However, if the logged in user's token contains SeSecurityPrivilege
>>> and rejected_share_access was only SYSTEM_SECURITY, then I believe
>>> that the requested operation should be allowed to continue to further
>>> checks.
>>>
>>> What say ye?
>>
>> Errrr. Yeah. Sounds about right to me :-).
>
> OK, but in thinking about this some more, I think the correct fix is a
> slight restructuring of the current code.
>
> We currently have share_rejected_access, which is checked first up,
> and we error out immediately if any bits are rejected at that point.
>
> Rather than duplicate the privilege checks that are in
> se_access_check, I would rather use those share_rejected_bits to feed
> into rejected_mask and have se_access_check pull those in to see if
> privileges override them ...
>
> I guess I should look at how Windows handles this. The algorithm is
> available, at least.
>
> The issue came up because xcopy /X with a SACL does really weird
> things if it cannot get access to the parent directory with
> SYSTEM_SECURITY but can get access to the newly created file with that
> access mode. (It stores the SACL but creates an empty DACL :-)

I think we should also need to change the way we do our 'vuid' cache
based on the connection_struct.

We currently call SMB_VFS_CONNECT/SMB_VFS_DISCONNECT only
for the session that creates the tcon.
And we also do the share security_descriptor checking also only at
tcon time.

Which means that others sessions will use the wrong 'share_access' masks.

I think we should create a connection_struct for each vuid/tid combination
and call SMB_VFS_CONNECT/SMB_VFS_DISCONNECT when ever we
create a new connection_struct for a new vuid/tid combination.

metze

[Richard Sharpe-2]

On Wed, May 16, 2012 at 12:53 AM, Stefan (metze) Metzmacher
<[hidden email]> wrote:

> Hi,
>
>>>> in smbd_check_access_rights we first check if the share permissions
>>>> allow the mode of access. This fails of the caller asked for
>>>> SYSTEM_SECURITY and no further check is made.
>>>>
>>>> However, if the logged in user's token contains SeSecurityPrivilege
>>>> and rejected_share_access was only SYSTEM_SECURITY, then I believe
>>>> that the requested operation should be allowed to continue to further
>>>> checks.
>>>>
>>>> What say ye?
>>>
>>> Errrr. Yeah. Sounds about right to me :-).
>>
>> OK, but in thinking about this some more, I think the correct fix is a
>> slight restructuring of the current code.
>>
>> We currently have share_rejected_access, which is checked first up,
>> and we error out immediately if any bits are rejected at that point.
>>
>> Rather than duplicate the privilege checks that are in
>> se_access_check, I would rather use those share_rejected_bits to feed
>> into rejected_mask and have se_access_check pull those in to see if
>> privileges override them ...
>>
>> I guess I should look at how Windows handles this. The algorithm is
>> available, at least.
>>
>> The issue came up because xcopy /X with a SACL does really weird
>> things if it cannot get access to the parent directory with
>> SYSTEM_SECURITY but can get access to the newly created file with that
>> access mode. (It stores the SACL but creates an empty DACL :-)
>
> I think we should also need to change the way we do our 'vuid' cache
> based on the connection_struct.
>
> We currently call SMB_VFS_CONNECT/SMB_VFS_DISCONNECT only
> for the session that creates the tcon.
> And we also do the share security_descriptor checking also only at
> tcon time.
>
> Which means that others sessions will use the wrong 'share_access' masks.
>
> I think we should create a connection_struct for each vuid/tid combination
> and call SMB_VFS_CONNECT/SMB_VFS_DISCONNECT when ever we
> create a new connection_struct for a new vuid/tid combination.

Hmmm, that is something I had not noticed, but seem right now you mention it.

One other thing is that when you look at Share Permissions, you only
get a choice between Full Access, Read Access (I think) and None.
Today we use an Access Mask and compute MAXIMUM ALLOWED from the SD on
the share (but only for the first connecting user, as you say.)

Seems like I have two or three problems to solve then.

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