[PATCHES][BUG 13673] smbd: Fix DELETE_ON_CLOSE behaviour on files with READ_ONLY attribute

[Jeremy Allison]

On Fri, Nov 02, 2018 at 02:31:10PM -0700, Christof Schmitt wrote:
> On Fri, Nov 02, 2018 at 02:07:35PM -0700, Jeremy Allison wrote:
> > On Fri, Nov 02, 2018 at 02:01:11PM -0700, Christof Schmitt wrote:
> > > On Fri, Nov 02, 2018 at 01:53:55PM -0700, Jeremy Allison via samba-technical wrote:
> > > > On Fri, Nov 02, 2018 at 01:29:55PM -0700, Christof Schmitt via samba-technical wrote:
> > > > > smbd allowed CREATE on a file with the READONLY attribute set and
> > > > > DELETE_ON_CLOSE requested, even though MS-FSA states that this should be
> > > > > denied. On the other hand, SETINFO for DELETE_ON_CLOSE is correctly
> > > > > rejected.
> > > > > 
> > > > > See the attached patches for a fix. This also adds a smbtorture test for
> > > > > this case and the Samba override 'delete readonly=yes'
> > > > > 
> > > > > gitlab pipeline
> > > > > https://gitlab.com/samba-team/devel/samba/pipelines/35263504
> > > > 
> > > > Oh this is a great catch ! How did you find it ?
> > > 
> > > Long story. For a special case we had some testing done of trying to
> > > delete files with READONLY set. This showed differences between trying
> > > this from Powershell or cmd.exe. It turns out that this triggers two
> > > different patterns (CREATE then SETINFO-DELETE_ON_CLOSE vs.
> > > CREATE-DELET_ON_CLOSE). Samba allowing one while denying the other
> > > seemed inconsistent.
> > > 
> > > > 
> > > > Reviewing now...
> > > 
> > > Please hold on. This fails the base.delete.deltest12 test for Samba, but
> > > the same succeeds against a Windows server:
> > > 
> > > $ bin/smbtorture ... base.delete.deltest12 smb2.delete-on-close-perms.READONLY
> > > smbtorture 4.10.0pre1-DEVELOPERBUILD
> > > Using seed 1541192134
> > > time: 2018-11-02 20:55:34.630579
> > > test: deltest12
> > > time: 2018-11-02 20:55:34.630684
> > > time: 2018-11-02 20:55:37.730715
> > > success: deltest12
> > > time: 2018-11-02 20:55:37.739500
> > > test: READONLY
> > > time: 2018-11-02 20:55:37.741927
> > > Creating file with READ_ONLY attribute.
> > > Testing CREATE with DELETE_ON_CLOSE on READ_ONLY attribute file.
> > > Testing setting DELETE_ON_CLOSE disposition on  file with READONLY
> > > attribute.
> > > time: 2018-11-02 20:55:42.679877
> > > success: READONLY
> > > 
> > > I will look into this, maybe this is different in SMB1 vs. SMB2/3.
> > 
> > Thanks. Yes this very well could be different between SMB1 & SMB2
> > on Windows. Can you ensure the torture tests pass against recent
> > Windows then we'll implement that behavior in Samba.
> 
> The above smbtorture is against a Windows 2012R2, so these work against
> Windows. I think the issue is that base.delete.deltest12 is testing a
> CREATE on a new file while smb2.delete-on-close-perms.READONLY is
> testing against an existing file. My assumption now is that we need two
> checks to handle both cases
> can_set_delete_on_close(fsp, new_dos_attributes);
> and
> can_set_delete_on_close(fsp, existing_dos_attributes);

Yep, just came to that conclusion myself :-).

Something like this ?

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index d6359aac0c6..45ac33483d1 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -3732,8 +3732,13 @@ static NTSTATUS open_file_ntcreate(connection_struct *conn,
 
        /* Handle strange delete on close create semantics. */
        if (create_options & FILE_DELETE_ON_CLOSE) {
-
-               status = can_set_delete_on_close(fsp, new_dos_attributes);
+               if (info == FILE_WAS_CREATED) {
+                       status = can_set_delete_on_close(fsp,
+                                               new_dos_attributes);
+               } else {
+                       status = can_set_delete_on_close(fsp,
+                                               existing_dos_attributes);
+               }
 
                if (!NT_STATUS_IS_OK(status)) {
                        /* Remember to delete the mode we just added. */