Bug 14617 - smbd tries to delete files with wrong permissions (uses guest instead of user from force user =)
Summary: smbd tries to delete files with wrong permissions (uses guest instead of user...
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.13.3
Hardware: All All
: P5 regression (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-22 22:02 UTC by Arkadiusz Miskiewicz
Modified: 2021-02-04 08:26 UTC (History)
3 users (show)

See Also:


Attachments
git-am fix for master. (3.59 KB, patch)
2021-01-23 03:24 UTC, Jeremy Allison
no flags Details
Alternative patch for master using fsp->conn->session_info (2.37 KB, patch)
2021-01-23 17:53 UTC, Ralph Böhme
jra: review+
Details
git-am fix for 4.13.next. (9.94 KB, patch)
2021-01-26 17:41 UTC, Jeremy Allison
no flags Details
git-am fix for 4.13.next. (9.79 KB, patch)
2021-01-26 17:57 UTC, Jeremy Allison
slow: review+
Details
git-am fix for 4.14.next. (9.90 KB, patch)
2021-01-26 20:52 UTC, Jeremy Allison
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arkadiusz Miskiewicz 2021-01-22 22:02:09 UTC
Samba installed on Linux os.

There is a directory with files owner by 1000:1000 user.
Samba has share with this dir with force user = someuser (which is uid 1000, gid 1000).

Deleting files from guest smb client works fine with 4.12.6 but fails after upgrade to 4.13.3. Downgrading to 4.12.6 and things work again.

The other report which looks to be the same issue:
https://bbs.archlinux.org/viewtopic.php?id=259893
https://lists.samba.org/archive/samba/2020-December/233588.html

   security = user
   map to guest = Bad Password

[storage]
        path = /mnt/storage-md2/public
        public = yes
        writable = yes
        force user = someuser

log from 4.13.3, it drops uid/gid to 99/99 and then tries to unlink:

  close_remove_share_mode: file S/aaa.file. Delete on close was set and unlink failed with error Permission denied

and indeed 
451513 getgroups(14, [3, 4, 7, 10, 11, 17, 23, 24, 27, 28, 78, 84, 99, 1000]) = 14
451513 geteuid()                        = 1000
451513 setresuid(0, 0, -1)              = 0
451513 geteuid()                        = 0
451513 geteuid()                        = 0
451513 getegid()                        = 1000
451513 setresgid(-1, 0, -1)             = 0
451513 getegid()                        = 0
451513 getegid()                        = 0
451513 setgroups(1, [99])               = 0
451513 setresgid(-1, 99, -1)            = 0
451513 getegid()                        = 99
451513 setresuid(99, 99, -1)            = 0
451513 geteuid()                        = 99
451513 stat("S/(here was full file name)", {st_mode=S_IFREG|0644, st_size=929913507, ...}) = 0
451513 unlinkat(AT_FDCWD, "S/(here was full file name)", 0) = -1 EACCES (Permission denied)


samba log:

[2021/01/22 20:55:18.883935,  5, pid=451043, effective(1000, 1000), real(1000, 0)] ../../source3/smbd/close.c:380(close_remove_share_mode)
  close_remove_share_mode: file S/aaa.file. Delete on close was set - deleting file.
[2021/01/22 20:55:18.883944, 10, pid=451043, effective(1000, 1000), real(1000, 0), class=locking] ../../source3/locking/locking.c:960(find_delete_on_close_token)
  find_delete_on_close_token: name_hash = 0x9db0b11
[2021/01/22 20:55:18.883953, 10, pid=451043, effective(1000, 1000), real(1000, 0), class=locking] ../../source3/locking/locking.c:965(find_delete_on_close_token)
  find_delete_on_close_token: dt->name_hash = 0x9db0b11
[2021/01/22 20:55:18.883961,  5, pid=451043, effective(1000, 1000), real(1000, 0)] ../../source3/smbd/close.c:395(close_remove_share_mode)
  close_remove_share_mode: file S/aaa.file-dir/aaa.file. Change user to uid 99
[2021/01/22 20:55:18.883971,  4, pid=451043, effective(1000, 1000), real(1000, 0)] ../../source3/smbd/sec_ctx.c:215(push_sec_ctx)
  push_sec_ctx(1000, 1000) : sec_ctx_stack_ndx = 1
[2021/01/22 20:55:18.883983,  4, pid=451043, effective(1000, 1000), real(1000, 0)] ../../source3/smbd/sec_ctx.c:319(set_sec_ctx_internal)
  setting sec ctx (99, 99) - sec_ctx_stack_ndx = 1
[2021/01/22 20:55:18.883992,  5, pid=451043, effective(1000, 1000), real(1000, 0)] ../../libcli/security/security_token.c:56(security_token_debug)
  Security token SIDs (5):
    SID[  0]: S-1-5-7
    SID[  1]: S-1-1-0
    SID[  2]: S-1-5-2
    SID[  3]: S-1-22-1-99
    SID[  4]: S-1-22-2-99
   Privileges (0x               0):
   Rights (0x               0):
[2021/01/22 20:55:18.884013,  5, pid=451043, effective(1000, 1000), real(1000, 0)] ../../source3/auth/token_util.c:873(debug_unix_user_token)
  UNIX token of user 99
  Primary group is 99 and contains 1 supplementary groups
  Group[  0]: 99
[2021/01/22 20:55:18.884043,  5, pid=451043, effective(99, 99), real(99, 0)] ../../source3/smbd/close.c:488(close_remove_share_mode)
  close_remove_share_mode: file S/aaa.file. Delete on close was set and unlink failed with error Permission denied
Comment 1 Jeremy Allison 2021-01-22 23:09:57 UTC
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.
Comment 2 Jeremy Allison 2021-01-22 23:15:45 UTC
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.
Comment 3 Jeremy Allison 2021-01-22 23:23:41 UTC
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.
Comment 4 Jeremy Allison 2021-01-22 23:24:09 UTC
Ralph, you might want to look at this. I screwed up giving a +1 to your code in review, sorry :-).
Comment 5 Jeremy Allison 2021-01-23 03:24:17 UTC
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 6 Jeremy Allison 2021-01-23 05:49:46 UTC
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 :-).
Comment 7 Rowland Penny 2021-01-23 12:00:30 UTC
You are probably correct Jeremy, there is however the problem that on 4.13.2 it works for myself ;)
Comment 8 Ralph Böhme 2021-01-23 12:40:05 UTC
(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.
Comment 9 Arkadiusz Miskiewicz 2021-01-23 12:46:56 UTC
Doesn't work for me on vanilla 4.13.2, just tested.
Comment 10 Ralph Böhme 2021-01-23 17:52:09 UTC
(In reply to Jeremy Allison from comment #5)
Test idea sounds good.
Comment 11 Ralph Böhme 2021-01-23 17:53:51 UTC
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.
Comment 12 Jeremy Allison 2021-01-23 22:41:57 UTC
(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 :-).
Comment 13 Jeremy Allison 2021-01-23 23:03:39 UTC
(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 :-).
Comment 14 Jeremy Allison 2021-01-23 23:11:59 UTC
(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.
Comment 15 Jeremy Allison 2021-01-23 23:29:50 UTC
(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.
Comment 16 Arkadiusz Miskiewicz 2021-01-24 12:20:39 UTC
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.
Comment 17 Ralph Böhme 2021-01-25 14:01:39 UTC
Fix including test running at <https://gitlab.com/samba-team/samba/-/merge_requests/1772>.
Comment 18 Jeremy Allison 2021-01-26 17:41:03 UTC
Created attachment 16405 [details]
git-am fix for 4.13.next.

Cherry-picked from master.
Comment 20 Jeremy Allison 2021-01-26 17:47:41 UTC
Patch needs fix-up. Sorry for the premature click for review.
Comment 21 Jeremy Allison 2021-01-26 17:57:51 UTC
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
Comment 22 Ralph Böhme 2021-01-26 18:10:12 UTC
(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?
Comment 23 Jeremy Allison 2021-01-26 20:52:37 UTC
Created attachment 16408 [details]
git-am fix for 4.14.next.

Cherry-picked from master.
Comment 24 Ralph Böhme 2021-01-27 08:52:31 UTC
Reassigning to Karolin for inclusion in 4.13 and 4.14.
Comment 25 Karolin Seeger 2021-01-27 10:23:21 UTC
(In reply to Ralph Böhme from comment #24)
Pushed to autobuild-v4-{14,13}-test.
Comment 26 Samba QA Contact 2021-01-29 13:54:06 UTC
This bug was referenced in samba master:

c44dad3ac2eb36fc5eb5a9f80a9ef97183be26ef
f3f8fdfbf10f690bc8d972a13d6f74f1fb0fb375
aa1f09cda0a097617e34dd0a8b1b0acc7a37bca8
e06f86bbd93d024c70016e1adcf833db85742aca
Comment 27 Samba QA Contact 2021-01-29 13:55:29 UTC
This bug was referenced in samba v4-14-test:

d5a696fc8869b9ce65f063ce93b04d07b01c2b2a
4c9cf755eb2a9e9e257265c5bbef6113e7b2e8e3
4bfdc4eff93419a8608f163e6fdade7192be678e
fcc6a32e069ed20d1ec141f792481f03b57dbdb0
Comment 28 Samba QA Contact 2021-02-01 08:48:12 UTC
This bug was referenced in samba v4-13-test:

483c1dc818ec748daad85fd8e4f223d6edf22f60
35eddb388f248cb206518eb2843f9aaf1479bfeb
ba12f0c3ae02d002435ecbb32ac018f8eb821691
f6e5fe6f122f85dda46872045cbc4cb020b399b9
Comment 29 Karolin Seeger 2021-02-01 08:49:28 UTC
Closing out bug report.

Thanks!
Comment 30 Samba QA Contact 2021-02-04 08:26:58 UTC
This bug was referenced in samba v4-14-stable (Release samba-4.14.0rc2):

d5a696fc8869b9ce65f063ce93b04d07b01c2b2a
4c9cf755eb2a9e9e257265c5bbef6113e7b2e8e3
4bfdc4eff93419a8608f163e6fdade7192be678e
fcc6a32e069ed20d1ec141f792481f03b57dbdb0