DKIM: error verification on missing tags. Bug 1853

5 files changed, 66 insertions, 17 deletions

diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt
index 128aef3d1..ca64f9f11 100644
--- a/doc/doc-docbook/spec.xfpt
+++ b/doc/doc-docbook/spec.xfpt
@@ -38115,7 +38115,7 @@ used.
 .section "Verifying DKIM signatures in incoming mail" "SECID514"
 .cindex "DKIM" "verification"
 
-Verification of DKIM signatures in incoming email is implemented via the
+Verification of DKIM signatures in SMTP incoming email is implemented via the
 &%acl_smtp_dkim%& ACL. By default, this ACL is called once for each
 syntactically(!) correct signature in the incoming message.
 A missing ACL definition defaults to accept.
diff --git a/src/src/dkim.c b/src/src/dkim.c
index 4e20f14f2..24de2bc33 100644
--- a/src/src/dkim.c
+++ b/src/src/dkim.c
@@ -148,10 +148,12 @@ for (sig = dkim_signatures; sig; sig = sig->next)
 	string_sprintf("d=%s s=%s c=%s/%s a=%s b=%d ",
 	      sig->domain,
 	      sig->selector,
-	      sig->canon_headers == PDKIM_CANON_SIMPLE ?  "simple" : "relaxed",
-	      sig->canon_body == PDKIM_CANON_SIMPLE ?  "simple" : "relaxed",
-	      sig->algo == PDKIM_ALGO_RSA_SHA256 ?  "rsa-sha256" : "rsa-sha1",
-	      sig->sigdata.len * 8
+	      sig->canon_headers == PDKIM_CANON_SIMPLE ? "simple" : "relaxed",
+	      sig->canon_body == PDKIM_CANON_SIMPLE ? "simple" : "relaxed",
+	      sig->algo == PDKIM_ALGO_RSA_SHA256
+	      ? "rsa-sha256"
+	      : sig->algo == PDKIM_ALGO_RSA_SHA1 ? "rsa-sha1" : "err",
+	      (int)sig->sigdata.len > -1 ? sig->sigdata.len * 8 : 0
 	      ),
 
 	sig->identity ? string_sprintf("i=%s ", sig->identity) : US"",
@@ -186,6 +188,16 @@ for (sig = dkim_signatures; sig; sig = sig->next)
 		       "syntax error in public key record]");
 	  break;
 
+        case PDKIM_VERIFY_INVALID_SIGNATURE_ERROR:
+          logmsg = string_append(logmsg, &size, &ptr, 1,
+                       "signature tag missing or invalid]");
+          break;
+
+        case PDKIM_VERIFY_INVALID_DKIM_VERSION:
+          logmsg = string_append(logmsg, &size, &ptr, 1,
+                       "unsupported DKIM version]");
+          break;
+
 	default:
 	  logmsg = string_append(logmsg, &size, &ptr, 1,
 			"unspecified problem]");
diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c
index 29277baeb..a0f022685 100644
--- a/src/src/pdkim/pdkim.c
+++ b/src/src/pdkim/pdkim.c
@@ -134,6 +134,8 @@ pdkim_verify_ext_status_str(int ext_status)
     case PDKIM_VERIFY_INVALID_BUFFER_SIZE: return "PDKIM_VERIFY_INVALID_BUFFER_SIZE";
     case PDKIM_VERIFY_INVALID_PUBKEY_DNSRECORD: return "PDKIM_VERIFY_INVALID_PUBKEY_DNSRECORD";
     case PDKIM_VERIFY_INVALID_PUBKEY_IMPORT: return "PDKIM_VERIFY_INVALID_PUBKEY_IMPORT";
+    case PDKIM_VERIFY_INVALID_SIGNATURE_ERROR: return "PDKIM_VERIFY_INVALID_SIGNATURE_ERROR";
+    case PDKIM_VERIFY_INVALID_DKIM_VERSION: return "PDKIM_VERIFY_INVALID_DKIM_VERSION";
     default: return "PDKIM_VERIFY_UNKNOWN";
   }
 }
@@ -407,6 +409,10 @@ sig = store_get(sizeof(pdkim_signature));
 memset(sig, 0, sizeof(pdkim_signature));
 sig->bodylength = -1;
 
+/* Set so invalid/missing data error display is accurate */
+sig->algo = -1;
+sig->version = 0;
+
 q = sig->rawsig_no_b_val = store_get(Ustrlen(raw_hdr)+1);
 
 for (p = raw_hdr; ; p++)
