[PATCH 1/3] Hoist loading default root certificates into ircConfig()

[PATCH 1/3] Hoist loading default root certificates into ircConfig()

From: Klemens Nanni
To: list+catgirl
Cc: Klemens Nanni
tls_connect_socket(3) in ircConnect() does that by default already
unless tls_load_file(3)/tls_config_set_ca_mem(3) was used.

Loading CA certificates before connecting makes no practical difference
except on OpenBSD where this allows for tighter unveil und pledge setups
now that all required (TLS related) file I/O is finished by the time
ircConnect() gets to do network I/O.

In case of the hidden `-!' insecure flag which is implied by `-o' to
print server certificates and exit, loading root certificates is not
required at all;  likewise, using explicit self signed server
certificates will not involve certificate authorities either, hence load
them only if needed.
---
 irc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/irc.c b/irc.c
index c98193a..92a70fe 100644
--- a/irc.c
+++ b/irc.c
@@ -71,6 +71,15 @@ void ircConfig(
 		if (error) errx(EX_NOINPUT, "%s: %s", trust, tls_config_error(config));
 	}
 
+	if (!insecure && !trust) {
+		size_t ca_sz;
+		uint8_t *ca = tls_load_file(tls_default_ca_cert_file(), &ca_sz, NULL);
+		if (!ca) errx(EX_SOFTWARE, "tls_load_file");
+		error = tls_config_set_ca_mem(config, ca, ca_sz);
+		if (error == -1) errx(EX_SOFTWARE, "tls_config_set_ca_mem");
+		tls_unload_file(ca, ca_sz);
+	}
+
 	if (cert) {
 		const char *dirs = NULL;
 		for (const char *path; NULL != (path = configPath(&dirs, cert));) {
-- 
2.32.0
1 reply

[PATCH 2/3] OpenBSD: Remove now obsolete unveil code

From: Klemens Nanni
To: list+catgirl
Cc: Klemens Nanni
Previous tls_default_ca_cert_file(3) hoisting makes this possible: all
TLS related files are fully loaded into memory by ircConfig() such that
ircConnect() will not do any file I/O.

Call ircConfig() before pledge(2) in the `-o' "print cert" case so this
works out -- that order should have been preserved in the previous
a989e15 "OpenBSD: hoist -o/printCert code to simplify" but fixing it now
nicely demonstrates the achivement even more so.
---
 chat.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/chat.c b/chat.c
index 653a6ab..18174ae 100644
--- a/chat.c
+++ b/chat.c
@@ -128,14 +128,6 @@ static void parseHash(char *str) {
 
 #ifdef __OpenBSD__
 
-static void unveilConfig(const char *name) {
-	const char *dirs = NULL;
-	for (const char *path; NULL != (path = configPath(&dirs, name));) {
-		int error = unveil(path, "r");
-		if (error && errno != ENOENT) err(EX_NOINPUT, "%s", path);
-	}
-}
-
 static void unveilData(const char *name) {
 	const char *dirs = NULL;
 	for (const char *path; NULL != (path = dataPath(&dirs, name));) {
@@ -144,25 +136,12 @@ static void unveilData(const char *name) {
 	}
 }
 
-static void unveilAll(const char *trust, const char *cert, const char *priv) {
+static void unveilAll(void) {
 	if (save || logEnable) {
 		dataMkdir("");
 		unveilData("");
 	}
-	if (trust) unveilConfig(trust);
-	if (cert) unveilConfig(cert);
-	if (priv) unveilConfig(priv);
 	if (save) unveilData(save);
-	struct {
-		const char *path;
-		const char *perm;
-	} paths[] = {
-		{ tls_default_ca_cert_file(), "r" },
-	};
-	for (size_t i = 0; i < ARRAY_LEN(paths); ++i) {
-		int error = unveil(paths[i].path, paths[i].perm);
-		if (error) err(EX_OSFILE, "%s", paths[i].path);
-	}
 }
 
 #endif /* __OpenBSD__ */
@@ -266,12 +245,11 @@ int main(int argc, char *argv[]) {
 	if (!host) errx(EX_USAGE, "host required");
 
 	if (printCert) {
+		ircConfig(insecure, trust, cert, priv);
 #ifdef __OpenBSD__
-		unveilAll(trust, cert, priv);
 		int error = pledge("stdio rpath inet dns", NULL);
 		if (error) err(EX_OSERR, "pledge");
 #endif
-		ircConfig(insecure, trust, cert, priv);
 		ircConnect(bind, host, port);
 		ircPrintCert();
 		ircClose();
@@ -310,7 +288,7 @@ int main(int argc, char *argv[]) {
 	uiInitEarly();
 
 #ifdef __OpenBSD__
-	if (self.restricted) unveilAll(trust, cert, priv);
+	if (self.restricted) unveilAll();
 
 	char promises[64] = "stdio tty";
 	char *ptr = &promises[strlen(promises)], *end = &promises[sizeof(promises)];
-- 
2.32.0
1 reply

[PATCH 3/3] OpenBSD: Drop now unneeded promise from initial pledge

From: Klemens Nanni
To: list+catgirl
Cc: Klemens Nanni
Both ssl(8) as well as ncurses(3) related files are now read completely
by the time of ircConfig() and uiInitEarly() respectively, so read
access to the filesystem is no longer needed at all unless the "log" or
"save" options are used.
---
 chat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/chat.c b/chat.c
index 18174ae..9a276d5 100644
--- a/chat.c
+++ b/chat.c
@@ -247,7 +247,7 @@ int main(int argc, char *argv[]) {
 	if (printCert) {
 		ircConfig(insecure, trust, cert, priv);
 #ifdef __OpenBSD__
-		int error = pledge("stdio rpath inet dns", NULL);
+		int error = pledge("stdio inet dns", NULL);
 		if (error) err(EX_OSERR, "pledge");
 #endif
 		ircConnect(bind, host, port);
@@ -298,7 +298,7 @@ int main(int argc, char *argv[]) {
 	char *promisesFinal = strdup(promises);
 	if (!promisesFinal) err(EX_OSERR, "strdup");
 
-	seprintf(ptr, end, " rpath inet dns");
+	seprintf(ptr, end, " inet dns");
 	int error = pledge(promises, NULL);
 	if (error) err(EX_OSERR, "pledge");
 #endif
-- 
2.32.0
3 replies

Re: [PATCH 1/3] Hoist loading default root certificates into ircConfig()

From: june
To: Klemens Nanni, list+catgirl
On Wed, Jun 9, 2021, at 21:32, Klemens Nanni wrote:
> diff --git a/irc.c b/irc.c
> index c98193a..92a70fe 100644
> --- a/irc.c
> +++ b/irc.c
> @@ -71,6 +71,15 @@ void ircConfig(
>  		if (error) errx(EX_NOINPUT, "%s: %s", trust, tls_config_error(config));
>  	}
>  
> +	if (!insecure && !trust) {
> +		size_t ca_sz;
> +		uint8_t *ca = tls_load_file(tls_default_ca_cert_file(), &ca_sz, NULL);
> +		if (!ca) errx(EX_SOFTWARE, "tls_load_file");
> +		error = tls_config_set_ca_mem(config, ca, ca_sz);
> +		if (error == -1) errx(EX_SOFTWARE, "tls_config_set_ca_mem");
> +		tls_unload_file(ca, ca_sz);
> +	}
> +

Can't this be tls_config_set_ca_file(config, tls_default_ca_cert_file())?

Re: [PATCH 1/3] Hoist loading default root certificates into ircConfig()

From: Klemens Nanni
To: june
Cc: list+catgirl
On Thu, Jun 10, 2021 at 03:43:36PM -0400, june wrote:
> On Thu, Jun 10, 2021, at 11:49, june wrote:
> > Can't this be tls_config_set_ca_file(config, tls_default_ca_cert_file())?
> 
> This seems to work, so I'll apply these and amend to use it.
Right, that does the trick;  the manual left me under the impression
that tls_config_set_ca_file(3) just sets the path but doesn't load it
immediately, i.e. I assumed it'd be loaded only during connect similar
to the default behaviour.

Thanks.