Fix signing with simple canonicalization

* The AMS passed to arc-canon has to be in its final form with all
  whitespace exactly as it will appear in the message.
* Generate ARC headers with a space after the colon since that's nicer
  looking (and matches what the milter was doing.)
* Lowercasing will be handled by the actual canonicalization if
  necessary, don't do it preemptively.

CHANGELOG.md

@@ -11,10 +11,12 @@ All notable changes to this project will be documented in this file.
 ### Changed
 - libopenarc - `ARC-Message-Signature` and `ARC-Authentication-Results` headers
   are excluded from the AMS, as required by RFC 8617.
+- libopenarc - ARC headers are returned with a space before the header value.
 
 ### Fixed
 - libopenarc - seals on failed chains only cover the latest ARC header set,
   as required by RFC 8617 section 5.1.2.
+- libopenarc - signing with simple header canonicalization works.
 
 ## 1.0.0 - 2024-10-18
 

libopenarc/arc-canon.c

@@ -1048,12 +1048,10 @@ arc_canon_runheaders_seal(ARC_MESSAGE *msg)
 
 				tmphdr.hdr_text = arc_dstring_get(msg->arc_hdrbuf);
 				tmphdr.hdr_namelen = cur->canon_sigheader->hdr_namelen;
-				tmphdr.hdr_colon = tmphdr.hdr_text + (cur->canon_sigheader->hdr_colon - cur->canon_sigheader->hdr_text);
 				tmphdr.hdr_textlen = arc_dstring_len(msg->arc_hdrbuf);
 				tmphdr.hdr_flags = 0;
 				tmphdr.hdr_next = NULL;
 
-				arc_lowerhdr(tmphdr.hdr_text);
 				/* XXX -- void? */
 				(void) arc_canon_header(msg, cur, &tmphdr,
 				                        FALSE);
@@ -1357,7 +1355,6 @@ arc_canon_runheaders(ARC_MESSAGE *msg)
 		/* canonicalize */
 		tmphdr.hdr_text = arc_dstring_get(msg->arc_hdrbuf);
 		tmphdr.hdr_namelen = cur->canon_sigheader->hdr_namelen;
-		tmphdr.hdr_colon = tmphdr.hdr_text + (cur->canon_sigheader->hdr_colon - cur->canon_sigheader->hdr_text);
 		tmphdr.hdr_textlen = arc_dstring_len(msg->arc_hdrbuf);
 		tmphdr.hdr_flags = 0;
 		tmphdr.hdr_next = NULL;
@@ -1427,11 +1424,9 @@ arc_canon_signature(ARC_MESSAGE *msg, struct arc_hdrfield *hdr, int type)
 		}
 		tmphdr.hdr_text = arc_dstring_get(msg->arc_hdrbuf);
 		tmphdr.hdr_namelen = hdr->hdr_namelen;
-		tmphdr.hdr_colon = tmphdr.hdr_text + (hdr->hdr_colon - hdr->hdr_text);
 		tmphdr.hdr_textlen = arc_dstring_len(msg->arc_hdrbuf);
 		tmphdr.hdr_flags = 0;
 		tmphdr.hdr_next = NULL;
-		arc_lowerhdr(tmphdr.hdr_text);
 
 		/* canonicalize the signature */
 		status = arc_canon_header(msg, cur, &tmphdr, FALSE);

libopenarc/arc-types.h

@@ -57,7 +57,6 @@ struct arc_hdrfield
 	uint32_t		hdr_flags;
 	size_t			hdr_namelen;
 	size_t			hdr_textlen;
-	char *			hdr_colon;
 	char *			hdr_text;
 	void *			hdr_data;
 	struct arc_hdrfield *	hdr_next;

libopenarc/arc.c

