Bug 13690 - Adding `force group = ...` to an active SMB sessoin causes PANIC
Summary: Adding `force group = ...` to an active SMB sessoin causes PANIC
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.8.6
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-23 06:03 UTC by SATOH Fumiyasu
Modified: 2019-02-22 08:24 UTC (History)
3 users (show)

See Also:


Attachments
git-am fix for 4.7.x and above. (2.60 KB, patch)
2019-01-18 22:27 UTC, Jeremy Allison
no flags Details
git-am fix for master (9.13 KB, patch)
2019-01-24 18:41 UTC, Jeremy Allison
no flags Details
git-am fix for 4.10.rcNext - cherry-pick from master (9.56 KB, patch)
2019-01-25 17:23 UTC, Jeremy Allison
slow: review+
asn: review+
Details
git-am fix for 4.9.next, 4.8.next (8.97 KB, patch)
2019-01-25 17:25 UTC, Jeremy Allison
slow: review+
asn: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description SATOH Fumiyasu 2018-11-23 06:03:41 UTC
How to reproduce:

  1. Define a share without `force group=...` in the smb.conf.
  2. Connect to the share by an SMB client.
  3. Add `force group=...` to the share definition in the smb.conf.
  4. Run `smbcontrol all reload-config` to apply the change.
  5. Access a file in the share by the SMB client.

log.smbd:

[2018/11/23 14:57:52, 10, pid=23158, effective(0, 0), real(0, 0), class=smb2] ../source3/smbd/smb2_server.c:3934(smbd_smb2_io_handler)
  smbd_smb2_request idx[1] of 5 vectors
[2018/11/23 14:57:52, 10, pid=23158, effective(0, 0), real(0, 0), class=smb2_credits] ../source3/smbd/smb2_server.c:678(smb2_validate_sequence_number)
  smb2_validate_sequence_number: smb2_validate_sequence_number: clearing id 8 (position 8) from bitmap
[2018/11/23 14:57:52, 10, pid=23158, effective(0, 0), real(0, 0), class=smb2] ../source3/smbd/smb2_server.c:2327(smbd_smb2_request_dispatch)
  smbd_smb2_request_dispatch: opcode[SMB2_OP_CREATE] mid = 8
[2018/11/23 14:57:52,  0, pid=23158, effective(0, 0), real(0, 0)] ../source3/smbd/uid.c:319(change_to_user_internal)
  PANIC: assert failed at ../source3/smbd/uid.c(319): conn->force_group_gid != (gid_t)-1
[2018/11/23 14:57:52,  0, pid=23158, effective(0, 0), real(0, 0)] ../source3/lib/util.c:815(smb_panic_s3)
  PANIC (pid 23158): assert failed: conn->force_group_gid != (gid_t)-1
[2018/11/23 14:57:52,  0, pid=23158, effective(0, 0), real(0, 0)] ../lib/util/fault.c:261(log_stack_trace)
  BACKTRACE: 21 stack frames:
   #0 /usr/lib64/samba/libsamba-util.so.0(log_stack_trace+0x1a) [0x7f3c8b99a48a]
   #1 /usr/lib64/samba/libsmbconf.so.0(smb_panic_s3+0x20) [0x7f3c89690b20]
   #2 /usr/lib64/samba/libsamba-util.so.0(smb_panic+0x2f) [0x7f3c8b99a56f]
   #3 /usr/lib64/samba/libsmbd-base-samba4.so(+0x17987f) [0x7f3c8b57587f]
   #4 /usr/lib64/samba/libsmbd-base-samba4.so(smbd_smb2_request_dispatch+0x9f5) [0x7f3c8b5b1395]
   #5 /usr/lib64/samba/libsmbd-base-samba4.so(+0x1b7bf3) [0x7f3c8b5b3bf3]
   #6 /usr/lib64/samba/libtevent.so.0(+0xa58b) [0x7f3c8a9de58b]
   #7 /usr/lib64/samba/libtevent.so.0(+0x8957) [0x7f3c8a9dc957]
   #8 /usr/lib64/samba/libtevent.so.0(_tevent_loop_once+0x9d) [0x7f3c8a9d8d4d]
   #9 /usr/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b) [0x7f3c8a9d8f7b]
   #10 /usr/lib64/samba/libtevent.so.0(+0x88f7) [0x7f3c8a9dc8f7]
   #11 /usr/lib64/samba/libsmbd-base-samba4.so(smbd_process+0x5e1) [0x7f3c8b5a1281]
   #12 /usr/sbin/smbd(+0xe380) [0x55ddcd1e5380]
   #13 /usr/lib64/samba/libtevent.so.0(+0xa58b) [0x7f3c8a9de58b]
   #14 /usr/lib64/samba/libtevent.so.0(+0x8957) [0x7f3c8a9dc957]
   #15 /usr/lib64/samba/libtevent.so.0(_tevent_loop_once+0x9d) [0x7f3c8a9d8d4d]
   #16 /usr/lib64/samba/libtevent.so.0(tevent_common_loop_wait+0x1b) [0x7f3c8a9d8f7b]
   #17 /usr/lib64/samba/libtevent.so.0(+0x88f7) [0x7f3c8a9dc8f7]
   #18 /usr/sbin/sv/smbd(main+0x1aec) [0x55ddcd1df56c]
   #19 /lib64/libc.so.6(__libc_start_main+0xf5) [0x7f3c87eeec05]
   #20 /usr/sbin/sv/smbd(+0x883c) [0x55ddcd1df83c]
