Bug 15648 - check_parent_access_fsp() doesn't grant owner sid
Summary: check_parent_access_fsp() doesn't grant owner sid
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.15.13
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-24 03:11 UTC by Mike Liu
Modified: 2024-06-07 18:31 UTC (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Liu 2024-05-24 03:11:36 UTC
Given the following ACL, onwer fails to create subfolder with ACCESS_DENIED.

# /usr/local/samba/bin/smbcacls -U mike%mikeliu123 //localhost/test /
REVISION:1
CONTROL:SR|DP
OWNER:MIKE-X85U\mike
GROUP:MIKE-X85U\administrators
ACL:Owner Rights:ALLOWED/OI|CI/FULL

# /usr/local/samba/bin/smbclient -U  mike%mikeliu123 //localhost/test -c 'mkdir haha'
NT_STATUS_ACCESS_DENIED making remote directory \haha

-----

The problem is triggered by:

* mkdir_internal() calls check_parent_access_fsp()

* SMB_VFS_FGET_NT_ACL(fsp, SECINFO_DACL, frame, &parent_sd) only fetchs SECINFO_DAL 
  and passes parent_sd to se_file_access_check()

* se_access_check() will not check for explicit owner rights because sd->owner_sid is NULL.


Should we grant owner sid to check for explicit owner rights ?

NTSTATUS check_parent_access_fsp(struct files_struct *fsp,
				 uint32_t access_mask)
{
    /* ... */
	
	status = SMB_VFS_FGET_NT_ACL(fsp,
+				SECINFO_OWNER | SECINFO_DACL,
-				SECINFO_DACL,
				frame,
				&parent_sd);
				
	/* ... */			
}
Comment 1 Jeremy Allison 2024-05-24 17:30:32 UTC
Just checking, changing:

SECINFO_DACL -> SECINFO_OWNER | SECINFO_DACL

fixes this for you, correct ? If so, looks an obviously correct fix to me, but we'll need a regression test. Thanks !
Comment 2 Stefan Metzmacher 2024-05-24 18:07:38 UTC
(In reply to Jeremy Allison from comment #1)

While there I'd also add SECINFO_GROUP...
Comment 3 Jeremy Allison 2024-05-24 18:18:05 UTC
> While there I'd also add SECINFO_GROUP

Yep of course !
Comment 4 Mike Liu 2024-05-27 01:00:35 UTC
> Just checking, changing:
> SECINFO_DACL -> SECINFO_OWNER | SECINFO_DACL
> fixes this for you, correct ?
Correct!
Comment 6 Samba QA Contact 2024-06-07 18:31:05 UTC
This bug was referenced in samba master:

a9b3522f53aa2e6151cf83f1eeb65e3adea2b1d0