diff mbox series

[v1,2/3] HostAPD: Add reload crl support

Message ID 1538417575-35315-2-git-send-email-jared.bents@rockwellcollins.com
State Superseded
Headers show
Series [v1,1/3] HostAPD: Add option 'check_crl_strict' | expand

Commit Message

Jared Bents Oct. 1, 2018, 6:12 p.m. UTC 
This patch has been added new flag 'crl_reload_interval' to reload crl. This can
be used to reload ca_cert file on every new tls session if
difference between last reload and current reload time(seconds) greater-than
crl_reload_interval.

Note: If interval time > 0 and < 300 then crl_reload_interval will reset to
300 seconds. For this support 'check_crl' should be 1 or 2.
Here, We have selected 300 seconds as a reset value to reduce overhead of crl
reload funtionality on new session. We can choose less than 300 seconds but it
will increase overhead of crl reload on new session.

Signed-off-by: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com>
Signed-off-by: Sam Voss <sam.voss at rockwellcollins.com>
Signed-off-by: Jared Bents <jared.bents@rockwellcollins.com>
---
 hostapd/config_file.c    |  2 +
 hostapd/hostapd.conf     |  8 ++++
 src/ap/ap_config.h       |  1 +
 src/ap/authsrv.c         |  8 ++++
 src/crypto/tls.h         | 11 ++++++
 src/crypto/tls_openssl.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 125 insertions(+), 2 deletions(-)

Comments

Jouni Malinen Jan. 2, 2019, 4:36 p.m. UTC | #1 
On Mon, Oct 01, 2018 at 01:12:54PM -0500, Jared Bents wrote:
> This patch has been added new flag 'crl_reload_interval' to reload crl. This can
> be used to reload ca_cert file on every new tls session if
> difference between last reload and current reload time(seconds) greater-than
> crl_reload_interval.
> 
> Note: If interval time > 0 and < 300 then crl_reload_interval will reset to
> 300 seconds. For this support 'check_crl' should be 1 or 2.
> Here, We have selected 300 seconds as a reset value to reduce overhead of crl
> reload funtionality on new session. We can choose less than 300 seconds but it
> will increase overhead of crl reload on new session.

This sounds like a reasonable thing to do, but couple of comments on the
implementation details:

> diff --git a/src/crypto/tls.h b/src/crypto/tls.h
>  #ifndef TLS_H
>  #define TLS_H
> +#include <openssl/x509v3.h>

This tls.h file needs to be independent of which crypto/TLS library is
used, so it cannot pull in OpenSSL header files.

> +X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl);

Or define something to return an OpenSSL specific data structure.

That said, it does not look like either of these changes are really
needed in tls.h since that tls_crl_cert_reload() is not called from
anywhere outside tls_openssl.c. In other words, that function should be
marked static in tls_openssl.c and not declared in tls.h.

> diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
> +#include <pthread.h>

Could you please clarify why this would need pthread mutex support? What
is that trying to protect against? hostapd is a single-threaded
process, so it is not immediately obvious what this is trying to do.
Either remove this unnecessary mutex or document why it is needed.

> @@ -189,6 +190,8 @@ struct tls_context {
>  	int cert_in_cb;
>  	char *ocsp_stapling_response;
>  	int check_crl_strict;
> +	int check_crl;
> +	const char *ca_cert;
>  };

These should more likely be added into struct tls_data which is also
where I moved the check_crl_strict in an earlier patch.

> @@ -968,8 +975,19 @@ void * tls_init(const struct tls_config *conf)

> +		data->crl_reload_interval  = conf->crl_reload_interval;
> +		/* Set crl_reload_interval is equal to 5 minutes to reduce overhead of
> +		 * crl reload funtionality on new session. We can choose less than 5
> +		 * minutes but it will increase overhead of crl reload on new session.
> +		 * */
> +		if(data->crl_reload_interval > 0 && data->crl_reload_interval < 300) {
> +			 wpa_printf(MSG_INFO,
> +					"crl reload interval is set too low, reset it to 300");
> +			 data->crl_reload_interval = 300;
> +		}
> +	}

