[9front] smtpd: add certificate chain to tls connection

[9front] smtpd: add certificate chain to tls connection

[sirjofri]

Smtpd currently only sends the certificate itself, not the intermediate certificates. Some servers like deltachat are more strict and want the intermediate certificates. 
 
This patch makes smtpd send the full chain that we got from acmed, if possible. The first cert in that chain is our own cert, everything else is the intermediate chain. 
 
As you can see in the patch, I tried to free the cert chain after use. However, it didn't work and I wasn't able to investigate why. Any hints would be great so I can fix the patch. 
 
In case my mail client garbles the paste, here's the patch for download: https://sirjofri.de/oat/patches/smtpdchain.patch 
 
sirjofri 
 
diff bd1305a0b9841d65f95b4d81fa0d29067425833a uncommitted 
--- a/sys/src/cmd/upas/smtp/smtpd.c 
+++ b/sys/src/cmd/upas/smtp/smtpd.c 
@@ -1582,25 +1582,41 @@ 
} 
 
void 
+freecertchain(PEMChain *chain) 
+{ 
+ PEMChain *p; 
+ 
+ while (chain) { 
+ p = chain->next; 
+ if (chain->pem) 
+ free(chain->pem); 
+ free(chain); 
+ chain = p; 
+ } 
+} 
+ 
+void 
starttls(void) 
{ 
- int certlen, fd; 
- uchar *cert; 
+ int fd; 
TLSconn conn; 
+ PEMChain *chain; 
 
if (tlscert == nil) { 
reply("500 5.5.1 illegal command or bad syntax\r\n"); 
return; 
} 
- cert = readcert(tlscert, &certlen); 
- if (cert == nil) { 
+ chain = readcertchain(tlscert); 
+ if (chain == nil) { 
reply("454 4.7.5 TLS not available\r\n"); 
return; 
} 
reply("220 2.0.0 Go ahead make my day\r\n"); 
memset(&conn, 0, sizeof(conn)); 
- conn.cert = cert; 
- conn.certlen = certlen; 
+ conn.cert = chain->pem; 
+ conn.certlen = chain->pemlen; 
+ if (chain->next) 
+ conn.chain = chain->next; 
fd = tlsServer(Bfildes(&bin), &conn); 
if (fd < 0) { 
syslog(0, "smtpd", "TLS start-up failed with %s", him); 
@@ -1611,7 +1627,7 @@ 
fprint(2, "dup of %d failed: %r\n", fd); 
close(fd); 
Binit(&bin, 0, OREAD); 
- free(conn.cert); 
+ //freecertchain(chain); 
free(conn.sessionID); 
passwordinclear = 1; 
syslog(0, "smtpd", "started TLS with %s", him); 

[9front] Re: smtpd: add certificate chain to tls connection

[Anthony Martin]

sirjofri <sirjofri+ml-9front@sirjofri.de> once said:
> As you can see in the patch, I tried to free the cert chain after use.
> However, it didn't work and I wasn't able to investigate why. Any
> hints would be great so I can fix the patch.

TlsServer(2) frees conn->cert after the handshake as a signal that
client certificates aren't supported. Read the manual page carefully.
Make a copy of the first certificate in the chain and your free
function should work fine.

Check how it's done in /sys/src/cmd/ndb/dntcpserver.c for example.

Cheers,
  Anthony

Re: [9front] Re: smtpd: add certificate chain to tls connection

[sirjofri]

03.03.2025 22:11:01 Anthony Martin <ality@pbrane.org>: 
> sirjofri <sirjofri+ml-9front@sirjofri.de> once said: 
>> As you can see in the patch, I tried to free the cert chain after use. 
>> However, it didn't work and I wasn't able to investigate why. Any 
>> hints would be great so I can fix the patch. 
> 
> TlsServer(2) frees conn->cert after the handshake as a signal that 
> client certificates aren't supported. Read the manual page carefully. 
> Make a copy of the first certificate in the chain and your free 
> function should work fine. 
 
That's funny, because the current smtpd is freeing conn.cert manually, so I assumed that it would work. 
 
I'll try to understand the situation better and fix the patch. Thanks for the response. 
 
sirjofri 

Re: [9front] Re: smtpd: add certificate chain to tls connection

[Anthony Martin]

sirjofri <sirjofri+ml-9front@sirjofri.de> once said:
> That's funny, because the current smtpd is freeing conn.cert manually,
> so I assumed that it would work.

That's because tlsServer(2) also sets conn->cert to nil after freeing it.
Calling free(2) on a nil pointer is a no-op.

Cheers,
  Anthony

Re: [9front] Re: smtpd: add certificate chain to tls connection

[sirjofri]

updated patch, with proper certificate freeing. I tested this with https://www.checktls.com/TestReceiver in case you want to do some tests yourself.

sirjofri

diff bd1305a0b9841d65f95b4d81fa0d29067425833a uncommitted
--- a/sys/src/cmd/upas/smtp/smtpd.c
+++ b/sys/src/cmd/upas/smtp/smtpd.c
@@ -1582,25 +1582,44 @@
}

void
+freecertchain(PEMChain *chain)
+{
+   PEMChain *p;
+
+   while (chain) {
+       p = chain->next;
+       if (chain->pem)
+           free(chain->pem);
+       free(chain);
+       chain = p;
+   }
+}
+
+void
starttls(void)
{
-   int certlen, fd;
-   uchar *cert;
+   int fd;
    TLSconn conn;
+   PEMChain *chain;

    if (tlscert == nil) {
        reply("500 5.5.1 illegal command or bad syntax\r\n");
        return;
    }
-   cert = readcert(tlscert, &certlen);
-   if (cert == nil) {
+   chain = readcertchain(tlscert);
+   if (chain == nil) {
        reply("454 4.7.5 TLS not available\r\n");
        return;
    }
    reply("220 2.0.0 Go ahead make my day\r\n");
    memset(&conn, 0, sizeof(conn));
-   conn.cert = cert;
-   conn.certlen = certlen;
+   conn.cert = malloc(chain->pemlen);
+   if (conn.cert == nil)
+       sysfatal("out of memory in starttls: %r");
+   memmove(conn.cert, chain->pem, chain->pemlen);
+   conn.certlen = chain->pemlen;
+   if (chain->next)
+       conn.chain = chain->next;
    fd = tlsServer(Bfildes(&bin), &conn);
    if (fd < 0) {
        syslog(0, "smtpd", "TLS start-up failed with %s", him);
@@ -1611,6 +1630,7 @@
        fprint(2, "dup of %d failed: %r\n", fd);
    close(fd);
    Binit(&bin, 0, OREAD);
+   freecertchain(chain);
    free(conn.cert);
    free(conn.sessionID);
    passwordinclear = 1;

Re: [9front] Re: smtpd: add certificate chain to tls connection

[cinap_lenrek]

why would we need to copy the pem? we can just pass
the pointer from chain->pem to conn.cert and free
the chain head.

how about this:

diff acd2079ebf458c20ad5b3e5504c00a92d5fe46f1 uncommitted
--- a/sys/src/cmd/upas/smtp/smtpd.c
+++ b/sys/src/cmd/upas/smtp/smtpd.c
@@ -1584,23 +1584,25 @@
 void
 starttls(void)
 {
-	int certlen, fd;
-	uchar *cert;
+	int fd;
 	TLSconn conn;
+	PEMChain *chain;
 
 	if (tlscert == nil) {
 		reply("500 5.5.1 illegal command or bad syntax\r\n");
 		return;
 	}
-	cert = readcert(tlscert, &certlen);
-	if (cert == nil) {
+	chain = readcertchain(tlscert);
+	if (chain == nil) {
 		reply("454 4.7.5 TLS not available\r\n");
 		return;
 	}
 	reply("220 2.0.0 Go ahead make my day\r\n");
 	memset(&conn, 0, sizeof(conn));
-	conn.cert = cert;
-	conn.certlen = certlen;
+	conn.cert = chain->pem;
+	conn.certlen = chain->pemlen;
+	conn.chain = chain->next;
+	free(chain);
 	fd = tlsServer(Bfildes(&bin), &conn);
 	if (fd < 0) {
 		syslog(0, "smtpd", "TLS start-up failed with %s", him);
@@ -1611,7 +1613,11 @@
 		fprint(2, "dup of %d failed: %r\n", fd);
 	close(fd);
 	Binit(&bin, 0, OREAD);
-	free(conn.cert);
+	while((chain = conn.chain) != nil){
+		conn.chain = chain->next;
+		free(chain->pem);
+		free(chain);
+	}
 	free(conn.sessionID);
 	passwordinclear = 1;
 	syslog(0, "smtpd", "started TLS with %s", him);


--
cinap

Re: [9front] Re: smtpd: add certificate chain to tls connection

[cinap_lenrek]

final version with added comments, and also
unconditionally free conn.cert (in case tlsServer()
implements client certificates at some point).

does it make sense?

diff 7dd51004c150c35898b0931722ee9173c687f325 uncommitted
--- a/sys/src/cmd/upas/smtp/smtpd.c
+++ b/sys/src/cmd/upas/smtp/smtpd.c
@@ -1584,23 +1584,25 @@
 void
 starttls(void)
 {
-	int certlen, fd;
-	uchar *cert;
+	int fd;
 	TLSconn conn;
+	PEMChain *chain;
 
 	if (tlscert == nil) {
 		reply("500 5.5.1 illegal command or bad syntax\r\n");
 		return;
 	}
-	cert = readcert(tlscert, &certlen);
-	if (cert == nil) {
+	chain = readcertchain(tlscert);
+	if (chain == nil) {
 		reply("454 4.7.5 TLS not available\r\n");
 		return;
 	}
 	reply("220 2.0.0 Go ahead make my day\r\n");
 	memset(&conn, 0, sizeof(conn));
-	conn.cert = cert;
-	conn.certlen = certlen;
+	conn.cert = chain->pem;
+	conn.certlen = chain->pemlen;
+	conn.chain = chain->next;
+	free(chain);	/* chain->pem freed by tlsSevrer() */
 	fd = tlsServer(Bfildes(&bin), &conn);
 	if (fd < 0) {
 		syslog(0, "smtpd", "TLS start-up failed with %s", him);
@@ -1611,7 +1613,12 @@
 		fprint(2, "dup of %d failed: %r\n", fd);
 	close(fd);
 	Binit(&bin, 0, OREAD);
-	free(conn.cert);
+	while((chain = conn.chain) != nil){
+		conn.chain = chain->next;
+		free(chain->pem);
+		free(chain);
+	}
+	free(conn.cert);	/* client cert */
 	free(conn.sessionID);
 	passwordinclear = 1;
 	syslog(0, "smtpd", "started TLS with %s", him);

--
cinap

Re: [9front] Re: smtpd: add certificate chain to tls connection

[sirjofri]

04.03.2025 16:26:22 cinap_lenrek@felloff.net:
> diff 7dd51004c150c35898b0931722ee9173c687f325 uncommitted
> ---
>     Binit(&bin, 0, OREAD);
> -   free(conn.cert);
> +   while((chain = conn.chain) != nil){
> +       conn.chain = chain->next;
> +       free(chain->pem);
> +       free(chain);
> +   }
> +   free(conn.cert);    /* client cert */

Ah, so you basically skip the head of the chain and free the container early? Intelligent, and so clean!

Thank you for looking into this. Should I test it and report back or were you able to test it?

sirjofri

Re: [9front] Re: smtpd: add certificate chain to tls connection

[sirjofri]

Hi cinap, 
 
Just in case, I tested your patch (applied manually) and it didn't work. I didn't look closer at the reason as I don't have time yet, but the online tls check fails like with my previous double free. 
 
Just to prevent an early commit. 
 
sirjofri