Fix DKIM signing to always ;-terminate. Bug 2295

4 files changed, 55 insertions, 78 deletions

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 7372ac244..8d368c8f3 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -81,6 +81,11 @@ JH/16 Bug 2615: Fix pause during message reception, on systems that have been
       post-message next-tick-wait.  Also change to using CLOCK_BOOTTIME if it
       exists, just to get a clock slightly more aligned to reality.
 
+JH/17 Bug 2295: Fix DKIM signing to always semicolon-terminate.  Although the
+      RFC says it is optional some validators care.  The missing char was not
+      intended but triggered by a line-wrap alignement.  Discovery and fix by
+      Guillaume Outters, hacked on by JH.
+
 
 Exim version 4.94
 -----------------
diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index 7712409b5..3a6ca4e91 100644
--- a/src/src/pdkim/pdkim.c
+++ b/src/src/pdkim/pdkim.c
@@ -1111,14 +1111,14 @@ return string_catn(str, US"\r\n\t", 3);
 
 /*
  * RFC 5322 specifies that header line length SHOULD be no more than 78
- * lets make it so!
  *  pdkim_headcat
  *
- * returns uschar * (not nul-terminated)
+ * Returns gstring (not nul-terminated) appending to one supplied
  *
  * col: this int holds and receives column number (octets since last '\n')
  * str: partial string to append to
- * pad: padding, split line or space after before or after eg: ";"
+ * pad: padding, split line or space after before or after eg: ";".
+ *               Only the initial charater is used.
  * intro: - must join to payload eg "h=", usually the tag name
  * payload: eg base64 data - long data can be split arbitrarily.
  *
@@ -1127,7 +1127,7 @@ return string_catn(str, US"\r\n\t", 3);
  * pairs and inside long values. it also always spaces or breaks after the
  * "pad"
  *
- * no guarantees are made for output given out-of range input. like tag
+ * No guarantees are made for output given out-of range input. like tag
  * names longer than 78, or bogus col. Input is assumed to be free of line breaks.
  */
 
@@ -1135,92 +1135,64 @@ static gstring *
 pdkim_headcat(int * col, gstring * str,
   const uschar * pad, const uschar * intro, const uschar * payload)
 {
-size_t l;
+int len, chomp, padded = 0;
 
-if (pad)
-  {
-  l = Ustrlen(pad);
-  if (*col + l > 78)
-    str = pdkim_hdr_cont(str, col);
-  str = string_catn(str, pad, l);
-  *col += l;
-  }
-
-l = (pad?1:0) + (intro?Ustrlen(intro):0);
+/* If we can fit at least the pad at the end of current line, do it now.
+Otherwise, wrap if there is a pad. */
 
-if (*col + l > 78)
-  { /*can't fit intro - start a new line to make room.*/
-  str = pdkim_hdr_cont(str, col);
-  l = intro?Ustrlen(intro):0;
-  }
-
-l += payload ? Ustrlen(payload):0 ;
-
-while (l>77)
-  { /* this fragment will not fit on a single line */
-  if (pad)
-    {
-    str = string_catn(str, US" ", 1);
-    *col += 1;
-    pad = NULL; /* only want this once */
-    l--;
-    }
-
-  if (intro)
-    {
-    size_t sl = Ustrlen(intro);
-
-    str = string_catn(str, intro, sl);
-    *col += sl;
-    l -= sl;
-    intro = NULL; /* only want this once */
-    }
-
-  if (payload)
+if (pad)
+  if (*col + 1 <= 78)
     {
-    size_t sl = Ustrlen(payload);
-    size_t chomp = *col+sl < 77 ? sl : 78-*col;
-
-    str = string_catn(str, payload, chomp);
-    *col += chomp;
-    payload += chomp;
-    l -= chomp-1;
+    str = string_catn(str, pad, 1);
+    (*col)++;
+    pad = NULL;
+    padded = 1;
     }
+  else
+    str = pdkim_hdr_cont(str, col);
 
-  /* the while precondition tells us it didn't fit. */
-  str = pdkim_hdr_cont(str, col);
-  }
+/* Special case: if the whole addition does not fit at the end of the current
+line, but could fit on a new line, wrap to give it its full, dedicated line.  */
 
-if (*col + l > 78)
+len = (pad ? 2 : padded)
+    + (intro ? Ustrlen(intro) : 0)
+    + (payload ? Ustrlen(payload) : 0);
+if (len <= 77 && *col+len > 78)
   {
   str = pdkim_hdr_cont(str, col);
-  pad = NULL;
+  padded = 0;
   }
 
+/* Either we already dealt with the pad or we know there is room */
+
 if (pad)
   {
+  str = string_catn(str, pad, 1);
   str = string_catn(str, US" ", 1);
-  *col += 1;
-  pad = NULL;
+  *col += 2;
   }
-
-if (intro)
+else if (padded && *col < 78)
   {
-  size_t sl = Ustrlen(intro);
-
-  str = string_catn(str, intro, sl);
-  *col += sl;
-  l -= sl;
-  intro = NULL;
+  str = string_catn(str, US" ", 1);
+  (*col)++;
   }
 
-if (payload)
-  {
-  size_t sl = Ustrlen(payload);
+/* Call recursively with intro as payload: it gets the same, special treatment
+(that is, not split if < 78).  */
 
-  str = string_catn(str, payload, sl);
-  *col += sl;
-  }
+if (intro)
+  str = pdkim_headcat(col, str, NULL, NULL, intro);
+
+if (payload)
+  for (len = Ustrlen(payload); len; len -= chomp)
+    {
+    if (*col >= 78)
+      str = pdkim_hdr_cont(str, col);
+    chomp = *col+len > 78 ? 78 - *col : len;
+    str = string_catn(str, payload, chomp);
+    *col += chomp;
+    payload += chomp;
+    }
 
 return str;
 }
