Hi OpenLDAP team,
I am investigating writing a module to perform certificate validation as has been discussed in previous posts (See the "Proposal to strengthen slapd EXTERNAL authentication" thread on the openldap-technical@openldap.org list).
The idea is to inspect the subject name on the client certificate as soon as possible after TLS is established and close the connection if it does not match some predefined pattern. This would allow client certificates signed by public CAs to be used safely with slapd.
Looking through the code, I see that dnX509peerNormalize() is called almost immediately after the TLS is established and that it may be handled by a callable handler installed by the register_certificate_map_function() entry point. This would be an ideal place to inspect the certificate. The only problem being it that there is no way to "reject" a certificate and force the connection to be closed.
It may be possible to use the ssl context passed into the dnX509peerNormalize() function to close the connection but this would not be very clean and likely have undesirable side effects. What would be good is if dnX509peerNormalize() could return a particular error code to signal that the connection should be immediately closed.
I see that LDAP_INVALID_CREDENTIALS is already used to signal benign invalid credentials.
Maybe a new error code is required. something like "LDAP_HOSTILE_CREDENTIALS".
connection.c: connection_read() could then detect this and close the connection in an orderly fashion by calling
connection_closing( c, "TLS Client certificate rejected" ); connection_close( c );
This would seem to be a generally useful thing to have in the slapd core. Is this something the OpenLDAP project would consider doing?
Sean.
On Thu, Aug 03, 2023 at 10:00:37AM +1000, Sean Gallagher wrote:
Looking through the code, I see that dnX509peerNormalize() is called almost immediately after the TLS is established and that it may be handled by a callable handler installed by the register_certificate_map_function() entry point. This would be an ideal place to inspect the certificate. The only problem being it that there is no way to "reject" a certificate and force the connection to be closed.
It may be possible to use the ssl context passed into the dnX509peerNormalize() function to close the connection but this would not be very clean and likely have undesirable side effects. What would be good is if dnX509peerNormalize() could return a particular error code to signal that the connection should be immediately closed.
Calling SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) sounds like it should do the trick? Next read will fail and so you never receive data that you consider "hostile"?
I see that LDAP_INVALID_CREDENTIALS is already used to signal benign invalid credentials.
Maybe a new error code is required. something like "LDAP_HOSTILE_CREDENTIALS".
Right now there is no way to tell whether a failure to extract a client DN is because there isn't any or that it's invalid. Someone would have to clean all the relevant code up to make this possible.
LDAP_INVALID_CREDENTIALS corresponds to the "invalidCredentials (49)" resultCode as per RFC 4511 and is used in Bind handling, we're not at Bind time yet. If you want to prevent anonymous connections from doing anything, you might want to consider writing an overlay and attaching it to a DB or even globally.
This would seem to be a generally useful thing to have in the slapd core. Is this something the OpenLDAP project would consider doing?
Touching that code would probably tie into an overhaul of the entire SASL Bind identity support (EXTERNAL and otherwise): how librewrite comes into the picture, etc. Unless there is broad consensus on how that would look and work, I don't feel like that is going to happen any time soon? So 2.7 doesn't look like it would include this kind of change.
Regards,
On 4/08/2023 2:04 am, Ondřej Kuzník wrote:
On Thu, Aug 03, 2023 at 10:00:37AM +1000, Sean Gallagher wrote:
Looking through the code, I see that dnX509peerNormalize() is called almost immediately after the TLS is established and that it may be handled by a callable handler installed by the register_certificate_map_function() entry point. This would be an ideal place to inspect the certificate. The only problem being it that there is no way to "reject" a certificate and force the connection to be closed.
It may be possible to use the ssl context passed into the dnX509peerNormalize() function to close the connection but this would not be very clean and likely have undesirable side effects. What would be good is if dnX509peerNormalize() could return a particular error code to signal that the connection should be immediately closed.
Calling SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) sounds like it should do the trick? Next read will fail and so you never receive data that you consider "hostile"?
That sounds promising. Thanks. I might throw together some proof of concept and see if it works.
Sean.
On 4/08/2023 6:30 am, Sean Gallagher wrote:
On 4/08/2023 2:04 am, Ondřej Kuzník wrote:
On Thu, Aug 03, 2023 at 10:00:37AM +1000, Sean Gallagher wrote:
Looking through the code, I see that dnX509peerNormalize() is called almost immediately after the TLS is established and that it may be handled by a callable handler installed by the register_certificate_map_function() entry point. This would be an ideal place to inspect the certificate. The only problem being it that there is no way to "reject" a certificate and force the connection to be closed.
It may be possible to use the ssl context passed into the dnX509peerNormalize() function to close the connection but this would not be very clean and likely have undesirable side effects. What would be good is if dnX509peerNormalize() could return a particular error code to signal that the connection should be immediately closed.
Calling SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) sounds like it should do the trick? Next read will fail and so you never receive data that you consider "hostile"?
That sounds promising. Thanks. I might throw together some proof of concept and see if it works.
Sean.
Thinking it through, I see a few problems..
1) slapd doesn't call SSL directly, all calls seem to go through libldap - which implements a pluggable TLS switch. calling SSL directly is risky.
2) SSL_set_shutdown() doesn't seem to block reads.
3) It's not clear if the filehandle actually gets released. this could open up to a trivial DOS attack by exhausting the file handles. granted, this is a risk anyway.
Could I put forward another idea - a change to core that is less likely to lead to an avalanche of mods and is not externally visible. Something like:
int alien_credentials = 0; rc = dnX509peerNormalize( ssl, &authid, &alien_credentials ); if ( alien_credentials ) { connection_closing( c, "TLS alien client certificate rejected" ); connection_close( c ); connection_return( c ); return 0; }
Any existing uses of dnX509peerNormalize() would be unaware of the extra parameter and just ignore it.
Sean.
I was thinking more about what would be required to provide the necessary hook in the main code. Here is a sketch of following the idea through.
I have maintained all the existing entry points and their current semantics in case any modules out there call them. I would be the first to admit this looks ugly. It also occurred to me that it doesn't make much sense. If it were possible to add a hook to the core slapd code, it would be simpler to just add another callback to validate the client cert.
But yeah, allowing modules to call back into the core without the discipline of a formal API _really_ ossifies the core quickly!
Sean.
|==========================================================================================|| ||--- proto-slap.h.orig|| ||+++ proto-slap.h|| ||@@ -994,6 +994,7 @@|| || LDAP_SLAPD_F (int) dnX509normalize LDAP_P(( void *x509_name, struct berval *out ));|| |||| || LDAP_SLAPD_F (int) dnX509peerNormalize LDAP_P(( void *ssl, struct berval *dn ));|| ||+LDAP_SLAPD_F (int) dnX509peerNormalize3 LDAP_P(( void *ssl, struct berval *dn, int *aliencert ));|| |||| || LDAP_SLAPD_F (int) dnPrettyNormalDN LDAP_P(( Syntax *syntax, struct berval *val, LDAPDN *dn, int flags, void *ctx ));|| || #define dnPrettyDN(syntax, val, dn, ctx) || ||@@ -1003,6 +1004,9 @@|| |||| || typedef int (SLAP_CERT_MAP_FN) LDAP_P(( void *ssl, struct berval *dn ));|| || LDAP_SLAPD_F (int) register_certificate_map_function LDAP_P(( SLAP_CERT_MAP_FN *fn ));|| ||+|| ||+typedef int (SLAP_CERT_MAP_FN3) LDAP_P(( void *ssl, struct berval *dn, int *alientcert ));|| ||+LDAP_SLAPD_F (int) register_certificate_map_function3 LDAP_P(( SLAP_CERT_MAP_FN3 *fn ));|| |||| || /*|| || * entry.c|| ||==========================================================================================|| ||--- connection.c.orig|| ||+++ connection.c|| ||@@ -1369,7 +1369,17 @@|| || c->c_ssf = c->c_tls_ssf;|| || }|| |||| ||- rc = dnX509peerNormalize( ssl, &authid );|| ||+ int aliencert = 0;|| ||+ rc = dnX509peerNormalize3( ssl, &authid, &aliencert );|| ||+ if ( alientcert ) {|| ||+ Debug( LDAP_DEBUG_TRACE, "connection_read(%d): " || ||+ "TLS client certificate looks alien, error=%d id=%lu\n",|| ||+ s, rc, c->c_connid );|| ||+ connection_closing( c, "TLS alien client certificate rejected" ); || ||+ connection_close( c ); || ||+ connection_return( c );|| ||+ return 0; || ||+ } || || if ( rc != LDAP_SUCCESS ) {|| || Debug( LDAP_DEBUG_TRACE, "connection_read(%d): "|| || "unable to get TLS client DN, error=%d id=%lu\n",|| || ||==========================================================================================|| ||--- dn.c.orig|| ||+++ dn.c|| ||@@ -1295,6 +1295,11 @@|| || return -1;|| || }|| |||| ||+int register_certificate_map_function3(SLAP_CERT_MAP_FN3 *fn)|| ||+{|| ||+ return register_certificate_map_function((SLAP_CERT_MAP_FN *)fn);|| ||+}|| ||+|| || /*|| || * Convert an X.509 DN into a normalized LDAP DN|| || */|| ||@@ -1318,16 +1323,32 @@|| || int|| || dnX509peerNormalize( void *ssl, struct berval *dn )|| || {|| ||- int rc = LDAP_INVALID_CREDENTIALS;|| ||+ int alientcert = 0;|| |||| ||- if ( DNX509PeerNormalizeCertMap != NULL )|| ||- rc = (*DNX509PeerNormalizeCertMap)( ssl, dn );|| ||+ int rc = dnX509peerNormalize3( ssl, dn, &alientcert )|| |||| ||- if ( rc != LDAP_SUCCESS ) {|| ||- rc = ldap_pvt_tls_get_peer_dn( ssl, dn,|| ||- (LDAPDN_rewrite_dummy *)LDAPDN_rewrite, 0 );|| ||- }|| ||+ /* mimic behaviour of original 2-parameter fn */|| ||+ if ( aliencert && rc != LDAP_SUCCESS ) {|| ||+ rc = ldap_pvt_tls_get_peer_dn( ssl, dn,|| ||+ (LDAPDN_rewrite_dummy *)LDAPDN_rewrite, 0 );|| ||+ }|| |||| ||- return rc;|| ||+ return rc;|| ||+}|| ||+|| ||+int|| ||+dnX509peerNormalize3( void *ssl, struct berval *dn, int *alientcert )|| ||+{|| ||+ int rc = LDAP_INVALID_CREDENTIALS;|| ||+|| ||+ if ( DNX509PeerNormalizeCertMap != NULL )|| ||+ rc = (*DNX509PeerNormalizeCertMap)( ssl, dn, alientcert );|| ||+|| ||+ if ( !alientcert && rc != LDAP_SUCCESS ) {|| ||+ rc = ldap_pvt_tls_get_peer_dn( ssl, dn,|| ||+ (LDAPDN_rewrite_dummy *)LDAPDN_rewrite, 0 );|| ||+ }|| ||+|| ||+ return rc;|| || }|| || #endif|| ||==========================================================================================|| |
On 4/08/2023 10:47 am, Sean Gallagher wrote:
On 4/08/2023 6:30 am, Sean Gallagher wrote:
On 4/08/2023 2:04 am, Ondřej Kuzník wrote:
On Thu, Aug 03, 2023 at 10:00:37AM +1000, Sean Gallagher wrote:
Looking through the code, I see that dnX509peerNormalize() is called almost immediately after the TLS is established and that it may be handled by a callable handler installed by the register_certificate_map_function() entry point. This would be an ideal place to inspect the certificate. The only problem being it that there is no way to "reject" a certificate and force the connection to be closed.
It may be possible to use the ssl context passed into the dnX509peerNormalize() function to close the connection but this would not be very clean and likely have undesirable side effects. What would be good is if dnX509peerNormalize() could return a particular error code to signal that the connection should be immediately closed.
Calling SSL_set_shutdown(SSL_RECEIVED_SHUTDOWN) sounds like it should do the trick? Next read will fail and so you never receive data that you consider "hostile"?
That sounds promising. Thanks. I might throw together some proof of concept and see if it works.
Sean.
Thinking it through, I see a few problems..
- slapd doesn't call SSL directly, all calls seem to go through
libldap - which implements a pluggable TLS switch. calling SSL directly is risky.
SSL_set_shutdown() doesn't seem to block reads.
It's not clear if the filehandle actually gets released. this could
open up to a trivial DOS attack by exhausting the file handles. granted, this is a risk anyway.
Could I put forward another idea - a change to core that is less likely to lead to an avalanche of mods and is not externally visible. Something like:
int alien_credentials = 0; rc = dnX509peerNormalize( ssl, &authid, &alien_credentials ); if ( alien_credentials ) { connection_closing( c, "TLS alien client certificate rejected" ); connection_close( c ); connection_return( c ); return 0; }
Any existing uses of dnX509peerNormalize() would be unaware of the extra parameter and just ignore it.
That one got a bit messy in the detail. Here is another idea with extremely low impact on any existing code. The idea is for the ||DNX509PeerNormalizeCertMap ||function to return an "auth_id" with NULL bv_val and non-zero bv_len.|||| This would be a non-sensical situation in typical code and would usually suggest a bug. It should never occur in code unaware of this functionallity and so, should be safe use.
||================================================================================ --- dn.c.orig +++ dn.c @@ -1323,6 +1323,9 @@ if ( DNX509PeerNormalizeCertMap != NULL ) rc = (*DNX509PeerNormalizeCertMap)( ssl, dn );
+ /* special case for "alien" certificate detection */ + if ( dn->bv_len != 0 && dn->bv_val == NULL ) return rc; + if ( rc != LDAP_SUCCESS ) { rc = ldap_pvt_tls_get_peer_dn( ssl, dn, (LDAPDN_rewrite_dummy *)LDAPDN_rewrite, 0 ); |================================================================================ --- connection.c.orig +++ connection.c @@ -1375,6 +1375,16 @@ "unable to get TLS client DN, error=%d id=%lu\n", s, rc, c->c_connid ); } + if ( authid.bv_len && !authid.bv_val ) + Debug( LDAP_DEBUG_TRACE, "connection_read(%d): " + "TLS client certificate looks alien, code=%d id=%lu\n", + s, authid.bv_len, c->c_connid ); + /* c_mutex is locked */ + connection_closing( c, "TLS alien client certificate rejected" ); + connection_close( c ); + connection_return( c ); + return 0 + } sprintf(msgbuf, "tls_ssf=%u ssf=%u", c->c_tls_ssf, c->c_ssf); Debug( LDAP_DEBUG_STATS, "conn=%lu fd=%d TLS established %s tls_proto=%s tls_cipher=%s\n", ================================================================================ |
Sean||.
openldap-technical@openldap.org
-
Ondřej Kuzník -
Sean Gallagher