From e21a4d0042e48109cef06e48b8a73dd79d7a4330 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Tue, 22 Nov 2016 15:22:11 +0000 Subject: [PATCH] DKIM: More validation of DNS key record. Bug 1926 --- src/src/pdkim/pdkim.c | 161 +++++++++--------- src/src/pdkim/pdkim.h | 3 +- test/dnszones-src/db.test.ex | 3 + test/log/4502 | 3 + .../4500-Domain-Keys-Identified-Mail/4502 | 54 +++++- 5 files changed, 142 insertions(+), 82 deletions(-) diff --git a/src/src/pdkim/pdkim.c b/src/src/pdkim/pdkim.c index 7bfcdf4aa..c4bdb5a93 100644 --- a/src/src/pdkim/pdkim.c +++ b/src/src/pdkim/pdkim.c @@ -603,90 +603,87 @@ pub = store_get(sizeof(pdkim_pubkey)); memset(pub, 0, sizeof(pdkim_pubkey)); for (p = raw_record; ; p++) - { - char c = *p; - - /* Ignore FWS */ - if (c == '\r' || c == '\n') - goto NEXT_CHAR; - - if (where == PDKIM_HDR_LIMBO) { - /* In limbo, just wait for a tag-char to appear */ - if (!(c >= 'a' && c <= 'z')) - goto NEXT_CHAR; - - where = PDKIM_HDR_TAG; - } + uschar c = *p; - if (where == PDKIM_HDR_TAG) - { - if (c >= 'a' && c <= 'z') - cur_tag = string_catn(cur_tag, &ts, &tl, p, 1); - - if (c == '=') + /* Ignore FWS */ + if (c != '\r' && c != '\n') switch (where) { - cur_tag[tl] = '\0'; - where = PDKIM_HDR_VALUE; - goto NEXT_CHAR; - } - } + case PDKIM_HDR_LIMBO: /* In limbo, just wait for a tag-char to appear */ + if (!(c >= 'a' && c <= 'z')) + break; + where = PDKIM_HDR_TAG; + /*FALLTHROUGH*/ - if (where == PDKIM_HDR_VALUE) - { - if (c == ';' || c == '\0') - { - if (tl && vl) - { - cur_val[vl] = '\0'; - pdkim_strtrim(cur_val); - DEBUG(D_acl) debug_printf(" %s=%s\n", cur_tag, cur_val); + case PDKIM_HDR_TAG: + if (c >= 'a' && c <= 'z') + cur_tag = string_catn(cur_tag, &ts, &tl, p, 1); - switch (cur_tag[0]) + if (c == '=') { - case 'v': - /* This tag isn't evaluated because: - - We only support version DKIM1. - - Which is the default for this value (set below) - - Other versions are currently not specified. */ - break; - case 'h': - case 'k': - pub->hashes = string_copy(cur_val); break; - case 'g': - pub->granularity = string_copy(cur_val); break; - case 'n': - pub->notes = pdkim_decode_qp(cur_val); break; - case 'p': - pdkim_decode_base64(US cur_val, &pub->key); - break; - case 's': - pub->srvtype = string_copy(cur_val); break; - case 't': - if (Ustrchr(cur_val, 'y') != NULL) pub->testing = 1; - if (Ustrchr(cur_val, 's') != NULL) pub->no_subdomaining = 1; - break; - default: - DEBUG(D_acl) debug_printf(" Unknown tag encountered\n"); - break; + cur_tag[tl] = '\0'; + where = PDKIM_HDR_VALUE; } - } - tl = 0; - vl = 0; - where = PDKIM_HDR_LIMBO; + break; + + case PDKIM_HDR_VALUE: + if (c == ';' || c == '\0') + { + if (tl && vl) + { + cur_val[vl] = '\0'; + pdkim_strtrim(cur_val); + DEBUG(D_acl) debug_printf(" %s=%s\n", cur_tag, cur_val); + + switch (cur_tag[0]) + { + case 'v': + pub->version = string_copy(cur_val); break; + case 'h': + case 'k': +/* This field appears to never be used. Also, unclear why +a 'k' (key-type_ would go in this field name. There is a field +"keytype", also never used. + pub->hashes = string_copy(cur_val); +*/ + break; + case 'g': + pub->granularity = string_copy(cur_val); break; + case 'n': + pub->notes = pdkim_decode_qp(cur_val); break; + case 'p': + pdkim_decode_base64(US cur_val, &pub->key); break; + case 's': + pub->srvtype = string_copy(cur_val); break; + case 't': + if (Ustrchr(cur_val, 'y') != NULL) pub->testing = 1; + if (Ustrchr(cur_val, 's') != NULL) pub->no_subdomaining = 1; + break; + default: + DEBUG(D_acl) debug_printf(" Unknown tag encountered\n"); + break; + } + } + tl = 0; + vl = 0; + where = PDKIM_HDR_LIMBO; + } + else + cur_val = string_catn(cur_val, &vs, &vl, p, 1); + break; } - else - cur_val = string_catn(cur_val, &vs, &vl, p, 1); - } -NEXT_CHAR: - if (c == '\0') break; - } + if (c == '\0') break; + } /* Set fallback defaults */ if (!pub->version ) pub->version = string_copy(PDKIM_PUB_RECORD_VERSION); +else if (Ustrcmp(pub->version, PDKIM_PUB_RECORD_VERSION) != 0) return NULL; + if (!pub->granularity) pub->granularity = string_copy(US"*"); +/* if (!pub->keytype ) pub->keytype = string_copy(US"rsa"); +*/ if (!pub->srvtype ) pub->srvtype = string_copy(US"*"); /* p= is required */ @@ -794,7 +791,7 @@ for (sig = ctx->sig; sig; sig = sig->next) DEBUG(D_acl) { debug_printf("PDKIM [%s] Body bytes hashed: %lu\n" - "PDKIM [%s] bh computed: ", + "PDKIM [%s] Body hash computed: ", sig->domain, sig->signed_body_bytes, sig->domain); pdkim_hexprint(CUS bh.data, bh.len); } @@ -822,7 +819,7 @@ for (sig = ctx->sig; sig; sig = sig->next) { DEBUG(D_acl) { - debug_printf("PDKIM [%s] bh signature: ", sig->domain); + debug_printf("PDKIM [%s] Body hash signature from headers: ", sig->domain); pdkim_hexprint(sig->bodyhash.data, exim_sha_hashlen(&sig->body_hash)); debug_printf("PDKIM [%s] Body hash did NOT verify\n", sig->domain); @@ -1341,7 +1338,7 @@ while (sig) exim_sha_init(&hhash_ctx, is_sha1 ? HASH_SHA1 : HASH_SHA256); DEBUG(D_acl) debug_printf( - "PDKIM >> Hashed header data, canonicalized, in sequence >>>>>>>>>>>>>>\n"); + "PDKIM >> Header data for hash, canonicalized, in sequence >>>>>>>>>>>>>>\n"); /* SIGNING ---------------------------------------------------------------- */ /* When signing, walk through our header list and add them to the hash. As we @@ -1471,7 +1468,7 @@ while (sig) DEBUG(D_acl) { - debug_printf("PDKIM [%s] hh computed: ", sig->domain); + debug_printf("PDKIM [%s] Header hash computed: ", sig->domain); pdkim_hexprint(hhash.data, hhash.len); } @@ -1520,6 +1517,7 @@ while (sig) { ev_ctx vctx; const uschar * errstr; + pdkim_pubkey * p; uschar *dns_txt_name, *dns_txt_reply; @@ -1578,16 +1576,25 @@ while (sig) pdkim_quoteprint(CUS dns_txt_reply, Ustrlen(dns_txt_reply)); } - if (!(sig->pubkey = pdkim_parse_pubkey_record(ctx, CUS dns_txt_reply))) + if ( !(p = pdkim_parse_pubkey_record(ctx, CUS dns_txt_reply)) + || (Ustrcmp(p->srvtype, "*") != 0 && Ustrcmp(p->srvtype, "email") != 0) + ) { sig->verify_status = PDKIM_VERIFY_INVALID; sig->verify_ext_status = PDKIM_VERIFY_INVALID_PUBKEY_DNSRECORD; - DEBUG(D_acl) debug_printf( - " Error while parsing public key record\n" + DEBUG(D_acl) + { + if (p) + debug_printf(" Invalid public key service type '%s'\n", p->srvtype); + else + debug_printf(" Error while parsing public key record\n"); + debug_printf( "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n"); + } goto NEXT_VERIFY; } + sig->pubkey = p; DEBUG(D_acl) debug_printf( "PDKIM <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n"); diff --git a/src/src/pdkim/pdkim.h b/src/src/pdkim/pdkim.h index 536e5af69..07ba5b5c4 100644 --- a/src/src/pdkim/pdkim.h +++ b/src/src/pdkim/pdkim.h @@ -100,13 +100,14 @@ typedef struct pdkim_pubkey { uschar *version; /* v= */ uschar *granularity; /* g= */ +#ifdef notdef uschar *hashes; /* h= */ uschar *keytype; /* k= */ +#endif uschar *srvtype; /* s= */ uschar *notes; /* n= */ blob key; /* p= */ - int testing; /* t=y */ int no_subdomaining; /* t=s */ } pdkim_pubkey; diff --git a/test/dnszones-src/db.test.ex b/test/dnszones-src/db.test.ex index 6555ec81d..16468bf79 100644 --- a/test/dnszones-src/db.test.ex +++ b/test/dnszones-src/db.test.ex @@ -476,9 +476,12 @@ DELAY=1500 delay1500 A HOSTIPV4 ; openssl genrsa -out aux-fixed/dkim/dkim.private 1024 ; openssl rsa -in aux-fixed/dkim/dkim.private -out /dev/stdout -pubout -outform PEM ; +; Deliberate bad version, having extra backslashes +; ; Another, 512-bit (with a Notes field) ; sel._domainkey TXT "v=DKIM1; p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDXRFf+VhT+lCgFhhSkinZKcFNeRzjYdW8vT29Rbb3NadvTFwAd+cVLPFwZL8H5tUD/7JbUPqNTCPxmpgIL+V5T4tEZMorHatvvUM2qfcpQ45IfsZ+YdhbIiAslHCpy4xNxIR3zylgqRUF4+Dtsaqy3a5LhwMiKCLrnzhXk1F1hxwIDAQAB" +sel_bad._domainkey TXT "v=DKIM1\; p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDXRFf+VhT+lCgFhhSkinZKcFNeRzjYdW8vT29Rbb3NadvTFwAd+cVLPFwZL8H5tUD/7JbUPqNTCPxmpgIL+V5T4tEZMorHatvvUM2qfcpQ45IfsZ+YdhbIiAslHCpy4xNxIR3zylgqRUF4+Dtsaqy3a5LhwMiKCLrnzhXk1F1hxwIDAQAB" ses._domainkey TXT "v=DKIM1; n=halfkilo; p=MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBAL6eAQxd9didJ0/+05iDwJOqT6ly826Vi8aGPecsBiYK5/tAT97fxXk+dPWMZp9kQxtknEzYjYjAydzf+HQ2yJMCAwEAAQ==" diff --git a/test/log/4502 b/test/log/4502 index ab5273ad0..b7e4a8ddd 100644 --- a/test/log/4502 +++ b/test/log/4502 @@ -10,3 +10,6 @@ 1999-03-02 09:44:33 10HmaZ-0005vi-00 DKIM: d=test.ex s=sel c=relaxed/simple a=rsa-sha1 b=1024 [verification succeeded] 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 +1999-03-02 09:44:33 10HmbA-0005vi-00 DKIM: d=test.ex s=sel_bad c=relaxed/relaxed a=rsa-sha1 b=1024 [invalid - syntax error in public key record] +1999-03-02 09:44:33 10HmbA-0005vi-00 signer: test.ex bits: 1024 +1999-03-02 09:44:33 10HmbA-0005vi-00 <= CALLER@bloggs.com H=(xxx) [127.0.0.1] P=smtp S=sss id=564CFC9B.1040905@yahoo.com diff --git a/test/scripts/4500-Domain-Keys-Identified-Mail/4502 b/test/scripts/4500-Domain-Keys-Identified-Mail/4502 index 5e63f129f..1e0005b46 100644 --- a/test/scripts/4500-Domain-Keys-Identified-Mail/4502 +++ b/test/scripts/4500-Domain-Keys-Identified-Mail/4502 @@ -1,11 +1,11 @@ -# DKIM relaxed canonicalisation +# DKIM verify, relaxed canonicalisation # exim -DSERVER=server -bd -oX PORT_D **** # # This should pass. # Mail original in aux-fixed/4502.msg1.txt -# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed/relaxed < aux_fixed/4502.msg1.txt +# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed/relaxed < aux-fixed/4502.msg1.txt client 127.0.0.1 PORT_D ??? 220 HELO xxx @@ -52,7 +52,7 @@ QUIT # # This should pass. # Mail original in aux-fixed/4502.msg2.txt -# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed < aux_fixed/4502.msg2.txt +# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed < aux-fixed/4502.msg2.txt client 127.0.0.1 PORT_D ??? 220 HELO xxx @@ -93,7 +93,7 @@ QUIT # # This should pass. # Mail original in aux-fixed/4502.msg3.txt -# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed < aux_fixed/4502.msg3.txt +# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed < aux-fixed/4502.msg3.txt client 127.0.0.1 PORT_D ??? 220 HELO xxx @@ -130,6 +130,52 @@ QUIT ??? 221 **** # +# This should fail, but passes - bug 1926 - due to an extra \ in the DNS record. +# Mail original in aux-fixed/4502.msg1.txt +# Sig generated by: perl aux-fixed/dkim/sign.pl --method=relaxed/relaxed --selector=sel_bad < aux-fixed/4502.msg1.txt +client 127.0.0.1 PORT_D +??? 220 +HELO xxx +??? 250 +MAIL FROM: +??? 250 +RCPT TO: +??? 250 +DATA +??? 354 +DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=test.ex; h= + message-id:date:from:mime-version:to:subject:content-type + :content-transfer-encoding; s=sel_bad; bh=rn0kk3aPKyhYbxzfi3WG8d + AxhNM=; b=kXWfssgeNTAHmr9u2U6VZvb8uXuzoeLtZqgxySmUERKBsjk9sV31yv + 3rEMCwdtM38yBNFK9zuLsoBUO6M7fGnpfgbGv7BnDHx8AJcsPc1Ay/7JbLKhiCxo + zMTFil/4pj1s3bQGLCCOcN688IgerUUFqNBM5vq0nIOKzj2dwhQC8= +Message-ID: <564CFC9B.1040905@yahoo.com> +Date: Wed, 18 Nov 2015 14:32:59 -0800 +From: Joaquin Lopez +User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:24.0) Gecko/20100101 Thunderbird/24.0 +MIME-Version: 1.0 +To: bakawolf@yahoo.com +Subject: test +Content-Type: text/plain; charset=ISO-8859-1; format=flowed +Content-Transfer-Encoding: 7bit +Content-Length: 13 + + + +test + + + + + + + + +. +??? 250 +QUIT +??? 221 +**** killdaemon no_stdout_check no_msglog_check -- 2.25.1