From 2e026f570832bcf05ad5c58533c9880785762da2 Mon Sep 17 00:00:00 2001 From: Joaquin Lopez Date: Mon, 27 Jun 2016 23:38:25 -0700 Subject: [PATCH] DKIM: Fix invalid signature verification issues * Fixes segfault when b/bh has invalid base64 * Handles missing/invalid tags more closely to how RFC4871 specifies * Makes error reporting a little more specific --- src/src/dkim.c | 15 +++++++++++++-- src/src/pdkim/pdkim.c | 45 +++++++++++++++++++++++++++++++++++++++---- src/src/pdkim/pdkim.h | 14 ++++++++------ 3 files changed, 62 insertions(+), 12 deletions(-) diff --git a/src/src/dkim.c b/src/src/dkim.c index 4e20f14f26..1ca06db009 100644 --- a/src/src/dkim.c +++ b/src/src/dkim.c @@ -150,8 +150,9 @@ for (sig = dkim_signatures; sig; sig = sig->next) 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->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 +187,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 29277baeb6..98cc736193 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++) @@ -478,6 +484,8 @@ for (p = raw_hdr; ; p++) only version there is. */ if (Ustrcmp(cur_val, PDKIM_SIGNATURE_VERSION) == 0) sig->version = 1; + else + sig->version = -1; break; case 'a': for (i = 0; pdkim_algos[i]; i++) @@ -542,10 +550,6 @@ for (p = raw_hdr; ; p++) *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')) @@ -943,9 +947,12 @@ if (ctx->mode == PDKIM_MODE_VERIFY) } } else + { DEBUG(D_acl) debug_printf( "Error while parsing signature header\n" "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n"); + goto BAIL; + } } /* every other header is stored for signature verification */ @@ -1475,6 +1482,36 @@ while (sig) uschar *dns_txt_name, *dns_txt_reply; + /* Make sure we have all required signature tags */ + if (!(sig->domain && (*(sig->domain) != '\0') && + sig->selector && (*(sig->selector) != '\0') && + sig->headernames && (*(sig->headernames) != '\0') && + 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 ba984c1d9e..0803ea0b08 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 */