DKIM: check pointer to calculated body hash before verify comparison. Bug 2029

We can have a missing body hash from a malformed DKIM-Signature: header

3 files changed, 31 insertions, 33 deletions

diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index ad5b1096d..5e8984c6f 100644
--- a/src/src/pdkim/pdkim.c
+++ b/src/src/pdkim/pdkim.c
@@ -192,7 +192,8 @@ static void
 pdkim_hexprint(const uschar *data, int len)
 {
 int i;
-for (i = 0 ; i < len; i++) debug_printf("%02x", data[i]);
+if (data) for (i = 0 ; i < len; i++) debug_printf("%02x", data[i]);
+else debug_printf("<NULL>");
 debug_printf("\n");
 }
 
@@ -803,11 +804,11 @@ for (sig = ctx->sig; sig; sig = sig->next)
       sig->bodylength = -1;
     }
 
-  /* VERIFICATION --------------------------------------------------------- */
   else
-    {
-    /* Compare bodyhash */
-    if (memcmp(bh.data, sig->bodyhash.data, bh.len) == 0)
+  /* VERIFICATION --------------------------------------------------------- */
+  /* Be careful that the header sig included a bodyash */
+
+    if (sig->bodyhash.data && memcmp(bh.data, sig->bodyhash.data, bh.len) == 0)
       {
       DEBUG(D_acl) debug_printf("PDKIM [%s] Body hash verified OK\n", sig->domain);
       }
@@ -962,27 +963,24 @@ else
 		  DKIM_SIGNATURE_HEADERNAME,
 		  Ustrlen(DKIM_SIGNATURE_HEADERNAME)) == 0)
     {
-    pdkim_signature *new_sig;
+    pdkim_signature * new_sig, * last_sig;
+
+    /* Create and chain new signature block.  We could error-check for all
+    required tags here, but prefer to create the internal sig and expicitly
+    fail verification of it later. */
 
-    /* Create and chain new signature block */
     DEBUG(D_acl) debug_printf(
 	"PDKIM >> Found sig, trying to parse >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>\n");
 
-    if ((new_sig = pdkim_parse_sig_header(ctx, ctx->cur_header)))
+    new_sig = pdkim_parse_sig_header(ctx, ctx->cur_header);
+
+    if (!(last_sig = ctx->sig))
+      ctx->sig = new_sig;
+    else
       {
-      pdkim_signature *last_sig = ctx->sig;
-      if (!last_sig)
-	ctx->sig = new_sig;
-      else
-        {
-	while (last_sig->next) last_sig = last_sig->next;
-	last_sig->next = new_sig;
-	}
+      while (last_sig->next) last_sig = last_sig->next;
+      last_sig->next = new_sig;
       }
-    else
-      DEBUG(D_acl) debug_printf(
-	  "Error while parsing signature header\n"
-	  "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n");
     }
 
   /* all headers are stored for signature verification */