[2018/11/23 14:57:52,  0, pid=23158, effective(0, 0), real(0, 0)] ../source3/lib/dumpcore.c:315(dump_core)
  dumping core in /var/log/samba/cores/smbd
Comment 1 Andreas Hasenack 2019-01-08 19:43:59 UTC
I can reproduce this with samba 4.7.6 as well.
Comment 2 Amit Kumar 2019-01-18 06:27:57 UTC
Yes, reproduced with this config.

SMB-SERVER
smb.conf
 [global]
    workgroup = <>
    security = user
    map to guest = Bad User
    passdb backend = tdbsam
[myshare]
    path = /share
    browsable = yes
    guest ok = yes
    read only = no
    create mask = 0755
# service smb start

SMBCLIENT
$ smbclient //<samba-share-ip>/myshare -U <any-user>
Enter workgroup\<any-user>'s password:
Try "help" to get a list of possible commands.
smb: \> ls
  dir                                 D        0  Thu Oct 18 11:58:31 2018
smb: \>

SMB-SERVER
# vim smb.conf
[myshare]
  force group = root
# smbcontrol all reload-config

SMB-CLIENT
smb: \> cd dir1
cd \dir1\: NT_STATUS_CONNECTION_DISCONNECTED
smb: \> SMBecho failed (NT_STATUS_CONNECTION_DISCONNECTED). The connection is disconnected now

Core is dumped on Server.
Comment 3 Amit Kumar 2019-01-18 07:02:02 UTC
Here Wrong gid value is fetched.

Code crashes here:
SMB_ASSERT(conn->force_group_gid != (gid_t)-1);
(gdb) p conn->force_group_gid
$1 = 4294967295

Which will always fail since 'typedef unsigned int gid_t;'

Will investigate
Comment 4 Jeremy Allison 2019-01-18 21:39:01 UTC
Ah, now this makes sense.

On initial construction, inside conn_new(), we set:

conn->force_group_gid = (gid_t)-1;

which means ignore the force group parameter for this share.

A connection is active, but now you add the 'force group = XXX' to the existing connection and reload.

When the existing connection wants to change credentials back to the token, the new 'force group' requirement it finds the force_group_gid = -1 and refuses to change (which is failing safe, you don't want it to continue with this value).

The underlying issue is that we don't change credentials out from under an existing connection on reload, but in this case we don't record the fact this existing connection existed before the new 'force group' mandate.

Let me think about what the correct behavior here should be - I think we want the new 'force group' to be ignored for existing connections, but honored for new connections.
Comment 5 Jeremy Allison 2019-01-18 22:27:15 UTC
Created attachment 14790 [details]
git-am fix for 4.7.x and above.

Can you test this to see if it fixes the problem ?

Thanks,

Jeremy.
Comment 6 Andreas Hasenack 2019-01-21 13:11:30 UTC
Confirmed fixed in 4.7.6 with the patch applied
Comment 7 Jeremy Allison 2019-01-23 01:05:32 UTC
OK, thanks. Now I need to write the unit test for this.
Comment 8 Jeremy Allison 2019-01-24 18:41:56 UTC
Created attachment 14797 [details]
git-am fix for master

git-am fix for master, includes regression test.
Comment 9 Jeremy Allison 2019-01-25 17:23:13 UTC
Created attachment 14802 [details]
git-am fix for 4.10.rcNext - cherry-pick from master
Comment 10 Jeremy Allison 2019-01-25 17:25:07 UTC
Created attachment 14803 [details]
git-am fix for 4.9.next, 4.8.next

Back-port from master.
Comment 11 Jeremy Allison 2019-02-09 01:07:50 UTC
Ping Ralph, can I get a review on these ?
Comment 12 Ralph Böhme 2019-02-09 07:04:52 UTC
Reassigning to Karolin for inclusion in 4.8, 4.9 and 4.10.
Comment 13 Karolin Seeger 2019-02-21 11:11:53 UTC
(In reply to Ralph Böhme from comment #12)
Pushed to autobuild-v4-{10,9,8}-test.
Comment 14 Karolin Seeger 2019-02-22 08:24:40 UTC
(In reply to Karolin Seeger from comment #13)
Pushed to all branches.
Closing out bug report.

Thanks!