diff --git a/test/mail/4520.b12 b/test/mail/4520.b12
index e327a0deb..392cee332 100644
--- a/test/mail/4520.b12
+++ b/test/mail/4520.b12
@@ -6,9 +6,9 @@ Received: from the.local.host.name ([ip4.ip4.ip4.ip4] helo=myhost.test.ex)
 	for b12@test.ex; Tue, 2 Mar 1999 09:44:33 +0000
 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=test.ex;
 	s=sel; h=X-mine:X-mine:From; bh=/Ab0giHZitYQbDhFszoqQRUkgqueaX9zatJttIU/plc=;
-	 b=VSF3Fmf7fHLpqKWgaG0t/RJdd+7JJNRN/+q7Gcz0xtIOjt5tL4Pfc4VDSeQYPq6e1pCcjvj79k
-	57of1wr0KIfk5FAvUZXPu3OtRm71y1X2rUaQB8c2Y2SROSMyaT3tsPzaEigiptIeeOkULYkFl2Hln
-	15ssCcfufIlOx4EQ9fQA=;
+	b=VSF3Fmf7fHLpqKWgaG0t/RJdd+7JJNRN/+q7Gcz0xtIOjt5tL4Pfc4VDSeQYPq6e1pCcjvj79k5
+	7of1wr0KIfk5FAvUZXPu3OtRm71y1X2rUaQB8c2Y2SROSMyaT3tsPzaEigiptIeeOkULYkFl2Hln1
+	5ssCcfufIlOx4EQ9fQA=;
 Received: from CALLER by myhost.test.ex with local (Exim x.yz)
 	(envelope-from <CALLER@myhost.test.ex>)
 	id 10HmbE-0005vi-00
diff --git a/test/mail/4545.a b/test/mail/4545.a
index b4a931fa1..18752a778 100644
--- a/test/mail/4545.a
+++ b/test/mail/4545.a
@@ -5,9 +5,9 @@ Received: from the.local.host.name ([ip4.ip4.ip4.ip4] helo=myhost.test.ex)
 	id 10HmaY-0005vi-00
 	for a@test.ex; Tue, 2 Mar 1999 09:44:33 +0000
 DKIM-Signature: v=1; a=ed25519-sha256; q=dns/txt; c=relaxed/relaxed; d=test.ex
-	; s=sed; h=From:To:Subject; bh=/Ab0giHZitYQbDhFszoqQRUkgqueaX9zatJttIU/plc=;
-	 b=5fhyD3EILDrnL4DnkD4hDaeis7+GSzL9GMHrhIDZJjuJ00WD5iI8SQ1q9rDfzFL/Kdw0VIyB4R
-	Dq0a4H6HI+Bw==;
+	; s=sed; h=From:To:Subject; bh=/Ab0giHZitYQbDhFszoqQRUkgqueaX9zatJttIU/plc=; 
+	b=5fhyD3EILDrnL4DnkD4hDaeis7+GSzL9GMHrhIDZJjuJ00WD5iI8SQ1q9rDfzFL/Kdw0VIyB4RD
+	q0a4H6HI+Bw==;
 Received: from CALLER by myhost.test.ex with local (Exim x.yz)
 	(envelope-from <CALLER@myhost.test.ex>)
 	id 10HmaX-0005vi-00