Bug 14264 - "net sam listmem Administrators" leads to PANIC: Bad talloc magic value
Summary: "net sam listmem Administrators" leads to PANIC: Bad talloc magic value
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.9.5
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andreas Schneider
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-05 05:10 UTC by Jonathon Reinhart
Modified: 2020-03-06 17:11 UTC (History)
3 users (show)

See Also:


Attachments
Possible fix for talloc_steal() bug (965 bytes, patch)
2020-02-05 05:15 UTC, Jonathon Reinhart
no flags Details
patch for 4.12, 4.11 and 4.10 (2.14 KB, patch)
2020-03-06 17:11 UTC, Andreas Schneider
asn: review? (gd)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Reinhart 2020-02-05 05:10:42 UTC
I ran into this on samba 4.9.5+dfsg-5+deb10u1 (debian buster):

root@dc1:~# net sam listmem Administrators
Bad talloc magic value - unknown value
PANIC (pid 1111): Bad talloc magic value - unknown value
BACKTRACE: 10 stack frames:
 #0 /lib/x86_64-linux-gnu/libsamba-util.so.0(log_stack_trace+0x32) [0x7fd60f5948d2]
 #1 /lib/x86_64-linux-gnu/libsmbconf.so.0(smb_panic_s3+0x20) [0x7fd60f3d51c0]
 #2 /lib/x86_64-linux-gnu/libsamba-util.so.0(smb_panic+0x2f) [0x7fd60f5949df]
 #3 /lib/x86_64-linux-gnu/libtalloc.so.2(+0x3d3d) [0x7fd60e83fd3d]
 #4 /lib/x86_64-linux-gnu/libsamba-passdb.so.0(+0x480c6) [0x7fd60f63e0c6]
 #5 net(+0x6b8f1) [0x564b1aba18f1]
 #6 net(net_sam+0x5f) [0x564b1aba3c9f]
 #7 net(main+0x980) [0x564b1ab585b0]
 #8 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb) [0x7fd60e3b409b]
 #9 net(_start+0x2a) [0x564b1ab5887a]
Can not dump core: corepath not set up
Comment 1 Jonathon Reinhart 2020-02-05 05:13:05 UTC
While samba-libs-dbgsym does exist, it does not appear to be updated with security.debian.org. So debugging this without symbols in `net` or `libsamba-passdb.so` was a bit challenging.


At frame #5: net_sam_listmem() calls pdb_enum_aliasmem() here:

https://github.com/samba-team/samba/blob/samba-4.9.5/source3/utils/net_sam.c#L1366

After an indirect tail-call, we end up in pdb_samba_dsdb_enum_aliasmem(), which makes a call to dsdb_enum_group_mem() here:

https://github.com/samba-team/samba/blob/samba-4.9.5/source3/passdb/pdb_samba_dsdb.c#L1723-L1727


Here is the problematic function:

static NTSTATUS pdb_samba_dsdb_enum_aliasmem(struct pdb_methods *m,
				      const struct dom_sid *alias,
				      TALLOC_CTX *mem_ctx,
				      struct dom_sid **pmembers,
				      size_t *pnum_members)
{
	// ...

	status = dsdb_enum_group_mem(state->ldb, mem_ctx, dn, pmembers, &num_members);
	*pnum_members = num_members;
	if (NT_STATUS_IS_OK(status)) {
		talloc_steal(mem_ctx, pmembers);      // <<<<<<<< BUG
	}
	talloc_free(tmp_ctx);
	return status;
}

Frame #4 is the call to talloc_steal().


I'm very new to talloc but I think there are a total of 3 issues here:

1. It looks like pdb_samba_dsdb_enum_aliasmem() should be passing *pmembers to talloc_steal(), not pmembers, since it is just passing-through the double pointer to dsdb_enum_group_mem(), and thus *pmembers is the actual allocated object.

2. It looks like the talloc_steal() in parent pdb_samba_dsdb_enum_aliasmem() is
pointless since child dsdb_enum_group_mem() already calls talloc_steal() with
the same mem_ctx here:
https://github.com/samba-team/samba/blob/samba-4.9.5/source4/dsdb/common/util_samr.c#L484


3. It looks like even that talloc_steal() in dsdb_enum_group_mem() is
pointless, since `members` was allocated from the same mem_ctx:
https://github.com/samba-team/samba/blob/samba-4.9.5/source4/dsdb/common/util_samr.c#L449
Comment 2 Jonathon Reinhart 2020-02-05 05:15:42 UTC
Created attachment 15773 [details]
Possible fix for talloc_steal() bug

This patch is against tag samba-4.9.5 (214ec9c).

I didn't even try to compile it, much less test it, but based on my brief analysis and limited understanding of talloc(), I think it makes sense.
Comment 3 Andreas Schneider 2020-03-06 17:11:04 UTC
Created attachment 15846 [details]
patch for 4.12, 4.11 and 4.10