Fix #17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden #17940

remicollet · 2025-02-26T16:27:08Z

Alternative to #17939

robert-scheck · 2025-02-26T16:30:10Z

This is my personal favorite given the IMHO strange OpenLDAP API.

nielsdos

This is probably the better solution, at least better DX.

nielsdos · 2025-02-26T17:16:18Z

ext/ldap/ldap.c

@@ -987,6 +987,17 @@ PHP_FUNCTION(ldap_connect)
 			snprintf( url, urllen, "ldap://%s:" ZEND_LONG_FMT, host, port );
 		}

+#ifdef LDAP_OPT_X_TLS_NEWCTX
+		if (!memcmp(url, "ldaps:", 6)) {


Note that url can be NULL, because url = host and host is nullable.
Also I prefer if you use a normal string comparison rather than memcmp to avoid potential overreads. I don't know if that's possible (depends on the behaviour of ldap_is_ldap_url) but better safe than sorry and it wouldn't be really that much of a performance difference anyway.

robert-scheck · 2025-02-26T22:48:43Z

I am sorry, but as mentioned in #17776 (comment) I meanwhile noticed that this issue affects LDAPS (ldaps://) and STARTTLS (ldap://), so this pull request is unfortunately incomplete.

Most likely STARTTLS needs the following (untested!) adapted code block in PHP_FUNCTION(ldap_start_tls) after VERIFY_LDAP_LINK_CONNECTED(ld) but before calling ldap_start_tls_s(). In difference to LDAPS, STARTTLS starts with a plaintext connection and then upgrades the existing connection using STARTTLS, thus LDAP_OPT_X_TLS_NEWCTX should be applied to ld->link from my understanding. However, I'm not a C expert and also not sure about the OpenLDAP API…

#ifdef LDAP_OPT_X_TLS_NEWCTX
	int val = 0;

	/* ensure all pending TLS options are applied in a new context */
	if (ldap_set_option(ld->link, LDAP_OPT_X_TLS_NEWCTX, &val) != LDAP_OPT_SUCCESS) {
		php_error_docref(NULL, E_WARNING, "Could not create new security context");
	}
#endif

I also had a look to Postfix, there src/global/dict_ldap.c:580 uses quite similar code (they use the same code for LDAPS and STARTTLS, but their LDAP usage isn't directly comparable with PHP).

remicollet · 2025-02-27T06:29:50Z

@robert-scheck can you try this PR ?

It seems OK to me

Test LDAPS with LDAP_OPT_X_TLS_ALLOW
TLS certificate verification: Error, unable to get local issuer certificate
TLS certificate verification: Error, unable to verify the first certificate
LDAP bind succeeded (expected)

Test LDAPS with LDAP_OPT_X_TLS_DEMAND
TLS certificate verification: Error, unable to get local issuer certificate
TLS: can't connect: error:0A000086:SSL routines::certificate verify failed (unable to get local issuer certificate).

Warning: ldap_bind(): Unable to bind to server: Can't contact LDAP server in /work/build/phpmaster/bugldap.php on line 26
LDAP bind failed (expected)

Test STARTTLS with LDAP_OPT_X_TLS_ALLOW
TLS certificate verification: Error, unable to get local issuer certificate
TLS certificate verification: Error, unable to verify the first certificate
LDAP bind succeeded (expected)

Test STARTTLS with LDAP_OPT_X_TLS_DEMAND
TLS certificate verification: Error, unable to get local issuer certificate
TLS: can't connect: error:0A000086:SSL routines::certificate verify failed (unable to get local issuer certificate).

Warning: ldap_start_tls(): Unable to start TLS: Connect error in /work/build/phpmaster/bugldap.php on line 59

Warning: ldap_bind(): Unable to bind to server: Can't contact LDAP server in /work/build/phpmaster/bugldap.php on line 60
LDAP bind failed (expected)

remicollet · 2025-02-27T08:21:29Z

Sadly, don't understand why CI is failing on ext/ldap/tests/ldap_start_tls_basic.phpt :(

robert-scheck · 2025-02-27T13:19:25Z

@robert-scheck can you try this PR ?

It seems OK to me

Yes, commit 9dde13a followed by commit 6466ed7 work for me here as well - thank you. For the record:

$ php /tmp/ldap.php
Test LDAPS with LDAP_OPT_X_TLS_ALLOW
TLS certificate verification: Error, unable to get local issuer certificate
TLS certificate verification: Error, unable to verify the first certificate
LDAP bind succeeded (expected)

Test LDAPS with LDAP_OPT_X_TLS_DEMAND
TLS certificate verification: Error, unable to get local issuer certificate
TLS: can't connect: error:0A000086:SSL routines::certificate verify failed (unable to get local issuer certificate).
PHP Warning:  ldap_bind(): Unable to bind to server: Can't contact LDAP server in /tmp/ldap.php on line 26
LDAP bind failed (expected)

Test STARTTLS with LDAP_OPT_X_TLS_ALLOW
TLS certificate verification: Error, unable to get local issuer certificate
TLS certificate verification: Error, unable to verify the first certificate
LDAP bind succeeded (expected)

Test STARTTLS with LDAP_OPT_X_TLS_DEMAND
TLS certificate verification: Error, unable to get local issuer certificate
TLS: can't connect: error:0A000086:SSL routines::certificate verify failed (unable to get local issuer certificate).
PHP Warning:  ldap_start_tls(): Unable to start TLS: Connect error in /tmp/ldap.php on line 59
PHP Warning:  ldap_bind(): Unable to bind to server: Can't contact LDAP server in /tmp/ldap.php on line 60
LDAP bind failed (expected)
$

robert-scheck · 2025-02-27T13:33:26Z

Sadly, don't understand why CI is failing on ext/ldap/tests/ldap_start_tls_basic.phpt :(

Hard to say…if the CA of the certificate used for STARTTLS isn't part of the operating system trust store, I would expect it to fail. Or does the CI export something like LDAPTLS_REQCERT=… which influences the result? Local test using ldap_start_tls() leads to a failure here due to self-signed certificate, with and without this pull request applied (= as expected).

remicollet · 2025-02-27T14:56:39Z

Trying without any change, only for CI

more tests

remicollet · 2025-02-28T08:18:10Z

Summary:

We now have 2 tests for ldaps and start_tls checking

default config (no option), cert is checked
option can be changed (what this issue was about)

Locally, both passes, default config was OK without this PR, running on a sane env/config

Problems in CI

start_tls default config behavior change (previous was not checked)
ldaps test fails (cert never checked despite option)

Other eyes welcome

May be classified security issue?

robert-scheck

While I'm not a qualified reviewer for the PHP project, this pull request addresses #17776 for me with the technically correct result on the PHP side: Refuses untrusted SSL certificate by default, overriding LDAP_OPT_X_TLS_REQUIRE_CERT works for both, STARTTLS (ldap://) and LDAPS (ldaps://), no change/impact on plaintext LDAP connections.

For me, the previous behaviour (without this pull request) qualifies as a security flaw, because being not able to override LDAP_OPT_X_TLS_REQUIRE_CERT using PHP and not receiving any error for the failed override (let alone not being able to set LDAP_OPT_X_TLS_NEWCTX directly) may lead to unexpected MITM when working with (multiple) trusted and untrusted LDAPS connections. I admit that working with two or more LDAP connections in a PHP script and different values for LDAP_OPT_X_TLS_REQUIRE_CERT might not be the typical everyday usage.

And I personally prefer this approach over LDAP_OPT_X_TLS_NEWCTX in #17939, because the OpenLDAP API seems to be somewhat strange here, and this change just leads to the correct result for PHP developers, similar like for other bindings (I don't know if there is any other library binding requiring an equivalent to LDAP_OPT_X_TLS_NEWCTX).

What I can not comment on is the PHP specific part.

Last but not least: Thank really you very much, @remicollet!

nielsdos

Fix generally looks good to me , but no strong opinion on the target branch. A bit on the fence wrt the behaviour change. Not sure about security implications, I don't know enough about LDAP to judge that.

ext/ldap/ldap.c

remicollet · 2025-03-04T08:35:52Z

Ping @bukka @ericmann

bukka · 2025-03-04T09:49:28Z

I will need to investigate it as it's not immediately clear to me from a very quick look. I will try to find some time for it later today or tomorrow.

bukka · 2025-03-06T13:27:54Z

I have been looking into this and currently feel that it's more an API limitation and mainly a missing constant. I can get why this change is done and it might make sense but I'm a bit worried about the behavior change. I need to look more to the openldap source to see how this is handled in terms of resetting other options. I think we should be a bit careful here so I set some time for ldap to my weekly plan and will be looking into this more. I think I will have a bit better idea before the next release branching and will send some update here.

robert-scheck · 2025-03-06T13:55:24Z

[…] but I'm a bit worried about the behavior change

Am I missing something? If you don't use LDAP_OPT_X_TLS_… at all, I don't see any behaviour change in my manual tests. And if you do, I still don't see any behaviour change with a typical single LDAP connection in a PHP script (or multiple ones to the same server if you just connect, disconnect, reconnect for whatever reason). The only "behaviour change" that I see is if you use multiple LDAP connections with different a certificate verification expectation per connection by applying ldap_set_option(), that it IMHO behaves now like it always should have done (and what other extensions do correctly already). Yes, I admit that if you wrote sloppy PHP code using multiple ldap_set_option() that worked accidentally before, a subsequent LDAP connection might fail now "unexpectedly". But that's why I call the old behaviour a security flaw (compare e.g with two HTTPS calls, you would expect that certificate verification takes place there as you set it, right?) and something that should to be mentioned in the release notes.

I am very sorry if we're talking about two different things, but I really would like to understand what you are worried about exactly.

bukka · 2025-03-06T14:04:20Z

It's reseting the ctx (freeing the old one and creating a new one) that gets recreated from the currently saved options (combined with global ones) so there is a chance that there are some edge cases that we might be missing. I want to just analyse the impact of that to make sure that we are not going to miss anything and accidentaly break someones code.

remicollet · 2025-03-07T07:00:19Z

I don't see any behaviour change in my manual tests.

There is no behavior change on a modern distro (perhaps thanks to openldap 2.6 or good config).

The change was observed on CI (perhaps because of OpenLDAP 2.5 or poor config), where the only SSL test was failing, as cert check is now enabled by default, which makes sense.

bukka · 2025-03-07T11:05:00Z

@remicollet So why do you even ask RM's if you are so confident that there is no behaviour change and no risk? :)

bukka · 2025-03-07T11:08:33Z

If you feel that there is a security risk, you should open a new adivsory and not try to discuss it here - that should be always evaluated in private.

bukka · 2025-03-07T11:10:07Z

It's also because security is not a question for RM but for the security team.

bukka · 2025-03-07T11:20:57Z

In terms of older versions, I can see that 2.4 is still in Ubuntu 20.04 which is likely still used (even though support ends next month or so but think there is an extended one. I see also that 2.5 is in Ubuntu 22.04 which will be even more in use. So it sounds we need to make sure that it works there fine as well.

bukka · 2025-03-07T11:23:44Z

I think I got it. I guess you were asking RM's because of the potential change on those versions. I will check the older versions in that case more. Thanks for the hint.

bukka · 2025-03-07T11:26:41Z

As the security impact was mentioned here, I will then look into it as well and will open a new advisory if I feel, there is anything so it can be discussed further. But please try to not mention security related questions publicly in the future. As I said, if you have feeling that something might have security impact, just create a new advisory in the future.

remicollet · 2025-03-10T14:44:56Z

Previous tests run on Fedora with openldap 2.6.8

New set of tests on RHEL-8.10 with openldap 2.4.46 and Fedora with manually installed openldap 2.5.19

In all cases:

no behavior change (cert is checked by default)
bug reproduced
bug fixed by this PR

github-actions bot added the Extension: ldap label Feb 26, 2025

This was referenced Feb 26, 2025

add LDAP_OPT_X_TLS_NEWCTX constant #17939

Closed

ldap_set_option(null, LDAP_OPT_X_TLS_REQUIRE_CERT, $option) can't be overridden #17776

Open

nielsdos requested changes Feb 26, 2025

View reviewed changes

remicollet force-pushed the issue-ldap-newctx2 branch from eb25825 to 9dde13a Compare February 27, 2025 06:28

remicollet force-pushed the issue-ldap-newctx2 branch from 2226d2f to db4cd76 Compare February 27, 2025 14:54

Fix php#17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden

2d79174

remicollet force-pushed the issue-ldap-newctx2 branch from 55b6a46 to 2d79174 Compare February 27, 2025 15:31

remicollet marked this pull request as draft February 27, 2025 15:46

remicollet force-pushed the issue-ldap-newctx2 branch 2 times, most recently from 55ff888 to 881a9fa Compare February 27, 2025 16:42

Don't check self signed cert

2966b09

more tests

remicollet force-pushed the issue-ldap-newctx2 branch from 4cb7cd4 to 2966b09 Compare February 28, 2025 07:39

remicollet requested a review from nielsdos February 28, 2025 08:18

remicollet marked this pull request as ready for review February 28, 2025 08:19

robert-scheck reviewed Feb 28, 2025

View reviewed changes

nielsdos approved these changes Mar 2, 2025

View reviewed changes

ext/ldap/ldap.c Show resolved Hide resolved

ldap_connect fails if ldap_set_option fails

665e79c

remicollet added the Waiting on RM label Mar 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden #17940

Fix #17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden #17940

remicollet commented Feb 26, 2025

robert-scheck commented Feb 26, 2025

nielsdos left a comment

nielsdos Feb 26, 2025

remicollet Feb 27, 2025

robert-scheck commented Feb 26, 2025 •

edited

Loading

remicollet commented Feb 27, 2025 •

edited

Loading

remicollet commented Feb 27, 2025

robert-scheck commented Feb 27, 2025

robert-scheck commented Feb 27, 2025

remicollet commented Feb 27, 2025

remicollet commented Feb 28, 2025

robert-scheck left a comment

nielsdos left a comment

remicollet commented Mar 4, 2025

bukka commented Mar 4, 2025

bukka commented Mar 6, 2025

robert-scheck commented Mar 6, 2025

bukka commented Mar 6, 2025

remicollet commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025 •

edited

Loading

bukka commented Mar 7, 2025 •

edited

Loading

bukka commented Mar 7, 2025

remicollet commented Mar 10, 2025 •

edited

Loading

Fix #17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden #17940

Are you sure you want to change the base?

Fix #17776 LDAP_OPT_X_TLS_REQUIRE_CERT can't be overridden #17940

Conversation

remicollet commented Feb 26, 2025

robert-scheck commented Feb 26, 2025

nielsdos left a comment

Choose a reason for hiding this comment

nielsdos Feb 26, 2025

Choose a reason for hiding this comment

remicollet Feb 27, 2025

Choose a reason for hiding this comment

robert-scheck commented Feb 26, 2025 • edited Loading

remicollet commented Feb 27, 2025 • edited Loading

remicollet commented Feb 27, 2025

robert-scheck commented Feb 27, 2025

robert-scheck commented Feb 27, 2025

remicollet commented Feb 27, 2025

remicollet commented Feb 28, 2025

robert-scheck left a comment

Choose a reason for hiding this comment

nielsdos left a comment

Choose a reason for hiding this comment

remicollet commented Mar 4, 2025

bukka commented Mar 4, 2025

bukka commented Mar 6, 2025

robert-scheck commented Mar 6, 2025

bukka commented Mar 6, 2025

remicollet commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025

bukka commented Mar 7, 2025 • edited Loading

bukka commented Mar 7, 2025 • edited Loading

bukka commented Mar 7, 2025

remicollet commented Mar 10, 2025 • edited Loading

robert-scheck commented Feb 26, 2025 •

edited

Loading

remicollet commented Feb 27, 2025 •

edited

Loading

bukka commented Mar 7, 2025 •

edited

Loading

bukka commented Mar 7, 2025 •

edited

Loading

remicollet commented Mar 10, 2025 •

edited

Loading