Bug 13031 - Rename of site can crash ldap process (signal 6)
Rename of site can crash ldap process (signal 6)
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB
4.6.7
All All
: P5 normal
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-13 23:07 UTC by Andrew Bartlett
Modified: 2018-02-22 09:59 UTC (History)
9 users (show)

See Also:


Attachments
backtrace using ldbrename of subnet (3.18 KB, text/plain)
2017-09-19 04:50 UTC, Garming Sam
no flags Details
Patch and test (5.95 KB, patch)
2017-09-20 03:02 UTC, Garming Sam
no flags Details
patch with BUG:, tested on master, 4.5, 4.6, 4.7 (6.20 KB, patch)
2018-01-02 23:51 UTC, Douglas Bagnall
abartlet: review+
dbagnall: review? (gary)
Details
Additional patch for master testing if non-admin users can trigger this bug (5.41 KB, patch)
2018-02-15 02:46 UTC, Douglas Bagnall
dbagnall: review? (garming)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2017-09-13 23:07:59 UTC
Kevin Kunkel reports that renaming a site from ADUC crashed the LDAP server with signal 6. 

Details are pending.
Comment 1 Kevin Kunkel 2017-09-19 04:12:19 UTC
Created attachment 13606 [details]
Active Directory Sites and Service MSC plugin

When renaming a subnet in the Active Directory Sites and Services MSC plugin, Samba's LDAP process will crash:

[2017/09/12 11:28:37.037848,  0] ../lib/util/fault.c:78(fault_report)
  ===============================================================
[2017/09/12 11:28:37.037951,  0] ../lib/util/fault.c:79(fault_report)
  INTERNAL ERROR: Signal 11 in pid 10066 (4.6.7-SerNet-RedHat-11.el7)
  Please read the Trouble-Shooting section of the Samba HOWTO
[2017/09/12 11:28:37.037973,  0] ../lib/util/fault.c:81(fault_report)
  ===============================================================
[2017/09/12 11:28:37.037986,  0] ../lib/util/fault.c:151(smb_panic_default)
  PANIC: internal error
[2017/09/12 11:28:37.043107,  0] ../source4/smbd/process_standard.c:127(standard_child_pipe_handler)
  Child 10066 (ldap) terminated with signal 6
Comment 2 Garming Sam 2017-09-19 04:50:02 UTC
Created attachment 13607 [details]
backtrace using ldbrename of subnet

Command to trigger segfault:

bin/ldbrename -H st/ad_dc/private/sam.ldb CN=10.9.0.0/24,CN=Subnets,CN=Sites,CN=Configuration,... CN=10.9.0.1/24,CN=Subnets,CN=Sites,CN=Configuration,...
Comment 3 Garming Sam 2017-09-19 04:57:56 UTC
Fix is relatively trivial. Pass in a new parameter, DN, to the verify subnet function. Impact is restricted to those able to modify an existing subnet.
Comment 4 Garming Sam 2017-09-20 03:02:15 UTC
Created attachment 13611 [details]
Patch and test
Comment 6 Douglas Bagnall 2018-01-02 22:06:28 UTC
Created attachment 13889 [details]
patch and test v2

RB+ by me with the following modification:

--- a/python/samba/subnets.py
+++ b/python/samba/subnets.py
@@ -153,9 +153,11 @@ def rename_subnet(samdb, configDn, subnet_name, new_name):
         if enum == ldb.ERR_NO_SUCH_OBJECT:
             raise SubnetNotFound('Subnet %s does not exist' % subnet)
         elif enum == ldb.ERR_ENTRY_ALREADY_EXISTS:
-            raise SubnetAlreadyExists('A subnet with the CIDR %s already exists')
+            raise SubnetAlreadyExists('A subnet with the CIDR %s already exists'
+                                      % new_name)
         elif enum == ldb.ERR_INVALID_DN_SYNTAX:
-            raise SubnetInvalid("%s is not a valid subnet: %s" % (new_name, estr))
+            raise SubnetInvalid("%s is not a valid subnet: %s" % (new_name,
+                                                                  estr))
         else:
             raise

The first -+ adds a missing name in the error string.
The second -+ avoids the 80th column.
Comment 7 Douglas Bagnall 2018-01-02 22:09:05 UTC
Thanks Björn,

The status from my point of view is I assumed it had already gone upstream.

We will get moving.
Comment 8 Douglas Bagnall 2018-01-02 23:51:39 UTC
Created attachment 13890 [details]
patch with BUG:, tested on master, 4.5, 4.6, 4.7

This patch applies as far back as 4.4 which introduced the feature/bug.

The relevant tests pass on 4.5, 4.6, 4.7.
Comment 9 Douglas Bagnall 2018-02-14 02:40:32 UTC
The patch also applies against v4-8-test.
Comment 10 Douglas Bagnall 2018-02-15 02:46:45 UTC
Created attachment 13962 [details]
Additional patch for master testing if non-admin users can trigger this bug

Can non-admin users trigger this bug? These tests say NO.

gdb confirms that access rights are checked before samldb_verify_subnet() is reached.

This patch can go into master in a non urgent fashion, but NOT before the security patch. There is any point in backporting it, as far as I can see.
Comment 14 Ralph Böhme 2018-02-19 10:47:59 UTC
Reassigning to Karolin for inclusion in 4.8, 4.7 and 4.6.
Comment 15 Karolin Seeger 2018-02-20 11:50:21 UTC
(In reply to Ralph Böhme from comment #14)
Pushed to autobuild-v4{6,7,8}-test.
Comment 16 Karolin Seeger 2018-02-22 09:59:03 UTC
Pushed to all branches.
Closing out bug report.

Thanks!