Is there a strong need to enforce this 300 second? That makes it
inconvenient to test this functionality since any automated test case
would need to last at least five minutes to hit this limit.

> @@ -1333,8 +1355,39 @@ struct tls_connection * tls_connection_init(void *ssl_ctx)
> +	time_t now;

> +	/* Get current time */
> +	now = time(NULL);

Please use os_get_reltime() instead.

> +	/* Replace X509 store if it is time to update crl */
> +	/* Replace X509 store if difference between current time and previous store
> +	 * reload time greater-than crl_reload_interval */
> +	if (data->crl_reload_interval > 0 && data->crl_last_reload +
> +					data->crl_reload_interval <= now) {

So os_reltime_*() helpers here, e.g., with os_reltime_expired().

> +		pthread_mutex_lock(&data->mutex);
> +		/* recheck data->crl_last_reload because it may be inaccurate
> +		 * without mutex */

Why? What could have modified it without mutex in a single-threaded
process?

> +X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl)

This is the function that should be static within tls_openssl.c.
diff mbox series

Patch

diff --git a/hostapd/config_file.c b/hostapd/config_file.c
index 7b7b33f..f5b8e83 100644
--- a/hostapd/config_file.c
+++ b/hostapd/config_file.c
@@ -2133,6 +2133,8 @@  static int hostapd_config_fill(struct hostapd_config *conf,
 		bss->check_crl = atoi(pos);
 	} else if (os_strcmp(buf, "check_crl_strict") == 0) {
 		bss->check_crl_strict = atoi(pos);
+	} else if (os_strcmp(buf, "crl_reload_interval") == 0) {
+		bss->crl_reload_interval = atoi(pos);
 	} else if (os_strcmp(buf, "tls_session_lifetime") == 0) {
 		bss->tls_session_lifetime = atoi(pos);
 	} else if (os_strcmp(buf, "ocsp_stapling_response") == 0) {
diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf
index bc56f8d..c9c6e41 100644
--- a/hostapd/hostapd.conf
+++ b/hostapd/hostapd.conf
@@ -802,6 +802,14 @@  eap_server=0
 # 1 = do not ignore errors (default)
 #check_crl_strict=0
 
+# crl reload interval in seconds
+# This can be used to reload ca_cert file on every new tls session if difference
+# between last reload and current reload time(seconds) greater-than
+# crl_reload_interval
+# Note: If interval time > 0 and < 300 then crl_reload_interval will reset to
+# 300 seconds. For this support 'check_crl' should be 1 or 2.
+# 0 = do not reload CRLS (default)
+# crl_reload_interval = 300
 
 # TLS Session Lifetime in seconds
 # This can be used to allow TLS sessions to be cached and resumed with an
diff --git a/src/ap/ap_config.h b/src/ap/ap_config.h
index 6220185..fef966c 100644
--- a/src/ap/ap_config.h
+++ b/src/ap/ap_config.h
@@ -353,6 +353,7 @@  struct hostapd_bss_config {
 	char *private_key_passwd;
 	int check_crl;
 	int check_crl_strict;
+	unsigned int crl_reload_interval;
 	unsigned int tls_session_lifetime;
 	char *ocsp_stapling_response;
 	char *ocsp_stapling_response_multi;
diff --git a/src/ap/authsrv.c b/src/ap/authsrv.c
index 62ddc87..2add858 100644
--- a/src/ap/authsrv.c
+++ b/src/ap/authsrv.c
@@ -157,6 +157,14 @@  int authsrv_init(struct hostapd_data *hapd)
 
 		os_memset(&conf, 0, sizeof(conf));
 		conf.tls_session_lifetime = hapd->conf->tls_session_lifetime;
+		if(hapd->conf->crl_reload_interval > 0 && hapd->conf->check_crl <= 0) {
+			wpa_printf(MSG_INFO, "Failed to enable crl reload functionality,"
+				   "It's depend on check_crl.");
+		}
+		if(hapd->conf->check_crl > 0 && hapd->conf->crl_reload_interval > 0){
+			conf.crl_reload_interval = hapd->conf->crl_reload_interval;
+			wpa_printf(MSG_INFO, "Enabled crl reload functionality");
+		}
 		hapd->ssl_ctx = tls_init(&conf);
 		if (hapd->ssl_ctx == NULL) {
 			wpa_printf(MSG_ERROR, "Failed to initialize TLS");
diff --git a/src/crypto/tls.h b/src/crypto/tls.h
index bb497ce..85b6a5e 100644
--- a/src/crypto/tls.h
+++ b/src/crypto/tls.h
@@ -8,6 +8,7 @@ 
 
 #ifndef TLS_H
 #define TLS_H
+#include <openssl/x509v3.h>
 
 struct tls_connection;
 
@@ -80,6 +81,7 @@  struct tls_config {
 	int cert_in_cb;
 	const char *openssl_ciphers;
 	unsigned int tls_session_lifetime;
+	unsigned int crl_reload_interval;
 
 	void (*event_cb)(void *ctx, enum tls_event ev,
 			 union tls_event_data *data);
@@ -224,6 +226,15 @@  void tls_deinit(void *tls_ctx);
 int tls_get_errors(void *tls_ctx);
 
 /**
+ * tls_crl_cert_reload - Reload crl cert and init new store
+ * @ca_cert : ca_cert file
+ * @check_crl: 0 = do not verify CRLs, 1 = verify CRL for the user certificate,
+ * 2 = verify CRL for all certificates
+ * Returns : store pointer on success and NULL on failure
+ */
+X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl);
+
+/**
  * tls_connection_init - Initialize a new TLS connection
  * @tls_ctx: TLS context data from tls_init()
  * Returns: Connection context data, conn for other function calls
diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index 990c938..41fbf1b 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -7,6 +7,7 @@ 
  */
 
 #include "includes.h"
+#include <pthread.h>
 
 #ifndef CONFIG_SMARTCARD
 #ifndef OPENSSL_NO_ENGINE
@@ -189,6 +190,8 @@  struct tls_context {
 	int cert_in_cb;
 	char *ocsp_stapling_response;
 	int check_crl_strict;
+	int check_crl;
+	const char *ca_cert;
 };
 
 static struct tls_context *tls_global = NULL;
@@ -197,6 +200,10 @@  static struct tls_context *tls_global = NULL;
 struct tls_data {
 	SSL_CTX *ssl;
 	unsigned int tls_session_lifetime;
+	unsigned int crl_reload_interval;
+	unsigned int crl_last_reload;
+	X509_STORE   *old_x509_store;
+	pthread_mutex_t	mutex;
 };
 
 struct tls_connection {
@@ -968,8 +975,19 @@  void * tls_init(const struct tls_config *conf)
 		return NULL;
 	}
 	data->ssl = ssl;
-	if (conf)
+	if (conf) {
 		data->tls_session_lifetime = conf->tls_session_lifetime;
+		data->crl_reload_interval  = conf->crl_reload_interval;
+		/* Set crl_reload_interval is equal to 5 minutes to reduce overhead of
+		 * crl reload funtionality on new session. We can choose less than 5
+		 * minutes but it will increase overhead of crl reload on new session.
+		 * */
+		if(data->crl_reload_interval > 0 && data->crl_reload_interval < 300) {
+			 wpa_printf(MSG_INFO,
+					"crl reload interval is set too low, reset it to 300");
+			 data->crl_reload_interval = 300;
+		}
+	}
 
 	SSL_CTX_set_options(ssl, SSL_OP_NO_SSLv2);
 	SSL_CTX_set_options(ssl, SSL_OP_NO_SSLv3);
@@ -1028,6 +1046,9 @@  void * tls_init(const struct tls_config *conf)
 		return NULL;
 	}
 
+	/* Init mutex */
+	pthread_mutex_init(&data->mutex, NULL);
+
 	return data;
 }
 
@@ -1060,6 +1081,7 @@  void tls_deinit(void *ssl_ctx)
 		tls_global = NULL;
 	}
 
