Bug 13124 - StartTLS certificate verification broken in ldap ssl ads
Summary: StartTLS certificate verification broken in ldap ssl ads
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.5.12
Hardware: x86 Linux
: P5 regression (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-07 15:40 UTC by john.workman
Modified: 2020-12-16 12:21 UTC (History)
11 users (show)

See Also:

Attachments
fix for 4.12, cherry-picked from master (1.65 KB, patch)
2020-06-11 13:02 UTC, Björn Baumbach 		bbaumbach: ci-passed+
Details
fix for 4.11, cherry-picked from master (1.65 KB, patch)
2020-06-11 13:03 UTC, Björn Baumbach 		bbaumbach: ci-passed+
Details
fix for 4.12, cherry-picked from master (2.89 KB, patch)
2020-06-11 13:08 UTC, Björn Baumbach 		abartlet: review+
iboukris: review-
bbaumbach: ci-passed+
Details
fix for 4.11, cherry-picked from master (2.89 KB, patch)
2020-06-11 13:08 UTC, Björn Baumbach 		abartlet: review+
iboukris: review-
bbaumbach: ci-passed+
Details
patch for 4.13 (1.18 KB, patch)
2020-12-04 09:18 UTC, Andreas Schneider 		asn: review? (ab)
iboukris: review+
Details
additional patch for 4.12 and 4.11 (1.18 KB, patch)
2020-12-04 09:21 UTC, Andreas Schneider 		asn: review? (ab)
iboukris: review-
Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description john.workman 2017-11-07 15:40:22 UTC 
Summary:

StartTLS certificate verification broken in ldap ssl ads.


Configuration: 

 - Windows domain with at least one domain controller with LDAP starttls enabled and valid SSL certificates installed for this service
 - winbindd joined to the domain
 - windbind configured with /etc/samba/smb.conf with the following:
         
         ldap ssl = start_tls
         ldap ssl ads = yes
         client ldap sasl wrapping = plain
         ldap debug level = 10

- /etc/ldap/ldap.conf (libldap-2.4) configured with the following

         TLS_CACERT	/etc/ssl/certs/ca-certificates.crt
         TLS_REQCERT	demand
         

Steps to trigger the bug:

Start winbindd as:
windbind -F -S -d 11


Then run:
wbinfo --user-info=some_user_in_active_directory


winbindd will produce a message like:
[LDAP] TLS: hostname (134.29.52.75) does not match common name in certificate (mavdisk.mnsu.edu).


The problem appears to be in source3/libads/ldap.c, ldap_open() is being supplied with the LDAP server's IP address, instead of hostname.

Reversing the following change fixes this issue:

https://github.com/samba-team/samba/commit/2b44c85c7b322b392c8d3d0f393171ca54bb5f47#diff-bfc4ccf9689e89040e08d5730d53961e


References:

https://bugs.launchpad.net/ubuntu/+source/samba/+bug/1576799
Comment 1 Björn Baumbach 2017-12-13 14:37:49 UTC 
(In reply to john.workman from comment #0)

Dear John,

thank you for reporting this.

Unfortunately my commit message for the watch was very short and does not point to the reason for the patch in detail.
I assume my intention was to fix 'net rpc join' on systems with a broken dns configuration. This path might not be necessary anymore and should be verified.
If it's working fine, we should reverse my patch and use it as the bug fix.

John, can you verify this? Otherwise I'll try this later.

Best regards,
Björn
Comment 2 Andreas Hasenack 2017-12-18 13:23:37 UTC 
Ubuntu Xenial test packages with that change reversed are currently building in this PPA if someone wants to test them:

https://launchpad.net/~ahasenack/+archive/ubuntu/samba-tls-regression-1576799

I don't have an AD at hand right now to try it myself.
Comment 3 arjithpe 2017-12-19 05:46:21 UTC 
I have tried this fix.
But i am still getting the same issue.
Comment 4 john.workman 2017-12-19 15:47:04 UTC 
(In reply to Björn Baumbach from comment #1)

I assumed this was the reason for the change I linked to (where the NetBIOS name cannot be resolved through DNS, so make LDAP just use the IP).

I confirm that reversing the patch fixed the TLS issue for me. I am running production winbind against Active Directory servers with full TLS crypto and certificate verification.
Comment 5 Andreas Hasenack 2017-12-31 13:33:05 UTC 
Reverting that commit worked past the certificate issue for me, but I hit something else that looks like a microsoft issue:

[LDAP] res_errno: 53, res_error: <00002029: LdapErr: DSID-0C0904CB, comment: Cannot bind using sign/seal on a connection on which TLS or SSL is in effect, data 0, v3839>, res_matched: <>


I was able to work around that by setting "client ldap sasl wrapping = plain" in smb.conf.
Comment 6 arjithpe 2018-01-31 06:58:34 UTC 
With the patch Andreas and setting "client ldap sasl wrapping = plain" in smb.conf.
I am able to run net ads join successfully with StartTLS when AD DC is windows server.
But if i configure samba as AD DC i get below error:-

Sign or Seal are required.>, res_matched: <>
kinit succeeded but ads_sasl_spnego_gensec_bind(KRB5) failed: Strong(er) authentication required.

Note :- i have also tried to chnage "client ldap sasl wrapping = sign"

I have observe this issue on Ubuntu and hp-ux
Comment 7 gernot 2020-02-06 08:26:52 UTC 
This is undoubtedly a clear bug which prevents the validation of the server certificate. Are there plans to fix this bug in the near future?
Comment 8 Björn Baumbach 2020-06-11 13:02:43 UTC 
Created attachment 16030 [details]
fix for 4.12, cherry-picked from master

CI passed: https://gitlab.com/samba-team/devel/samba/-/pipelines/155201930
Comment 9 Björn Baumbach 2020-06-11 13:03:39 UTC 
Created attachment 16031 [details]
fix for 4.11, cherry-picked from master

CI passed: https://gitlab.com/samba-team/devel/samba/-/pipelines/155202380
Comment 10 Björn Baumbach 2020-06-11 13:08:06 UTC 
Created attachment 16032 [details]
fix for 4.12, cherry-picked from master
Comment 11 Björn Baumbach 2020-06-11 13:08:41 UTC 
Created attachment 16033 [details]
fix for 4.11, cherry-picked from master
Comment 12 Björn Baumbach 2020-06-11 17:53:23 UTC 
Comment on attachment 16032 [details]
fix for 4.12, cherry-picked from master

CI passed: https://gitlab.com/samba-team/devel/samba/-/pipelines/155252281
Comment 13 Björn Baumbach 2020-06-11 17:53:55 UTC 
Comment on attachment 16033 [details]
fix for 4.11, cherry-picked from master

https://gitlab.com/samba-team/devel/samba/-/pipelines/155252209
Comment 14 Andrew Bartlett 2020-06-11 22:20:38 UTC 
Assigning to Karolin for 4.11 and 4.12.
Comment 15 Isaac Boukris 2020-07-14 20:42:46 UTC 
Hi, I think we also need:
https://gitlab.com/samba-team/samba/-/merge_requests/1464
Comment 16 Andreas Schneider 2020-12-04 09:15:27 UTC 
Karolin, please apply the patches to 4.13, 4.12 and 4.11.
Comment 17 Andreas Schneider 2020-12-04 09:16:33 UTC 
And we need updated patches ...
Comment 18 Andreas Schneider 2020-12-04 09:18:35 UTC 
Created attachment 16357 [details]
patch for 4.13
Comment 19 Andreas Schneider 2020-12-04 09:21:00 UTC 
Created attachment 16358 [details]
additional patch for 4.12 and 4.11

This needs to be applied after "fix for 4.(11|12), cherry-picked from master".
Comment 20 Andreas Schneider 2020-12-04 10:22:31 UTC 
Isaac suggested to drop the patches for 4.12 and 4.11, however the patch for 4.13 is really needed. If you do 'net ads join' and the ldap server doesn't respond within 0.1s, the connection fails.
Comment 21 Isaac Boukris 2020-12-04 10:29:05 UTC 
(In reply to Andreas Schneider from comment #20)

Yeah, there is no sense in fixing TLS only to get it removed in the next release, I've -1 the irrelevant patches IMO.

I guess I should have opened another bug for the timeout, sorry about that.
Comment 22 Karolin Seeger 2020-12-08 09:19:28 UTC 
Pushed to autobuild-v4-13-test.
Comment 23 Samba QA Contact 2020-12-08 10:22:04 UTC 
This bug was referenced in samba v4-13-test:

f7490ec9d94edfc9cdc79e70580b3b226a2022d5
Comment 24 Karolin Seeger 2020-12-09 07:49:59 UTC 
Pushed to v4-13-test.
Closing out bug report.

Thanks!
Comment 25 Samba QA Contact 2020-12-16 12:21:19 UTC 
This bug was referenced in samba v4-13-stable (Release samba-4.13.3):

f7490ec9d94edfc9cdc79e70580b3b226a2022d5