@@ -1003,7 +1001,7 @@ return PDKIM_OK;
 DLLEXPORT int
 pdkim_feed(pdkim_ctx *ctx, char *data, int len)
 {
-int p;
+int p, rc;
 
 /* Alternate EOD signal, used in non-dotstuffing mode */
 if (!data)
@@ -1028,7 +1026,6 @@ else for (p = 0; p<len; p++)
       ctx->flags |= PDKIM_SEEN_CR;
     else if (c == '\n')
       {
-      int rc;
       ctx->flags &= ~PDKIM_SEEN_CR;
       if ((rc = pdkim_bodyline_complete(ctx)) != PDKIM_OK)
 	return rc;
@@ -1048,14 +1045,14 @@ else for (p = 0; p<len; p++)
 	ctx->cur_header = string_catn(ctx->cur_header, &ctx->cur_header_size,
 				&ctx->cur_header_len, CUS "\r", 1);
 
-      if (ctx->flags & PDKIM_SEEN_LF)
+      if (ctx->flags & PDKIM_SEEN_LF)		/* Seen last header line */
 	{
-	int rc = pdkim_header_complete(ctx); /* Seen last header line */
-	if (rc != PDKIM_OK) return rc;
+	if ((rc = pdkim_header_complete(ctx)) != PDKIM_OK)
+	  return rc;
 
 	ctx->flags = ctx->flags & ~(PDKIM_SEEN_LF|PDKIM_SEEN_CR) | PDKIM_PAST_HDRS;
 	DEBUG(D_acl) debug_printf(
-	    "PDKIM >> Body data for hash, canonicalized >>>>>>>>>>>>>>>>>>>>>>\n");
+	    "PDKIM >> Body data for hash, canonicalized >>>>>>>>>>>>>>>>>>>>>>>>>>>>\n");
 	continue;
 	}
       else
@@ -1063,11 +1060,9 @@ else for (p = 0; p<len; p++)
       }
     else if (ctx->flags & PDKIM_SEEN_LF)
       {
-      if (!(c == '\t' || c == ' '))
-	{
-	int rc = pdkim_header_complete(ctx); /* End of header */
-	if (rc != PDKIM_OK) return rc;
-	}
+      if (!(c == '\t' || c == ' '))			/* End of header */
+	if ((rc = pdkim_header_complete(ctx)) != PDKIM_OK)
+	  return rc;
       ctx->flags &= ~PDKIM_SEEN_LF;
       }
 
diff --git a/test/log/4506 b/test/log/4506
index d50bbe1f4..20e9c6b51 100644
--- a/test/log/4506
+++ b/test/log/4506
@@ -4,6 +4,9 @@
 1999-03-02 09:44:33 10HmaX-0005vi-00 DKIM: d=test.ex s=sel c=simple/simple a=rsa-sha1 b=0 [invalid - signature tag missing or invalid]
 1999-03-02 09:44:33 10HmaX-0005vi-00 signer: test.ex bits: 0
 1999-03-02 09:44:33 10HmaX-0005vi-00 <= CALLER@bloggs.com H=(xxx) [127.0.0.1] P=smtp S=sss id=qwerty1234@disco-zombie.net
-1999-03-02 09:44:33 10HmaY-0005vi-00 DKIM: d=test.ex s=sel c=simple/simple a=rsa-sha1 b=1024 [verification failed - body hash mismatch (body probably modified in transit)]
+1999-03-02 09:44:33 10HmaY-0005vi-00 DKIM: d=test.ex s=sel c=simple/simple a=rsa-sha1 b=1024 [invalid - signature tag missing or invalid]
 1999-03-02 09:44:33 10HmaY-0005vi-00 signer: test.ex bits: 1024
 1999-03-02 09:44:33 10HmaY-0005vi-00 <= CALLER@bloggs.com H=(xxx) [127.0.0.1] P=smtp S=sss id=qwerty1234@disco-zombie.net
+1999-03-02 09:44:33 10HmaZ-0005vi-00 DKIM: d=test.ex s=sel c=simple/simple a=rsa-sha1 b=1024 [verification failed - body hash mismatch (body probably modified in transit)]
+1999-03-02 09:44:33 10HmaZ-0005vi-00 signer: test.ex bits: 1024
+1999-03-02 09:44:33 10HmaZ-0005vi-00 <= CALLER@bloggs.com H=(xxx) [127.0.0.1] P=smtp S=sss id=qwerty1234@disco-zombie.net
diff --git a/test/scripts/4500-DKIM/4506 b/test/scripts/4500-DKIM/4506
index b2d53e8fc..14bba8346 100644
--- a/test/scripts/4500-DKIM/4506
+++ b/test/scripts/4500-DKIM/4506
@@ -1,6 +1,6 @@
 # DKIM verify, errors
 #
-exim -d-all+acl -DSERVER=server -bd -oX PORT_D
+exim -DSERVER=server -bd -oX PORT_D
 ****
 #
 # This should fail verify (missing header hash in sig header)