From 07eeb4df55e6c0843156aedaacf32bfebe0e9eaa Mon Sep 17 00:00:00 2001 From: mrgus Date: Fri, 15 Jul 2016 00:37:54 +0100 Subject: [PATCH] DKIM: error verification on missing tags. Bug 1853 --- doc/doc-docbook/spec.xfpt | 2 +- src/src/dkim.c | 20 +++++++++++++---- src/src/pdkim/pdkim.c | 45 +++++++++++++++++++++++++++++++++------ src/src/pdkim/pdkim.h | 14 ++++++------ src/src/verify.c | 2 ++ 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; -- 2.30.2