DKIM: fix signing bug induced by total size of parameter text

causing header-line fold between "b=" and terminating ";" of
pseudo-header.

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 18a43d29220bfd416b08985ebd00fe3d6c351069..afd41e40a7652dd30b5b0af6f8bee29715727ed2 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -38,6 +38,11 @@ JH/21 Bug 2151 (partial):
                 Avoid using SIZE on the MAIL for a callout verify, on any but
       the main verify for receipient in uncached-mode.
 
+JH/26 Fix DKIM bug: when the pseudoheader generated for signing was exactly
+      the right size to place the terminating semicolon on its own folded
+      line, the header hash was calculated to an incorrect value thanks to
+      the (relaxed) space the fold became.
+
 
 Exim version 4.89
 -----------------

diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index 4c93de70dd1e14124125bcb41626d82d940f121c..7dad1b896b292b6534b6554834da4cca07cf02d0 100644 (file)
--- a/src/src/pdkim/pdkim.c
+++ b/src/src/pdkim/pdkim.c
@@ -288,7 +288,7 @@ found:
 /* Performs "relaxed" canonicalization of a header. */
 
 static uschar *
-pdkim_relax_header(const uschar * header, int crlf)
+pdkim_relax_header(const uschar * header, BOOL append_crlf)
 {
 BOOL past_field_name = FALSE;
 BOOL seen_wsp = FALSE;
@@ -299,8 +299,8 @@ uschar * q = relaxed;
 for (p = header; *p; p++)
   {
   uschar c = *p;
-  /* Ignore CR & LF */
-  if (c == '\r' || c == '\n')
+
+  if (c == '\r' || c == '\n')  /* Ignore CR & LF */
     continue;
   if (c == '\t' || c == ' ')
     {
@@ -312,8 +312,8 @@ for (p = header; *p; p++)
   else
     if (!past_field_name && c == ':')
       {
-      if (seen_wsp) q--;       /* This removes WSP before the colon */
-      seen_wsp = TRUE;         /* This removes WSP after the colon */
+      if (seen_wsp) q--;       /* This removes WSP immediately before the colon */
+      seen_wsp = TRUE;         /* This removes WSP immediately after the colon */
       past_field_name = TRUE;
       }
     else
@@ -326,7 +326,7 @@ for (p = header; *p; p++)
 
 if (q > relaxed && q[-1] == ' ') q--; /* Squash eventual trailing SP */
 
-if (crlf) { *q++ = '\r'; *q++ = '\n'; }
+if (append_crlf) { *q++ = '\r'; *q++ = '\n'; }
 *q = '\0';
 return relaxed;
 }
@@ -1303,11 +1303,23 @@ if (sig->bodylength >= 0)
   }
 
 /* Preliminary or final version? */
-base64_b = final ? pdkim_encode_base64(&sig->sighash) : US"";
-hdr = pdkim_headcat(&col, hdr, &hdr_size, &hdr_len, US";", US"b=", base64_b);
+if (final)
+  {
+  base64_b = pdkim_encode_base64(&sig->sighash);
+  hdr = pdkim_headcat(&col, hdr, &hdr_size, &hdr_len, US";", US"b=", base64_b);
 
-/* add trailing semicolon: I'm not sure if this is actually needed */
-hdr = pdkim_headcat(&col, hdr, &hdr_size, &hdr_len, NULL, US";", US"");
+  /* add trailing semicolon: I'm not sure if this is actually needed */
+  hdr = pdkim_headcat(&col, hdr, &hdr_size, &hdr_len, NULL, US";", US"");
+  }
+else
+  {
+  /* To satisfy the rule "all surrounding whitespace [...] deleted"
+  ( RFC 6376 section 3.7 ) we ensure there is no whitespace here.  Otherwise
+  the headcat routine could insert a linebreak which the relaxer would reduce
+  to a single space preceding the terminating semicolon, resulting in an
+  incorrect header-hash. */
+  hdr = pdkim_headcat(&col, hdr, &hdr_size, &hdr_len, US";", US"b=;", US"");
+  }
 
 hdr[hdr_len] = '\0';
 return hdr;
@@ -1452,7 +1464,7 @@ while (sig)
                        p->value, (q - US p->value) + (p->next ? 1 : 0));
 
        rh = sig->canon_headers == PDKIM_CANON_RELAXED
-         ? pdkim_relax_header(p->value, 1) /* cook header for relaxed canon */
+         ? pdkim_relax_header(p->value, TRUE) /* cook header for relaxed canon */
          : string_copy(CUS p->value);      /* just copy it for simple canon */
 
        /* Feed header to the hash algorithm */
@@ -1513,7 +1525,7 @@ while (sig)
            /* cook header for relaxed canon, or just copy it for simple  */
 
            uschar * rh = sig->canon_headers == PDKIM_CANON_RELAXED
-             ? pdkim_relax_header(hdrs->value, 1)
+             ? pdkim_relax_header(hdrs->value, TRUE)
              : string_copy(CUS hdrs->value);
 
            /* Feed header to the hash algorithm */
@@ -1537,7 +1549,7 @@ while (sig)
 
   /* Relax header if necessary */
   if (sig->canon_headers == PDKIM_CANON_RELAXED)
-    sig_hdr = pdkim_relax_header(sig_hdr, 0);
+    sig_hdr = pdkim_relax_header(sig_hdr, FALSE);
 
   DEBUG(D_acl)
     {