@@ -705,7 +705,7 @@ arc_getamshdr_d(ARC_MESSAGE *msg, size_t initial, char **buf, size_t *buflen,
 			else if (forcewrap || len + pvlen > msg->arc_margin)
 			{
 				forcewrap = FALSE;
-				arc_dstring_catn(msg->arc_hdrbuf, "\n\t", 2);
+				arc_dstring_cat(msg->arc_hdrbuf, "\r\n\t");
 				len = 8;
 
 				if (strcmp(which, "h") == 0)
@@ -734,9 +734,8 @@ arc_getamshdr_d(ARC_MESSAGE *msg, size_t initial, char **buf, size_t *buflen,
 							arc_dstring_cat1(msg->arc_hdrbuf,
 							                 ':');
 							len += 1;
-							arc_dstring_catn(msg->arc_hdrbuf,
-							                 "\n\t ",
-							                 3);
+							arc_dstring_cat(msg->arc_hdrbuf,
+							                "\r\n\t ");
 							len = 9;
 							arc_dstring_catn(msg->arc_hdrbuf,
 							                 tmp,
@@ -781,9 +780,8 @@ arc_getamshdr_d(ARC_MESSAGE *msg, size_t initial, char **buf, size_t *buflen,
 					{
 						if (msg->arc_margin - len == 0)
 						{
-							arc_dstring_catn(msg->arc_hdrbuf,
-							                  "\n\t ",
-							                  3);
+							arc_dstring_cat(msg->arc_hdrbuf,
+							                "\r\n\t ");
 							len = 9;
 						}
 
@@ -2583,10 +2581,6 @@ arc_parse_header_field(ARC_MESSAGE *msg, const unsigned char *hdr, size_t hlen,
 
 	h->hdr_namelen = end != NULL ? end - hdr : hlen;
 	h->hdr_textlen = hlen;
-	if (colon == NULL)
-		h->hdr_colon = NULL;
-	else
-		h->hdr_colon = h->hdr_text + (colon - hdr);
 	h->hdr_flags = 0;
 	h->hdr_next = NULL;
 
@@ -2874,7 +2868,7 @@ arc_eoh(ARC_MESSAGE *msg)
 
 			kvtype = arc_name_to_code(archdrnames, hnbuf);
 			status = arc_process_set(msg, kvtype,
-			                         h->hdr_colon + 1,
+			                         h->hdr_text + h->hdr_namelen + 1,
 			                         h->hdr_textlen - h->hdr_namelen - 1,
 			                         h,
 			                         &set);
@@ -3375,7 +3369,7 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 	**  Part 1: Construct a new AAR
 	*/
 
-	arc_dstring_printf(dstr, "ARC-Authentication-Results:i=%u; %s",
+	arc_dstring_printf(dstr, "ARC-Authentication-Results: i=%u; %s",
 	                   msg->arc_nsets + 1,
 	                   msg->arc_authservid);
 	if (ar == NULL) {
@@ -3411,8 +3405,7 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 
 	/* construct the AMS */
 	arc_dstring_blank(dstr);
-	arc_dstring_catn(dstr, ARC_MSGSIG_HDRNAME ":",
-	                 sizeof ARC_MSGSIG_HDRNAME);
+	arc_dstring_cat(dstr, ARC_MSGSIG_HDRNAME ": ");
 
 	status = arc_getamshdr_d(msg, arc_dstring_len(dstr), &sighdr, &len,
 	                         FALSE);
@@ -3426,7 +3419,6 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 	len = arc_dstring_len(dstr);
 
 	hdr.hdr_text = arc_dstring_get(dstr);
-	hdr.hdr_colon = hdr.hdr_text + ARC_MSGSIG_HDRNAMELEN;
 	hdr.hdr_namelen = ARC_MSGSIG_HDRNAMELEN;
 	hdr.hdr_textlen = len;
 	hdr.hdr_flags = 0;
@@ -3506,6 +3498,11 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 		goto error;
 	}
 
+	/* This header is generated with \r\n so that simple canonicalization
+	 * will work, but the established interface is that the library returns
+	 * just \n.
+	 */
+	arc_dstring_strip(dstr, "\r");
 	h->hdr_text = ARC_STRDUP(arc_dstring_get(dstr));
 	if (h->hdr_text == NULL)
 	{
@@ -3515,7 +3512,6 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 		ARC_FREE(h);
 		goto error;
 	}
-	h->hdr_colon = h->hdr_text + ARC_MSGSIG_HDRNAMELEN;
 	h->hdr_namelen = ARC_MSGSIG_HDRNAMELEN;
 	h->hdr_textlen = arc_dstring_len(dstr);
 	h->hdr_flags = 0;
@@ -3529,8 +3525,7 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 	*/
 
 	arc_dstring_blank(dstr);
-	arc_dstring_catn(dstr, ARC_SEAL_HDRNAME ":",
-	                 sizeof ARC_SEAL_HDRNAME);
+	arc_dstring_cat(dstr, ARC_SEAL_HDRNAME ": ");
 
 	/* feed the seal we have so far */
 	status = arc_canon_add_to_seal(msg);
@@ -3551,7 +3546,6 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 	arc_dstring_catn(dstr, sighdr, len);
 
 	hdr.hdr_text = arc_dstring_get(dstr);
-	hdr.hdr_colon = hdr.hdr_text + ARC_SEAL_HDRNAMELEN;
 	hdr.hdr_namelen = ARC_SEAL_HDRNAMELEN;
 	hdr.hdr_textlen = len;
 	hdr.hdr_flags = 0;
@@ -3606,14 +3600,14 @@ arc_getseal(ARC_MESSAGE *msg, ARC_HDRFIELD **seal, char *authservid,
 		status = ARC_STAT_INTERNAL;
 		goto error;
 	}
+	arc_dstring_strip(dstr, "\r");
 	h->hdr_text = ARC_STRDUP(arc_dstring_get(dstr));
 	if (h->hdr_text == NULL)
 	{
 		arc_error(msg, "can't allocate %d bytes", sizeof hdr);
 		status = ARC_STAT_INTERNAL;
 		goto error;
 	}
-	h->hdr_colon = h->hdr_text + ARC_SEAL_HDRNAMELEN;
 	h->hdr_namelen = ARC_SEAL_HDRNAMELEN;
 	h->hdr_textlen = len;
 	h->hdr_flags = 0;
@@ -3669,7 +3663,7 @@ arc_hdr_name(ARC_HDRFIELD *hdr, size_t *len)
 unsigned char *
 arc_hdr_value(ARC_HDRFIELD *hdr)
 {
-	return (unsigned char *) hdr->hdr_colon + 1;
+	return (unsigned char *) hdr->hdr_text + hdr->hdr_namelen + 1;
 }
 
 /*

openarc/openarc.c

@@ -3716,24 +3716,20 @@ mlfi_eom(SMFICTX *ctx)
 		     sealhdr = arc_hdr_next(sealhdr))
 		{
 			size_t len;
-			char *hfvdest;
+			char *hfvalue;
 			char hfname[BUFRSZ + 1];
-			char hfvalue[ARC_MAXHEADER + 1];
 
 			memset(hfname, '\0', sizeof hfname);
 			strlcpy(hfname, (char *) arc_hdr_name(sealhdr, &len),
 			        sizeof hfname);
 			hfname[len] = '\0';
 
-			hfvdest = hfvalue;
-			memset(hfvalue, '\0', sizeof hfvalue);
-			if (cc->cctx_noleadspc)
+			hfvalue = (char *) arc_hdr_value(sealhdr);
+			if (!cc->cctx_noleadspc)
 			{
-				hfvalue[0] = ' ';
-				hfvdest++;
+				/* strip off the leading space */
+				hfvalue++;
 			}
-			strlcat(hfvalue, (char *) arc_hdr_value(sealhdr),
-			        sizeof hfvalue);
 
 			status = arcf_insheader(ctx, 0, hfname, hfvalue);
 			if (status == MI_FAILURE)

test/files/test_milter_canon_simple.conf

@@ -0,0 +1,7 @@
+Domain          example.com
+AuthservID      example.com
+KeyFile         private.key
+TestKeys        public.key
+Selector        elpmaxe
+Mode            s
+Canonicalization simple/simple

test/test_milter.py

@@ -48,6 +48,8 @@ def _run_miltertest(
         ins_headers = []
         for msg in resp:
             if msg[0] == miltertest.SMFIR_INSHEADER:
+                # Check for invalid characters
+                assert '\r' not in msg[1]['value']
                 ins_headers.insert(msg[1]['index'], [msg[1]['name'], msg[1]['value']])
             elif msg[0] in miltertest.DISPOSITION_REPLIES:
                 assert msg[0] == miltertest.SMFIR_ACCEPT
@@ -121,6 +123,16 @@ def test_milter_staticmsg(run_miltertest):
     assert res['headers'][3] == ['ARC-Authentication-Results', ' i=2; example.com; arc=pass smtp.remote-ip=127.0.0.1']
 
 
+def test_milter_canon_simple(run_miltertest):
+    """Sign a message with simple canonicalization and then verify it"""
+    res = run_miltertest()
+    assert 'c=simple/simple' in res['headers'][1][1]
+
+    res = run_miltertest(res['headers'])
+    assert 'cv=pass' in res['headers'][0][1]
+    assert res['headers'][2] == ['ARC-Authentication-Results', ' i=2; example.com; arc=pass smtp.remote-ip=127.0.0.1']
+
+
 def test_milter_resign(run_miltertest):
     """Extend the chain as much as possible"""
     res = run_miltertest()

util/arc-dstring.c

@@ -479,68 +479,66 @@ arc_dstring_printf(struct arc_dstring *dstr, char *fmt, ...)
 }
 
 /*
-**  ARC_COLLAPSE -- remove spaces from a string
+**  ARC_DSTRING_STRIP -- remove matching characters from a string
 **
 **  Parameters:
-**  	str -- string to process
+**      dstr -- string to process
+**      cset -- characters to remove
 **
 **  Return value:
-**  	None.
+**      None.
 */
-
 void
-arc_collapse(char *str)
+arc_dstring_strip(struct arc_dstring *dstr, const char *cset)
 {
-    char *q;
-    char *r;
-
-    assert(str != NULL);
-
-    for (q = str, r = str; *q != '\0'; q++)
+    size_t newlen = 0;
+    for (size_t i = 0; i <= dstr->ds_len; i++)
     {
-        if (!isspace(*q))
+        while (strchr(cset, dstr->ds_buf[i]) && i <= dstr->ds_len)
         {
-            if (q != r)
-            {
-                *r = *q;
-            }
-            r++;
+            i++;
+        }
+        if (i <= dstr->ds_len)
+        {
+            dstr->ds_buf[newlen] = dstr->ds_buf[i];
+            newlen++;
         }
     }
-
-    *r = '\0';
+    dstr->ds_buf[newlen] = '\0';
+    dstr->ds_len = newlen;
 }
 
 /*
-**  ARC_LOWERHDR -- convert a string (presumably a header) to all lowercase,
-**                  but only up to a colon
+**  ARC_COLLAPSE -- remove spaces from a string
 **
 **  Parameters:
-**  	str -- string to modify
+**  	str -- string to process
 **
 **  Return value:
 **  	None.
 */
 
 void
-arc_lowerhdr(char *str)
+arc_collapse(char *str)
 {
-    char *p;
+    char *q;
+    char *r;
 
     assert(str != NULL);
 
-    for (p = str; *p != '\0'; p++)
+    for (q = str, r = str; *q != '\0'; q++)
     {
-        if (*p == ':')
-        {
-            return;
-        }
-
-        if (isascii(*p) && isupper(*p))
+        if (!isspace(*q))
         {
-            *p = tolower(*p);
+            if (q != r)
+            {
+                *r = *q;
+            }
+            r++;
         }
     }
+
+    *r = '\0';
 }
 
 /*

util/arc-dstring.h

@@ -28,6 +28,7 @@ extern bool arc_dstring_cat(struct arc_dstring *, const char *);
 extern bool arc_dstring_cat1(struct arc_dstring *, int);
 extern bool arc_dstring_catn(struct arc_dstring *, const char *, size_t);
 extern bool arc_dstring_copy(struct arc_dstring *, const char *);
+extern void arc_dstring_strip(struct arc_dstring *, const char *);
 extern void arc_dstring_free(struct arc_dstring *);
 extern char *arc_dstring_get(struct arc_dstring *);
 extern int arc_dstring_len(struct arc_dstring *);
@@ -37,6 +38,5 @@ extern size_t arc_dstring_printf(struct arc_dstring *dstr, char *fmt, ...);
 extern void arc_clobber_array(char **);
 extern void arc_collapse(char *);
 extern char **arc_copy_array(char **);
-extern void arc_lowerhdr(char *);
 
 #endif /* ARC_DSTRING_H_ */