I think I see the problem. On initial delete on close it's writing in the user token as stored in the global session db, rather than the user token it's using as associated with the current connection.

Yep. The code in 4.12.x is:

----------------------------------------------
       if (fsp->initial_delete_on_close &&
                        !is_delete_on_close_set(lck, fsp->name_hash)) {
                bool became_user = False;

                /* Initial delete on close was set and no one else
                 * wrote a real delete on close. */

                if (get_current_vuid(conn) != fsp->vuid) {
                        become_user_without_service(conn, fsp->vuid);
                        became_user = True;
                }
                fsp->delete_on_close = true;
                set_delete_on_close_lck(fsp, lck,
                                get_current_nttok(conn),
                                get_current_utok(conn));
                if (became_user) {
                        unbecome_user_without_service();
                }
        }
----------------------------------------------

The code in 4.13.x and above is:

----------------------------------------------

        if (fsp->fsp_flags.initial_delete_on_close &&
                        !is_delete_on_close_set(lck, fsp->name_hash)) {
                struct auth_session_info *session_info = NULL;

                /* Initial delete on close was set and no one else
                 * wrote a real delete on close. */

                status = smbXsrv_session_info_lookup(conn->sconn->client,
                                                     fsp->vuid,
                                                     &session_info);
                if (!NT_STATUS_IS_OK(status)) {
                        return NT_STATUS_INTERNAL_ERROR;
                }
                fsp->fsp_flags.delete_on_close = true;
                set_delete_on_close_lck(fsp, lck,
                                        session_info->security_token,
                                        session_info->unix_token);
        }
----------------------------------------------

which isn't taking into account any impersonation status
on the connection.

The bug is here:

commit 367c0d191083240ccf9a59f1dc196da2d8ba17e4
Author: Ralph Boehme <slow@samba.org>
Date:   Tue Apr 7 09:54:24 2020 +0200

    smbd: avoid become_user_without_service() in close_remove_share_mode()
    
    Here we called become_user_without_service() just in order to be able to fetch
    the nt_token and unix_token subsequently via
    get_current_[nt|u]tok(conn). The same can be achieved by fetching the
    session_info with smbXsrv_session_info_lookup().
    
    Signed-off-by: Ralph Boehme <slow@samba.org>
    Reviewed-by: Jeremy Allison <jra@samba.org>

which I reviewed :-(. Sadly the comment:

"The same can be achieved by fetching the session_info with smbXsrv_session_info_lookup()"

isn't true.

Ralph, you might want to look at this. I screwed up giving a +1 to your code in review, sorry :-).

Created attachment 16403 [details]
git-am fix for master.

Ralph, I'm asking for your review here just so you can take a look at what I'm planning to run through ci. I'm not planning to push this without trying to create a regression test first so we keep this one fixed.

I reproduced the reporters bug locally, and this patch fixes it.

As it's actually an OS-level EACCES error this one will be difficult to reproduce (as in the test environment we're always running as one user). I'm thinking about adding a hack to vfs_error_inject:unlinkat() which checks if the containing directory of the object being deleted has a stat.st_uid owner that is different from the (emulated) uid returned by the uid_wrapper.c code for the current process, and refuses the unlink with EACCES if so.

Let me know if you can think of a better way to reproduce under the test environment !

Comment on attachment 16403 [details]
git-am fix for master.

The thing I'd like a comment on is if the:

+		if (!ok) {
+			smb_panic("close_remove_share_mode: unable to "
+				"become user.\n");
 		}

semantics are correct, or if we should return an NT_STATUS_INTERNAL_ERROR here instead. I can't see a place where become_user_without_service() here should fail, but more eyes are always better :-).

You are probably correct Jeremy, there is however the problem that on 4.13.2 it works for myself ;)