+	pthread_mutex_destroy(&data->mutex);
 	os_free(data);
 }
 
@@ -1333,8 +1355,39 @@  struct tls_connection * tls_connection_init(void *ssl_ctx)
 	SSL_CTX *ssl = data->ssl;
 	struct tls_connection *conn;
 	long options;
+	X509_STORE	*new_cert_store;
+	time_t now;
 	struct tls_context *context = SSL_CTX_get_app_data(ssl);
 
+	/* Get current time */
+	now = time(NULL);
+
+	/* Replace X509 store if it is time to update crl */
+	/* Replace X509 store if difference between current time and previous store
+	 * reload time greater-than crl_reload_interval */
+	if (data->crl_reload_interval > 0 && data->crl_last_reload +
+					data->crl_reload_interval <= now) {
+		pthread_mutex_lock(&data->mutex);
+		/* recheck data->crl_last_reload because it may be inaccurate
+		 * without mutex */
+		if (data->crl_last_reload + data->crl_reload_interval <= now) {
+			wpa_printf(MSG_INFO, "Flushing X509 store with ca_cert file");
+			new_cert_store = (X509_STORE *)tls_crl_cert_reload
+					(tls_global->ca_cert, tls_global->check_crl);
+			if (new_cert_store == NULL) {
+				wpa_printf(MSG_ERROR,
+						"Error replacing X509 store with ca_cert file");
+			} else {
+				/*Free old store */
+				if (data->old_x509_store) X509_STORE_free(data->old_x509_store);
+				data->old_x509_store = ssl->cert_store;
+				ssl->cert_store = new_cert_store;
+				data->crl_last_reload = now;
+			}
+		}
+		pthread_mutex_unlock(&data->mutex);
+	}
+
 	conn = os_zalloc(sizeof(*conn));
 	if (conn == NULL)
 		return NULL;
