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

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).

@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)

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

@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)
$ 

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).

Trying without any change, only for CI

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?

Ping @bukka @ericmann

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.

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.

[...] 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.

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.

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.

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

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.

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

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.

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.

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.

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

So I have been looking through OpenLDAP source code (specifically tls2.c and tls_o.c). It took me a bit of time and didn't have time to test it but it seemed quite clear from the code - although I might be wrong somewhere.

I think I get why the API is the way it is. I guess the reason is primarily performance. It means not recreating global SSL_CTX for each connection which is really the purpose of SSL_CTX (possibility to re-use it). This makes sense specifically for loading the certs and keys. The idea is that the options (pretty much all the TLS ones - not just REQUIRE_CERT) need to be set before the first connection and then it cannot be changed by design. Specifically this is due this check: https://github.com/openldap/openldap/blob/72f184a0e3cb500ae4a8f81d212a5b6d9a6d1c91/libraries/libldap/tls2.c#L208-L209 . So you can see that this is definitely done on purpose and it makes some sense.

Theoretically there seems to be also a minor behaviour change in this regard if I'm not mistaken. Currently it is possible to set LDAP_OPT_X_TLS_CACERTFILE, then connect and then delete the file. This should then work for subsequent connections because it was loaded initially and ctx is still re-used. With this change it will no longer work because it will try to recreate the ctx but the file will no longer exist. I don't think this is really much of an issues in reality but maybe there is more.

Personally I see it more as a documentation issue and missing feature (in terms of not supported constant LDAP_OPT_X_TLS_REQUIRE_CERT). Basically the current contract is that the global options cannot be changed after the first connection. This is also how it works in OpenLDAP and I think we should keep the option to re-use the ctx between connections.

That said I'm actually wondering if there is a bug in not resetting the ctx between requests. I cannot really see anywhere in ldap ext code that there is any code that would reset the global context (e.g. in RINIT). If this is not there, then that should be, of course, addressed - e.g. by setting LDAP_OPT_X_TLS_CACERTFILE at the end of the request.

Indeed, re-using CTX is an optimization which saves around 4ms for connection (from 12ms with new context to 8ms with re-used context, in my local test)

That said I'm actually wondering if there is a bug in not resetting the ctx between requests.

Yes, there is a bug, I just checked (using php-fpm configured for a single worker)
The context will be re-used for all connections during the process life (so LDAP_OPT_X_TLS_REQUIRE_CERT is not honoured)

Indeed, resetting context in RINIT may be a way to fix it.
BTW, I really think resetting the context before each connection is the safer way

• optimization is very small (4ms)
• and multiple connections in the same script is probably a rare use case (so optimization not really useful)

Indeed, resetting context in RINIT may be a way to fix it.

I don't find any API to do this, so as LDAP_OPT_X_TLS_NEWCTX creates the context, it must be called after the set option calls and before the connect.

@bukka can you please reconsider this PR

Yeah so I think we should introduce global reset flag (bool variable) that would get set in RINIT to true and it would also get set to true if any global context setting is done. Then LDAP_OPT_X_TLS_NEWCTX would get set when creating connection if flag is set to true but after that, it would get set to false. So if there is a further connection created but there is no globals change, then the context would still get reused.

Yeah so I think we should introduce global reset flag (bool variable)

I don't think it worth the complexity to save a few ms

But: Done

Thanks for the review

Merged in 8.3+ as 389de7c and 98fb27a