(In reply to Jeremy Allison from comment #5)
Hm. The question is: why is fsp->vuid not set to the vuid of the forced user on the share. Seems that is the real bug here, but I may be missing something as I just started to wrap my head around this.

Doesn't work for me on vanilla 4.13.2, just tested.

(In reply to Jeremy Allison from comment #5)
Test idea sounds good.

Created attachment 16404 [details]
Alternative patch for master using fsp->conn->session_info

Can't we just use fsp->conn->session_info directly?

Fixes the issue in local testing.

(In reply to Ralph Böhme from comment #11)

Oh that's much more elegant - thanks ! +1 on that patch. Now I just need to write a regression test :-).

(In reply to Ralph Böhme from comment #8)

> Hm. The question is: why is fsp->vuid not set to the vuid of the forced user
> on the share. Seems that is the real bug here, but I may be missing something
> as I just started to wrap my head around this.

Looking into this, in open.c:

fsp->vuid = req->vuid, not conn->vuid.

req->vuid is set in smb2_glue.c as:

smbreq->vuid = req->session->global->session_wire_id

so it's storing the session_wire_id, not the mapped user on the connection.

Maybe this code:


 44         if (req->session != NULL) {
 45                 smbreq->vuid = req->session->global->session_wire_id;
 46         }
 47         if (req->tcon != NULL) {
 48                 smbreq->tid = req->tcon->compat->cnum;
 49                 smbreq->conn = req->tcon->compat;
 50         }

should be:

 44         if (req->session != NULL) {
 45                 smbreq->vuid = req->session->global->session_wire_id;
 46         }
 47         if (req->tcon != NULL) {
 48                 smbreq->tid = req->tcon->compat->cnum;
 49                 smbreq->conn = req->tcon->compat;
 ++                 smbreq->vuid = conn->vuid;
 50         }

instead. I think we don't notice as 'fsp->vuid' is rarely used (other than identity checks that 'req->vuid == fsp->vuid' which it is by definition on fsp creation).

But we'd need to look really closely at the implications of changing this, which I think is a bug for another day :-).

(In reply to Arkadiusz Miskiewicz from comment #9)

Arkadiusz, can you test Ralph's patch ? It works for me here in local testing (and for Ralph too).

Thanks,

Jeremy.

(In reply to Jeremy Allison from comment #13)

> But we'd need to look really closely at the implications of changing this,
> which I think is a bug for another day :-).

Following up on myself, I looked more closely, and we mustn't change this.

The reason is all other active uses of fsp->vuid are in:

change_to_user_and_service_by_fsp()

and

become_user_without_service_by_fsp()

Both of these functions go through the change_to_user_impersonate() function in uid.c, which calls check_user_ok() and does the 'force user/force group' dance to get the tokens correct. Phew.

This 'delete on close' case seems to be the only case where we were using fsp->vuid directly without going through change_to_user_impersonate, and Ralph's patch shows we can use the cached tokens here instead - we're in the case where no one wrote a delete on close token in the lock (the common case where we're the only opener and are doing a open+delete_on_close/close case to delete a file/directory).

My patch worked in local testing (not sure why it didn't work for original submitter) but Ralphs is much more elegant and simple. I'll work on the regression test on Monday and submit an MR once I'm done.

Note that earlier I was testing only vanilla/unpached 4.13.3 and 4.13.2.

Anyway currently tested patch from comment 11 with 4.13.3 and it works for me. I can delete files without a problem now.

Fix including test running at <https://gitlab.com/samba-team/samba/-/merge_requests/1772>.

Created attachment 16405 [details]
git-am fix for 4.13.next.

Cherry-picked from master.

Patch needs fix-up. Sorry for the premature click for review.

Created attachment 16406 [details]
git-am fix for 4.13.next.

Properly back ported and locally tested with:

make test TESTS=samba3.blackbox.force-user-unlink

(In reply to Jeremy Allison from comment #21)
Thanks for the backport! We also need this for 4.14, does this apply to 4.14 as well?

Created attachment 16408 [details]
git-am fix for 4.14.next.

Cherry-picked from master.

Reassigning to Karolin for inclusion in 4.13 and 4.14.

(In reply to Ralph Böhme from comment #24)
Pushed to autobuild-v4-{14,13}-test.

This bug was referenced in samba master:

c44dad3ac2eb36fc5eb5a9f80a9ef97183be26ef
f3f8fdfbf10f690bc8d972a13d6f74f1fb0fb375
aa1f09cda0a097617e34dd0a8b1b0acc7a37bca8
e06f86bbd93d024c70016e1adcf833db85742aca

This bug was referenced in samba v4-14-test:

d5a696fc8869b9ce65f063ce93b04d07b01c2b2a
4c9cf755eb2a9e9e257265c5bbef6113e7b2e8e3
4bfdc4eff93419a8608f163e6fdade7192be678e
fcc6a32e069ed20d1ec141f792481f03b57dbdb0

This bug was referenced in samba v4-13-test:

483c1dc818ec748daad85fd8e4f223d6edf22f60
35eddb388f248cb206518eb2843f9aaf1479bfeb
ba12f0c3ae02d002435ecbb32ac018f8eb821691
f6e5fe6f122f85dda46872045cbc4cb020b399b9

Closing out bug report.

Thanks!

This bug was referenced in samba v4-14-stable (Release samba-4.14.0rc2):

d5a696fc8869b9ce65f063ce93b04d07b01c2b2a
4c9cf755eb2a9e9e257265c5bbef6113e7b2e8e3
4bfdc4eff93419a8608f163e6fdade7192be678e
fcc6a32e069ed20d1ec141f792481f03b57dbdb0

This bug was referenced in samba v4-13-stable (Release samba-4.13.5):

483c1dc818ec748daad85fd8e4f223d6edf22f60
35eddb388f248cb206518eb2843f9aaf1479bfeb
ba12f0c3ae02d002435ecbb32ac018f8eb821691
f6e5fe6f122f85dda46872045cbc4cb020b399b9