From 8b79633a19d8f64c58641b5b693dea6f17b77aeb Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Mon, 26 Jul 2021 10:44:12 -0400 Subject: [PATCH] cleanups and fixes --- raddb/certs/realms/README.md | 21 +++++++++++++++++++-- src/main/listen.c | 17 +++++++++++++---- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/raddb/certs/realms/README.md b/raddb/certs/realms/README.md index f4dee04dc0..0922403f24 100644 --- a/raddb/certs/realms/README.md +++ b/raddb/certs/realms/README.md @@ -10,12 +10,12 @@ network access, the server can present an `example.com` certificate. On the other hand, when a user `doug@example.org` requests network access, the server cna present an `example.org` certificate. -This functionaltiy means that it is possible to configure only one +This functionality means that it is possible to configure only one `eap` module, and then use multiple certificate chains. Previous versions of the server required that the administrator configure multiple EAP modules, one for each certificate being used. -This functionality can be used in one of two ways. First, the +The selection can be performed in one of two ways. First, the certificates can be loaded dynamically at run-time. Second, the certificates can be pre-loaded for speed. @@ -195,3 +195,20 @@ a file which taken from `realm_dir` portion of the filename is configurable. The SNI portion is taken from the TLS messages, and the `.pem` suffix is hard-coded in the source code. + +Taking the filename from an untrusted source is fine here. The +standard (RFC 6066 Section 3) says that the Server Name Indication +field is a DNS "A label". Which means that there are a limited number +of characters allowed: + +* `.`, `-`, `a-Z`, `A-Z`, `0-9` + +If the SNI contain anything else, the TLS connection is rejected. + +Note that if session resumption is enabled for RadSec, the session +cache *must* also cache the `TLS-Server-Name-Indication` attribute. +The SNI is sent on resumption for TLS 1.2 and earlier, but it is not +sent for TLS 1.3. As such, the only way to select the right policy on +resumption is to check the value of the cached +TLS-Server-Name-Indication attribute. + diff --git a/src/main/listen.c b/src/main/listen.c index bce82801b2..57a627aa6d 100644 --- a/src/main/listen.c +++ b/src/main/listen.c @@ -656,17 +656,27 @@ static int tls_sni_callback(SSL *ssl, UNUSED int *al, void *arg) if ((*p >= '0') && (*p <= '9')) continue; /* - * Anything else, ignore it. + * Anything else, ignore fail. */ - return SSL_TLSEXT_ERR_OK; + return SSL_TLSEXT_ERR_ALERT_FATAL; } + /* + * Too long, fail. + */ + if ((p - name) > 255) return SSL_TLSEXT_ERR_ALERT_FATAL; + snprintf(buffer, sizeof(buffer), "%s/%s.pem", conf->realm_dir, name); my_r.name = buffer; r = fr_hash_table_finddata(conf->realms, &my_r); - if (!r) return SSL_TLSEXT_ERR_OK; + /* + * If found, switch certs. Otherwise use the default + * one. + */ + if (r) (void) SSL_set_SSL_CTX(ssl, r->ctx); + /* * Set an attribute saying which server has been selected. */ @@ -675,7 +685,6 @@ static int tls_sni_callback(SSL *ssl, UNUSED int *al, void *arg) (void) pair_make_config("TLS-Server-Name-Indication", name, T_OP_SET); } - (void) SSL_set_SSL_CTX(ssl, r->ctx); return SSL_TLSEXT_ERR_OK; } #endif -- 2.47.2