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

diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index ad5b1096de002521b17d8f1331639987b9c34b66..5e8984c6feed05fe6b49444d0b43e7cd9251b2d5 100644 (file)
--- 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;
 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");
 }
 
 debug_printf("\n");
 }
 


@@ -803,11 +804,11 @@ for (sig = ctx->sig; sig; sig = sig->next)
       sig->bodylength = -1;
     }
 
       sig->bodylength = -1;
     }
 

-  /* VERIFICATION --------------------------------------------------------- */

   else
   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);
       }
       {
       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)
     {
                  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");
 
     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 */
     }
 
   /* 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)
 {
 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)
 
 /* 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')
       {
       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;
       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);
 
        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(
 
        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
        continue;
        }
       else


@@ -1063,11 +1060,9 @@ else for (p = 0; p<len; p++)
       }
     else if (ctx->flags & PDKIM_SEEN_LF)
       {
       }
     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;
       }
 
       ctx->flags &= ~PDKIM_SEEN_LF;
       }
 

diff --git a/test/log/4506 b/test/log/4506
index d50bbe1f4545f9dcf9b8f2057c9d73dd837253b1..20e9c6b51c0595e116edcf71ed1718101dba03ca 100644 (file)
--- 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 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 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 b2d53e8fceaae1396cedcefb61f9ff3c4ef1042c..14bba834680d815c6a94ef3e5a5526fe65452292 100644 (file)
--- a/test/scripts/4500-DKIM/4506
+++ b/test/scripts/4500-DKIM/4506
@@ -1,6 +1,6 @@
 # DKIM verify, errors
 #
 # 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)
 ****
 #
 # This should fail verify (missing header hash in sig header)