Bug 13664 - Wrong users logged by the groups membership change auditing.
Summary: Wrong users logged by the groups membership change auditing.
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.9.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-24 19:51 UTC by Gary Lockyer
Modified: 2018-11-06 08:14 UTC (History)
2 users (show)

See Also:
gary: review? (metze)


Attachments
Proposed bug fix. (29.59 KB, patch)
2018-10-30 00:15 UTC, Gary Lockyer
no flags Details
Backported fix for version 4.9 (31.52 KB, text/plain)
2018-11-02 01:46 UTC, Gary Lockyer
abartlet: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gary Lockyer 2018-10-24 19:51:00 UTC
log_membership_changes: START(0x555a6b7e6be0)
log_membership_changes: old_num_values[1] new_num_values[2]
max_num_values[3]
old_i[0/0x555a6b9aca60/<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;
new_i[0/0x555a6c5dc090/<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;
new_i[1/0x555a6c5dc0b0/<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;
log_membership_changes: i[0] cmp[0]
old[0/0x555a6b9aca60/<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;
new[0/0x555a6c5dc090/<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;
Group Change TX[fbb38813-671d-4482-8e26-b6c01a022ea5] [RemovedLESS] at
[Thu, 18 Oct 2018 15:19:57.430981 CEST] status [Success] Remote host
[ipv4:127.0.0.11:30884] SID
[S-1-5-21-2316301919-1546975925-949975352-500] Group [CN=Domain
Users,CN=Users,DC=addom,DC=samba,DC=example,DC=com] User
[CN=testuser131953,CN=Users,DC=addom,DC=samba,DC=example,DC=com]
log_membership_changes: i[1] cmp[3]
old[1/(nil)/]
new[0/0x555a6c5dc090/<GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>;
Group Change TX[fbb38813-671d-4482-8e26-b6c01a022ea5] [Added] at [Thu,
18 Oct 2018 15:19:57.431160 CEST] status [Success] Remote host
[ipv4:127.0.0.11:30884] SID
[S-1-5-21-2316301919-1546975925-949975352-500] Group [CN=Domain
Users,CN=Users,DC=addom,DC=samba,DC=example,DC=com] User
[cn=grpadttstuser01,cn=users,DC=addom,DC=samba,DC=example,DC=com]
log_membership_changes: i[2] cmp[3]
old[1/(nil)/]
new[1/0x555a6c5dc0b0/<GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>;
Group Change TX[fbb38813-671d-4482-8e26-b6c01a022ea5] [Added] at [Thu,
18 Oct 2018 15:19:57.431303 CEST] status [Success] Remote host
[ipv4:127.0.0.11:30884] SID
[S-1-5-21-2316301919-1546975925-949975352-500] Group [CN=Domain
Users,CN=Users,DC=addom,DC=samba,DC=example,DC=com] User
[CN=testuser131953,CN=Users,DC=addom,DC=samba,DC=example,DC=com]
log_membership_changes: END(0x555a6b7e6be0)

Because of the wrong logic the changes are reported instead of just one.

Remove <GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>
Add <GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>
Add <GUID=081519b5-a709-44a0-bc95-dd4bfe809bf8>

Instead of just
Add <GUID=cb8c2777-dcf5-419c-ab57-f645dbdf681b>
Comment 1 Gary Lockyer 2018-10-30 00:15:15 UTC
Created attachment 14548 [details]
Proposed bug fix.
Comment 2 Gary Lockyer 2018-11-02 01:46:33 UTC
Created attachment 14560 [details]
Backported fix for version 4.9

CI results https://gitlab.com/samba-team/devel/samba/pipelines/35144837
Comment 3 Andrew Bartlett 2018-11-04 22:13:12 UTC
Comment on attachment 14560 [details]
Backported fix for version 4.9

This looks wrong:

+	value = json_string_value(v);
+	if (strncmp("groupChange", value, strlen("groupChange") != 0)) {
+		cm_print_error(
+		    "Unexpected type \"%s\" != \"groupChange\"\n",
+		    value);
+		_fail(file, line);
+	}

As the strlen() implies we could just do a strcmp(), which would then catch a value of (eg) "groupChanged".  

The same applies to the other string checks.

These need to be fixed in master and an additional patch backported.  We do want this patch for 4.9.2 so I'll still approve, but we should have the stronger test long-term.
Comment 4 Andrew Bartlett 2018-11-04 22:14:33 UTC
G'Day Karolin,

Please pick for 4.9.next.

Thanks,

Andrew Bartlett
Comment 5 Karolin Seeger 2018-11-05 11:43:52 UTC
(In reply to Andrew Bartlett from comment #4)
Pushed to autobuild-v4-9-test
Comment 6 Karolin Seeger 2018-11-06 08:14:23 UTC
(In reply to Karolin Seeger from comment #5)
Pushed to v4-9-test.
Closing out bug report.

Thanks!