From ce93c6d840d56b3cd7b2ced65c4e96820040c8a7 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Thu, 28 Dec 2017 20:09:05 +0000 Subject: [PATCH] Fix crash associated with dnsdb lookup done from DKIM ACL. Bug 2215 Broken-by: cc55f4208e --- doc/doc-txt/ChangeLog | 12 ++++++++++ src/src/expand.c | 48 +++++++++++++++++++++++-------------- src/src/queue.c | 1 + test/confs/4500 | 3 +++ test/log/4506 | 5 ++++ test/scripts/4500-DKIM/4506 | 34 ++++++++++++++++++++++++++ 6 files changed, 85 insertions(+), 18 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 7ec669b1c..7d1d526d7 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -25,6 +25,18 @@ JH/04 Bug 2217: Tighten up the parsing of DKIM signature headers. Previously Assumptions at that stage could crash the receive process on malformed input. +JH/05 Bug 2215: Fix crash associated with dnsdb lookup done from DKIM ACL. + While running the DKIM ACL we operate on the Permanent memory pool so that + variables created with "set" persist to the DATA ACL. Also (at any time) + DNS lookups that fail create cache records using the Permanent pool. But + expansions release any allocations made on the current pool - so a dnsdb + lookup expansion done in the DKIM ACL releases the memory used for the + DNS negative-cache, and bad things result. Solution is to switch to the + Main pool for expansions. + While we're in that code, add checks on the DNS cache during store_reset, + active in the testsuite. + Problem spotted, and debugging aided, by Wolfgang Breyha. + Exim version 4.90 ----------------- diff --git a/src/src/expand.c b/src/src/expand.c index e754fbc8c..3a63a7c78 100644 --- a/src/src/expand.c +++ b/src/src/expand.c @@ -7546,7 +7546,7 @@ DEBUG(D_expand) if (expand_string_forcedfail) debug_printf_indent(UTF8_UP_RIGHT "failure was forced\n"); } -if (resetok_p) *resetok_p = resetok; +if (resetok_p && !resetok) *resetok_p = FALSE; expand_level--; return NULL; } @@ -7560,28 +7560,35 @@ Returns: the expanded string, or NULL if expansion failed; if failure was due to a lookup deferring, search_find_defer will be TRUE */ -uschar * -expand_string(uschar *string) +const uschar * +expand_cstring(const uschar * string) { -search_find_defer = FALSE; -malformed_header = FALSE; -return (Ustrpbrk(string, "$\\") == NULL)? string : - expand_string_internal(string, FALSE, NULL, FALSE, TRUE, NULL); +if (Ustrpbrk(string, "$\\") != NULL) + { + int old_pool = store_pool; + uschar * s; + + search_find_defer = FALSE; + malformed_header = FALSE; + store_pool = POOL_MAIN; + s = expand_string_internal(string, FALSE, NULL, FALSE, TRUE, NULL); + store_pool = old_pool; + return s; + } +return string; } - -const uschar * -expand_cstring(const uschar *string) +uschar * +expand_string(uschar * string) { -search_find_defer = FALSE; -malformed_header = FALSE; -return (Ustrpbrk(string, "$\\") == NULL)? string : - expand_string_internal(string, FALSE, NULL, FALSE, TRUE, NULL); +return US expand_cstring(CUS string); } + + /************************************************* * Expand and copy * *************************************************/ @@ -7812,8 +7819,6 @@ return ( ( Ustrstr(s, "failed to expand") != NULL * Error-checking for testsuite * *************************************************/ typedef struct { - const char * filename; - int linenumber; uschar * region_start; uschar * region_end; const uschar *var_name; @@ -7834,7 +7839,8 @@ if (var_data >= e->region_start && var_data < e->region_end) void assert_no_variables(void * ptr, int len, const char * filename, int linenumber) { -err_ctx e = {filename, linenumber, ptr, US ptr + len, NULL }; +err_ctx e = { .region_start = ptr, .region_end = US ptr + len, + .var_name = NULL, .var_data = NULL }; int i; var_entry * v; @@ -7855,10 +7861,16 @@ for (v = var_table; v < var_table + var_table_size; v++) if (v->type == vtype_stringptr) assert_variable_notin(US v->name, *(USS v->value), &e); +/* check dns and address trees */ +tree_walk(tree_dns_fails, assert_variable_notin, &e); +tree_walk(tree_duplicates, assert_variable_notin, &e); +tree_walk(tree_nonrecipients, assert_variable_notin, &e); +tree_walk(tree_unusable, assert_variable_notin, &e); + if (e.var_name) log_write(0, LOG_MAIN|LOG_PANIC_DIE, "live variable '%s' destroyed by reset_store at %s:%d\n- value '%.64s'", - e.var_name, e.filename, e.linenumber, e.var_data); + e.var_name, filename, linenumber, e.var_data); } diff --git a/src/src/queue.c b/src/src/queue.c index f94681197..09a149e9a 100644 --- a/src/src/queue.c +++ b/src/src/queue.c @@ -699,6 +699,7 @@ for (i = queue_run_in_order ? -1 : 0; } } /* End loop for list of messages */ + tree_nonrecipients = NULL; store_reset(reset_point1); /* Scavenge list of messages */ /* If this was the first time through for random order processing, and diff --git a/test/confs/4500 b/test/confs/4500 index f2e44beff..b53dff5b7 100644 --- a/test/confs/4500 +++ b/test/confs/4500 @@ -18,6 +18,9 @@ queue_run_in_order begin acl check_dkim: +.ifdef BAD + warn logwrite = ${lookup dnsdb{defer_never,txt=_adsp._domainkey.$dkim_cur_signer}{$value}{unknown}} +.endif .ifdef OPTION warn condition = ${if eq {$dkim_algo}{rsa-sha1}} condition = ${if eq {$dkim_verify_status}{pass}} diff --git a/test/log/4506 b/test/log/4506 index 07b3ee8ce..995dbde98 100644 --- a/test/log/4506 +++ b/test/log/4506 @@ -16,3 +16,8 @@ 1999-03-02 09:44:33 10HmbB-0005vi-00 signer: test.ex bits: 512 1999-03-02 09:44:33 10HmbB-0005vi-00 DKIM: d=test.ex s=ses_sha256 c=simple/simple a=rsa-sha1 b=512 [verification failed - unspecified reason] 1999-03-02 09:44:33 10HmbB-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 exim x.yz daemon started: pid=pppp, no queue runs, listening for SMTP on port 1225 +1999-03-02 09:44:33 10HmbC-0005vi-00 unknown +1999-03-02 09:44:33 10HmbC-0005vi-00 signer: test.ex bits: 0 +1999-03-02 09:44:33 10HmbC-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 10HmbC-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 e8d7c41f0..4499315d2 100644 --- a/test/scripts/4500-DKIM/4506 +++ b/test/scripts/4500-DKIM/4506 @@ -161,6 +161,40 @@ Date: Thu, 19 Nov 2015 17:00:07 -0700 Message-ID: Subject: simple test +This is a simple test. +. +??? 250 +QUIT +??? 221 +**** +killdaemon +# +# +# See what happens when we do a DNS lookup from the DKIM ACL +exim -DSERVER=server -DBAD=bad -bd -oX PORT_D +**** +# This should fail verify (missing header hash in sig header) +# - sha1, 1024b +# Mail original in aux-fixed/4500.msg1.txt +# Sig generated by: perl aux-fixed/dkim/sign.pl --method=simple/simple < aux-fixed/4500.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=simple/simple; d=test.ex; h=from:to + :date:message-id:subject; s=sel; bh=OB9dZVu7+5/ufs3TH9leIcEpXSo=; +From: mrgus@text.ex +To: bakawolf@yahoo.com +Date: Thu, 19 Nov 2015 17:00:07 -0700 +Message-ID: +Subject: simple test + This is a simple test. . ??? 250 -- 2.30.2