@@ -476,8 +482,8 @@ for (p = raw_hdr; ; p++)
 	  case 'v':
 	      /* We only support version 1, and that is currently the
 		 only version there is. */
-	    if (Ustrcmp(cur_val, PDKIM_SIGNATURE_VERSION) == 0)
-	      sig->version = 1;
+	    sig->version =
+	      Ustrcmp(cur_val, PDKIM_SIGNATURE_VERSION) == 0 ? 1 : -1;
 	    break;
 	  case 'a':
 	    for (i = 0; pdkim_algos[i]; i++)
@@ -542,10 +548,6 @@ NEXT_CHAR:
     *q++ = c;
   }
 
-/* Make sure the most important bits are there. */
-if (!sig->version)
-  return NULL;
-
 *q = '\0';
 /* Chomp raw header. The final newline must not be added to the signature. */
 while (--q > sig->rawsig_no_b_val  && (*q == '\r' || *q == '\n'))
@@ -1475,6 +1477,37 @@ while (sig)
 
     uschar *dns_txt_name, *dns_txt_reply;
 
+    /* Make sure we have all required signature tags */
+    if (!(  sig->domain        && *sig->domain
+	 && sig->selector      && *sig->selector
+	 && sig->headernames   && *sig->headernames
+	 && sig->bodyhash.data
+	 && sig->sigdata.data
+	 && sig->algo > -1
+	 && sig->version
+       ) )
+      {
+      sig->verify_status     = PDKIM_VERIFY_INVALID;
+      sig->verify_ext_status = PDKIM_VERIFY_INVALID_SIGNATURE_ERROR;
+
+      DEBUG(D_acl) debug_printf(
+	  " Error in DKIM-Signature header: tags missing or invalid\n"
+	  "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n");
+      goto NEXT_VERIFY;
+      }
+
+    /* Make sure sig uses supported DKIM version (only v1) */
+    if (sig->version != 1)
+      {
+      sig->verify_status     = PDKIM_VERIFY_INVALID;
+      sig->verify_ext_status = PDKIM_VERIFY_INVALID_DKIM_VERSION;
+
+      DEBUG(D_acl) debug_printf(
+          " Error in DKIM-Signature header: unsupported DKIM version\n"
+          "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n");
+      goto NEXT_VERIFY;
+      }
+
     /* Fetch public key for signing domain, from DNS */
 
     dns_txt_name = string_sprintf("%s._domainkey.%s.",
diff --git a/src/src/pdkim/pdkim.h b/src/src/pdkim/pdkim.h
index ba984c1d9..0803ea0b0 100644
--- a/src/src/pdkim/pdkim.h
+++ b/src/src/pdkim/pdkim.h
@@ -49,12 +49,14 @@
 #define PDKIM_VERIFY_FAIL      2
 #define PDKIM_VERIFY_PASS      3
 
-#define PDKIM_VERIFY_FAIL_BODY                  1
-#define PDKIM_VERIFY_FAIL_MESSAGE               2
-#define PDKIM_VERIFY_INVALID_PUBKEY_UNAVAILABLE 3
-#define PDKIM_VERIFY_INVALID_BUFFER_SIZE        4
-#define PDKIM_VERIFY_INVALID_PUBKEY_DNSRECORD   5
-#define PDKIM_VERIFY_INVALID_PUBKEY_IMPORT      6
+#define PDKIM_VERIFY_FAIL_BODY                    1
+#define PDKIM_VERIFY_FAIL_MESSAGE                 2
+#define PDKIM_VERIFY_INVALID_PUBKEY_UNAVAILABLE   3
+#define PDKIM_VERIFY_INVALID_BUFFER_SIZE          4
+#define PDKIM_VERIFY_INVALID_PUBKEY_DNSRECORD     5
+#define PDKIM_VERIFY_INVALID_PUBKEY_IMPORT        6
+#define PDKIM_VERIFY_INVALID_SIGNATURE_ERROR      7
+#define PDKIM_VERIFY_INVALID_DKIM_VERSION         8
 
 /* -------------------------------------------------------------------------- */
 /* Some parameter values */
diff --git a/src/src/verify.c b/src/src/verify.c
index 3624af0bc..b52533f7a 100644
--- a/src/src/verify.c
+++ b/src/src/verify.c
@@ -889,7 +889,9 @@ can do it there for the non-rcpt-verify case.  For this we keep an addresscount.
     /* Need proper integration with the proper transport mechanism. */
     if (cutthrough.delivery)
       {
+#ifndef DISABLE_DKIM
       uschar * s;
+#endif
       if (addr->transport->filter_command)
         {
         cutthrough.delivery = FALSE;