@@ -2167,6 +2220,35 @@  static int tls_connection_ca_cert(struct tls_data *data,
 	return 0;
 }
 
+X509_STORE *tls_crl_cert_reload(const char *ca_cert, int check_crl)
+{
+	int flags;
+	X509_STORE *store;
+
+	store = X509_STORE_new();
+	if (store == NULL) {
+		wpa_printf(MSG_DEBUG, "OpenSSL: %s - failed to allocate new "
+			   "certificate store", __func__);
+		return NULL;
+	}
+
+	if (ca_cert) {
+		if(!X509_STORE_load_locations(store, ca_cert, NULL)) {
+			tls_show_errors(MSG_WARNING, __func__,
+					"Failed to load root certificates");
+			return NULL;
+		}
+	}
+
+	if (check_crl)
+		flags = X509_V_FLAG_CRL_CHECK;
+	if (check_crl == 2)
+		flags |= X509_V_FLAG_CRL_CHECK_ALL;
+
+	 X509_STORE_set_flags(store, flags);
+
+	return store;
+}
 
 static int tls_global_ca_cert(struct tls_data *data, const char *ca_cert)
 {
@@ -2188,6 +2270,13 @@  static int tls_global_ca_cert(struct tls_data *data, const char *ca_cert)
 		SSL_CTX_set_client_CA_list(ssl_ctx,
 					   SSL_load_client_CA_file(ca_cert));
 #endif /* OPENSSL_NO_STDIO */
+
+		if (NULL == tls_global) {
+			tls_show_errors(MSG_INFO, __func__, "Failed setting "
+					"ca_cert in tls_global context.");
+		} else {
+			tls_global->ca_cert = ca_cert;
+		}
 	}
 
 	return 0;
@@ -2216,11 +2305,15 @@  int tls_global_set_verify(void *ssl_ctx, int check_crl, int strict)
 
 		if (NULL == tls_global) {
 			tls_show_errors(MSG_INFO, __func__, "Failed setting "
-					"strict mode in tls_global context.");
+					"strict mode and check crl mode in tls_global context.");
 		} else {
 			tls_global->check_crl_strict = strict;
+			tls_global->check_crl = check_crl;
 		}
 
+		/* Store crl last reload time */
+		data->crl_last_reload = time(NULL);
+		data->old_x509_store = NULL;
 	}
 	return 0;
 }