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..
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.
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||.
--
This email has been checked for viruses by AVG antivirus software.
www.avg.com
