From 101702337f7eb652fc99f440b64904b1e1629c2f Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Mon, 23 Apr 2018 12:41:31 +0100 Subject: [PATCH] Avoid doing logging in signal-handlers. Bug 1007 Cherry-picked from 9723f96673, with the LOCAL_SCAN changes removed --- doc/doc-txt/ChangeLog | 7 ++ src/src/functions.h | 4 + src/src/globals.c | 4 + src/src/globals.h | 4 + src/src/local_scan.c | 1 + src/src/receive.c | 193 +++++++++++++++++++++++------------------- src/src/smtp_in.c | 82 +++++++++++++----- src/src/spool_out.c | 4 +- src/src/tls-gnu.c | 20 +++-- src/src/tls-openssl.c | 11 ++- 10 files changed, 216 insertions(+), 114 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index b4c98572f..58673c23a 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -5,6 +5,13 @@ affect Exim's operation, with an unchanged configuration file. For new options, and new features, see the NewStuff file next to this ChangeLog. +Since version 4.91 +------------------ + +JH/02 Bug 1007: Avoid doing logging from signal-handlers, as that can result in + non-signal-safe functions being used. + + Exim version 4.91 ----------------- diff --git a/src/src/functions.h b/src/src/functions.h index 9be5c32a4..5f9deab62 100644 --- a/src/src/functions.h +++ b/src/src/functions.h @@ -427,6 +427,10 @@ extern int sieve_interpret(uschar *, int, uschar *, uschar *, uschar *, extern void sigalrm_handler(int); extern BOOL smtp_buffered(void); extern void smtp_closedown(uschar *); +extern void smtp_command_timeout_exit(void); +extern void smtp_command_sigterm_exit(void); +extern void smtp_data_timeout_exit(void); +extern void smtp_data_sigint_exit(void); extern uschar *smtp_cmd_hist(void); extern int smtp_connect(host_item *, int, uschar *, int, transport_instance *); diff --git a/src/src/globals.c b/src/src/globals.c index 7d18b38b5..5b4805723 100644 --- a/src/src/globals.c +++ b/src/src/globals.c @@ -770,6 +770,10 @@ uschar *gecos_name = NULL; uschar *gecos_pattern = NULL; rewrite_rule *global_rewrite_rules = NULL; +volatile sig_atomic_t had_command_timeout = 0; +volatile sig_atomic_t had_command_sigterm = 0; +volatile sig_atomic_t had_data_timeout = 0; +volatile sig_atomic_t had_data_sigint = 0; uschar *headers_charset = US HEADERS_CHARSET; int header_insert_maxlen = 64 * 1024; header_line *header_last = NULL; diff --git a/src/src/globals.h b/src/src/globals.h index da1230b7f..2915513f2 100644 --- a/src/src/globals.h +++ b/src/src/globals.h @@ -492,6 +492,10 @@ extern uschar *gecos_name; /* To be expanded when pattern matches */ extern uschar *gecos_pattern; /* Pattern to match */ extern rewrite_rule *global_rewrite_rules; /* Chain of rewriting rules */ +extern volatile sig_atomic_t had_command_timeout; /* Alarm sighandler called */ +extern volatile sig_atomic_t had_command_sigterm; /* TERM sighandler called */ +extern volatile sig_atomic_t had_data_timeout; /* Alarm sighandler called */ +extern volatile sig_atomic_t had_data_sigint; /* TERM/INT sighandler called */ extern int header_insert_maxlen; /* Max for inserting headers */ extern int header_maxsize; /* Max total length for header */ extern int header_line_maxsize; /* Max for an individual line */ diff --git a/src/src/local_scan.c b/src/src/local_scan.c index 3500047ca..4dd0b2baa 100644 --- a/src/src/local_scan.c +++ b/src/src/local_scan.c @@ -12,6 +12,7 @@ If you want to implement your own version, you should copy this file to, say Local/local_scan.c, and edit the copy. To use your version instead of the default, you must set +HAVE_LOCAL_SCAN=yes LOCAL_SCAN_SOURCE=Local/local_scan.c in your Local/Makefile. This makes it easy to copy your version for use with diff --git a/src/src/receive.c b/src/src/receive.c index cba53c20d..3b215f23e 100644 --- a/src/src/receive.c +++ b/src/src/receive.c @@ -8,6 +8,7 @@ /* Code for receiving a message and setting up spool files. */ #include "exim.h" +#include #ifdef EXPERIMENTAL_DCC extern int dcc_ok; @@ -27,6 +28,10 @@ static uschar *spool_name = US""; enum CH_STATE {LF_SEEN, MID_LINE, CR_SEEN}; +jmp_buf local_scan_env; /* error-handling context for local_scan */ +unsigned had_local_scan_crash; +unsigned had_local_scan_timeout; + /************************************************* * Non-SMTP character reading functions * @@ -40,7 +45,27 @@ changing the pointer variables.) */ int stdin_getc(unsigned lim) { -return getc(stdin); +int c = getc(stdin); + +if (had_data_timeout) + { + fprintf(stderr, "exim: timed out while reading - message abandoned\n"); + log_write(L_lost_incoming_connection, + LOG_MAIN, "timed out while reading local message"); + receive_bomb_out(US"data-timeout", NULL); /* Does not return */ + } +if (had_data_sigint) + { + if (filter_test == FTEST_NONE) + { + fprintf(stderr, "\nexim: %s received - message abandoned\n", + had_data_sigint == SIGTERM ? "SIGTERM" : "SIGINT"); + log_write(0, LOG_MAIN, "%s received while reading local message", + had_data_sigint == SIGTERM ? "SIGTERM" : "SIGINT"); + } + receive_bomb_out(US"signal-exit", NULL); /* Does not return */ + } +return c; } int @@ -316,11 +341,13 @@ if (spool_name[0] != '\0') /* Now close the file if it is open, either as a fd or a stream. */ -if (data_file != NULL) +if (data_file) { (void)fclose(data_file); data_file = NULL; -} else if (data_fd >= 0) { + } +else if (data_fd >= 0) + { (void)close(data_fd); data_fd = -1; } @@ -361,27 +388,7 @@ Returns: nothing static void data_timeout_handler(int sig) { -uschar *msg = NULL; - -sig = sig; /* Keep picky compilers happy */ - -if (smtp_input) - { - msg = US"SMTP incoming data timeout"; - log_write(L_lost_incoming_connection, - LOG_MAIN, "SMTP data timeout (message abandoned) on connection " - "from %s F=<%s>", - (sender_fullhost != NULL)? sender_fullhost : US"local process", - sender_address); - } -else - { - fprintf(stderr, "exim: timed out while reading - message abandoned\n"); - log_write(L_lost_incoming_connection, - LOG_MAIN, "timed out while reading local message"); - } - -receive_bomb_out(US"data-timeout", msg); /* Does not return */ +had_data_timeout = sig; } @@ -391,7 +398,18 @@ receive_bomb_out(US"data-timeout", msg); /* Does not return */ *************************************************/ /* Handler function for timeouts that occur while running a local_scan() -function. +function. Posix recommends against calling longjmp() from a signal-handler, +but the GCC manual says you can so we will, and trust that it's better than +calling probably non-signal-safe functions during logging from within the +handler, even with other compilers. + +See also https://cwe.mitre.org/data/definitions/745.html which also lists +it as unsafe. + +This is all because we have no control over what might be written for a +local-scan function, so cannot sprinkle had-signal checks after each +call-site. At least with the default "do-nothing" function we won't +ever get here. Argument: the signal number Returns: nothing @@ -400,11 +418,8 @@ Returns: nothing static void local_scan_timeout_handler(int sig) { -sig = sig; /* Keep picky compilers happy */ -log_write(0, LOG_MAIN|LOG_REJECT, "local_scan() function timed out - " - "message temporarily rejected (size %d)", message_size); -/* Does not return */ -receive_bomb_out(US"local-scan-timeout", US"local verification problem"); +had_local_scan_timeout = sig; +siglongjmp(local_scan_env, 1); } @@ -423,13 +438,12 @@ Returns: nothing static void local_scan_crash_handler(int sig) { -log_write(0, LOG_MAIN|LOG_REJECT, "local_scan() function crashed with " - "signal %d - message temporarily rejected (size %d)", sig, message_size); -/* Does not return */ -receive_bomb_out(US"local-scan-error", US"local verification problem"); +had_local_scan_crash = sig; +siglongjmp(local_scan_env, 1); } + /************************************************* * SIGTERM or SIGINT received * *************************************************/ @@ -444,26 +458,7 @@ Returns: nothing static void data_sigterm_sigint_handler(int sig) { -uschar *msg = NULL; - -if (smtp_input) - { - msg = US"Service not available - SIGTERM or SIGINT received"; - log_write(0, LOG_MAIN, "%s closed after %s", smtp_get_connection_info(), - (sig == SIGTERM)? "SIGTERM" : "SIGINT"); - } -else - { - if (filter_test == FTEST_NONE) - { - fprintf(stderr, "\nexim: %s received - message abandoned\n", - (sig == SIGTERM)? "SIGTERM" : "SIGINT"); - log_write(0, LOG_MAIN, "%s received while reading local message", - (sig == SIGTERM)? "SIGTERM" : "SIGINT"); - } - } - -receive_bomb_out(US"signal-exit", msg); /* Does not return */ +had_data_sigint = sig; } @@ -1678,6 +1673,7 @@ int dmarc_up = 0; uschar *timestamp; int tslen; + /* Release any open files that might have been cached while preparing to accept the message - e.g. by verifying addresses - because reading a message might take a fair bit of real time. */ @@ -1751,7 +1747,9 @@ received_time = message_id_tv; /* If SMTP input, set the special handler for timeouts. The alarm() calls happen in the smtp_getc() function when it refills its buffer. */ -if (smtp_input) os_non_restarting_signal(SIGALRM, data_timeout_handler); +had_data_timeout = 0; +if (smtp_input) + os_non_restarting_signal(SIGALRM, data_timeout_handler); /* If not SMTP input, timeout happens only if configured, and we just set a single timeout for the whole message. */ @@ -1764,6 +1762,7 @@ else if (receive_timeout > 0) /* SIGTERM and SIGINT are caught always. */ +had_data_sigint = 0; signal(SIGTERM, data_sigterm_sigint_handler); signal(SIGINT, data_sigterm_sigint_handler); @@ -3638,43 +3637,65 @@ dcc_ok = 0; version supplied with Exim always accepts, but this is a hook for sysadmins to supply their own checking code. The local_scan() function is run even when all the recipients have been discarded. */ -/*XXS could we avoid this for the standard case, given that few people will use it? */ lseek(data_fd, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET); /* Arrange to catch crashes in local_scan(), so that the -D file gets deleted, and the incident gets logged. */ -os_non_restarting_signal(SIGSEGV, local_scan_crash_handler); -os_non_restarting_signal(SIGFPE, local_scan_crash_handler); -os_non_restarting_signal(SIGILL, local_scan_crash_handler); -os_non_restarting_signal(SIGBUS, local_scan_crash_handler); - -DEBUG(D_receive) debug_printf("calling local_scan(); timeout=%d\n", - local_scan_timeout); -local_scan_data = NULL; - -os_non_restarting_signal(SIGALRM, local_scan_timeout_handler); -if (local_scan_timeout > 0) alarm(local_scan_timeout); -rc = local_scan(data_fd, &local_scan_data); -alarm(0); -os_non_restarting_signal(SIGALRM, sigalrm_handler); - -enable_dollar_recipients = FALSE; - -store_pool = POOL_MAIN; /* In case changed */ -DEBUG(D_receive) debug_printf("local_scan() returned %d %s\n", rc, - local_scan_data); - -os_non_restarting_signal(SIGSEGV, SIG_DFL); -os_non_restarting_signal(SIGFPE, SIG_DFL); -os_non_restarting_signal(SIGILL, SIG_DFL); -os_non_restarting_signal(SIGBUS, SIG_DFL); +if (sigsetjmp(local_scan_env, 1) == 0) + { + had_local_scan_crash = 0; + os_non_restarting_signal(SIGSEGV, local_scan_crash_handler); + os_non_restarting_signal(SIGFPE, local_scan_crash_handler); + os_non_restarting_signal(SIGILL, local_scan_crash_handler); + os_non_restarting_signal(SIGBUS, local_scan_crash_handler); + + DEBUG(D_receive) debug_printf("calling local_scan(); timeout=%d\n", + local_scan_timeout); + local_scan_data = NULL; + + had_local_scan_timeout = 0; + os_non_restarting_signal(SIGALRM, local_scan_timeout_handler); + if (local_scan_timeout > 0) alarm(local_scan_timeout); + rc = local_scan(data_fd, &local_scan_data); + alarm(0); + os_non_restarting_signal(SIGALRM, sigalrm_handler); + + enable_dollar_recipients = FALSE; + + store_pool = POOL_MAIN; /* In case changed */ + DEBUG(D_receive) debug_printf("local_scan() returned %d %s\n", rc, + local_scan_data); + + os_non_restarting_signal(SIGSEGV, SIG_DFL); + os_non_restarting_signal(SIGFPE, SIG_DFL); + os_non_restarting_signal(SIGILL, SIG_DFL); + os_non_restarting_signal(SIGBUS, SIG_DFL); + } +else + { + if (had_local_scan_crash) + { + log_write(0, LOG_MAIN|LOG_REJECT, "local_scan() function crashed with " + "signal %d - message temporarily rejected (size %d)", + had_local_scan_crash, message_size); + /* Does not return */ + receive_bomb_out(US"local-scan-error", US"local verification problem"); + } + if (had_local_scan_timeout) + { + log_write(0, LOG_MAIN|LOG_REJECT, "local_scan() function timed out - " + "message temporarily rejected (size %d)", message_size); + /* Does not return */ + receive_bomb_out(US"local-scan-timeout", US"local verification problem"); + } + } /* The length check is paranoia against some runaway code, and also because (for a success return) lines in the spool file are read into big_buffer. */ -if (local_scan_data != NULL) +if (local_scan_data) { int len = Ustrlen(local_scan_data); if (len > LOCAL_SCAN_MAX_RETURN) len = LOCAL_SCAN_MAX_RETURN; @@ -3706,7 +3727,7 @@ the spool file gets corrupted. Ensure that all recipients are qualified. */ if (rc == LOCAL_SCAN_ACCEPT) { - if (local_scan_data != NULL) + if (local_scan_data) { uschar *s; for (s = local_scan_data; *s != 0; s++) if (*s == '\n') *s = ' '; @@ -4331,9 +4352,9 @@ starting. */ if (blackholed_by) { - const uschar *detail = local_scan_data - ? string_printing(local_scan_data) - : string_sprintf("(%s discarded recipients)", blackholed_by); + const uschar *detail = + local_scan_data ? string_printing(local_scan_data) : + string_sprintf("(%s discarded recipients)", blackholed_by); log_write(0, LOG_MAIN, "=> blackhole %s%s", detail, blackhole_log_msg); log_write(0, LOG_MAIN, "Completed"); message_id[0] = 0; diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c index 33d6d3cc8..433c677e4 100644 --- a/src/src/smtp_in.c +++ b/src/src/smtp_in.c @@ -424,6 +424,53 @@ log_write(L_smtp_incomplete_transaction, LOG_MAIN|LOG_SENDER|LOG_RECIPIENTS, +void +smtp_command_timeout_exit(void) +{ +log_write(L_lost_incoming_connection, + LOG_MAIN, "SMTP command timeout on%s connection from %s", + tls_in.active >= 0 ? " TLS" : "", host_and_ident(FALSE)); +if (smtp_batched_input) + moan_smtp_batch(NULL, "421 SMTP command timeout"); /* Does not return */ +smtp_notquit_exit(US"command-timeout", US"421", + US"%s: SMTP command timeout - closing connection", + smtp_active_hostname); +exim_exit(EXIT_FAILURE, US"receiving"); +} + +void +smtp_command_sigterm_exit(void) +{ +log_write(0, LOG_MAIN, "%s closed after SIGTERM", smtp_get_connection_info()); +if (smtp_batched_input) + moan_smtp_batch(NULL, "421 SIGTERM received"); /* Does not return */ +smtp_notquit_exit(US"signal-exit", US"421", + US"%s: Service not available - closing connection", smtp_active_hostname); +exim_exit(EXIT_FAILURE, US"receiving"); +} + +void +smtp_data_timeout_exit(void) +{ +log_write(L_lost_incoming_connection, + LOG_MAIN, "SMTP data timeout (message abandoned) on connection from %s F=<%s>", + sender_fullhost ? sender_fullhost : US"local process", sender_address); +receive_bomb_out(US"data-timeout", US"SMTP incoming data timeout"); +/* Does not return */ +} + +void +smtp_data_sigint_exit(void) +{ +log_write(0, LOG_MAIN, "%s closed after %s", + smtp_get_connection_info(), had_data_sigint == SIGTERM ? "SIGTERM":"SIGINT"); +receive_bomb_out(US"signal-exit", + US"Service not available - SIGTERM or SIGINT received"); +/* Does not return */ +} + + + /* Refill the buffer, and notify DKIM verification code. Return false for error or EOF. */ @@ -441,18 +488,28 @@ Take care to not touch the safety NUL at the end of the buffer. */ rc = read(fileno(smtp_in), smtp_inbuffer, MIN(IN_BUFFER_SIZE-1, lim)); save_errno = errno; -alarm(0); +if (smtp_receive_timeout > 0) alarm(0); if (rc <= 0) { /* Must put the error text in fixed store, because this might be during header reading, where it releases unused store above the header. */ if (rc < 0) { + if (had_command_timeout) /* set by signal handler */ + smtp_command_timeout_exit(); /* does not return */ + if (had_command_sigterm) + smtp_command_sigterm_exit(); + if (had_data_timeout) + smtp_data_timeout_exit(); + if (had_data_sigint) + smtp_data_sigint_exit(); + smtp_had_error = save_errno; smtp_read_error = string_copy_malloc( string_sprintf(" (error: %s)", strerror(save_errno))); } - else smtp_had_eof = 1; + else + smtp_had_eof = 1; return FALSE; } #ifndef DISABLE_DKIM @@ -914,16 +971,7 @@ Returns: nothing static void command_timeout_handler(int sig) { -sig = sig; /* Keep picky compilers happy */ -log_write(L_lost_incoming_connection, - LOG_MAIN, "SMTP command timeout on%s connection from %s", - (tls_in.active >= 0)? " TLS" : "", - host_and_ident(FALSE)); -if (smtp_batched_input) - moan_smtp_batch(NULL, "421 SMTP command timeout"); /* Does not return */ -smtp_notquit_exit(US"command-timeout", US"421", - US"%s: SMTP command timeout - closing connection", smtp_active_hostname); -exim_exit(EXIT_FAILURE, US"receiving"); +had_command_timeout = sig; } @@ -941,13 +989,7 @@ Returns: nothing static void command_sigterm_handler(int sig) { -sig = sig; /* Keep picky compilers happy */ -log_write(0, LOG_MAIN, "%s closed after SIGTERM", smtp_get_connection_info()); -if (smtp_batched_input) - moan_smtp_batch(NULL, "421 SIGTERM received"); /* Does not return */ -smtp_notquit_exit(US"signal-exit", US"421", - US"%s: Service not available - closing connection", smtp_active_hostname); -exim_exit(EXIT_FAILURE, US"receiving"); +had_command_sigterm = sig; } @@ -1507,6 +1549,7 @@ int ptr = 0; smtp_cmd_list *p; BOOL hadnull = FALSE; +had_command_timeout = 0; os_non_restarting_signal(SIGALRM, command_timeout_handler); while ((c = (receive_getc)(buffer_lim)) != '\n' && c != EOF) @@ -3811,6 +3854,7 @@ cmd_list[CMD_LIST_STARTTLS].is_mail_cmd = TRUE; /* Set the local signal handler for SIGTERM - it tries to end off tidily */ +had_command_sigterm = 0; os_non_restarting_signal(SIGTERM, command_sigterm_handler); /* Batched SMTP is handled in a different function. */ diff --git a/src/src/spool_out.c b/src/src/spool_out.c index 8bebf1074..2aa603221 100644 --- a/src/src/spool_out.c +++ b/src/src/spool_out.c @@ -221,7 +221,7 @@ if (host_lookup_deferred) fprintf(f, "-host_lookup_deferred\n"); if (host_lookup_failed) fprintf(f, "-host_lookup_failed\n"); if (sender_local) fprintf(f, "-local\n"); if (local_error_message) fprintf(f, "-localerror\n"); -if (local_scan_data != NULL) fprintf(f, "-local_scan %s\n", local_scan_data); +if (local_scan_data) fprintf(f, "-local_scan %s\n", local_scan_data); #ifdef WITH_CONTENT_SCAN if (spam_bar) fprintf(f,"-spam_bar %s\n", spam_bar); if (spam_score) fprintf(f,"-spam_score %s\n", spam_score); @@ -231,7 +231,7 @@ if (deliver_manual_thaw) fprintf(f, "-manual_thaw\n"); if (sender_set_untrusted) fprintf(f, "-sender_set_untrusted\n"); #ifdef EXPERIMENTAL_BRIGHTMAIL -if (bmi_verdicts != NULL) fprintf(f, "-bmi_verdicts %s\n", bmi_verdicts); +if (bmi_verdicts) fprintf(f, "-bmi_verdicts %s\n", bmi_verdicts); #endif #ifdef SUPPORT_TLS diff --git a/src/src/tls-gnu.c b/src/src/tls-gnu.c index d73188277..35816cd60 100644 --- a/src/src/tls-gnu.c +++ b/src/src/tls-gnu.c @@ -2511,12 +2511,20 @@ sigalrm_seen = FALSE; if (smtp_receive_timeout > 0) alarm(smtp_receive_timeout); inbytes = gnutls_record_recv(state->session, state->xfer_buffer, MIN(ssl_xfer_buffer_size, lim)); -alarm(0); - -/* Timeouts do not get this far; see command_timeout_handler(). - A zero-byte return appears to mean that the TLS session has been - closed down, not that the socket itself has been closed down. Revert to - non-TLS handling. */ +if (smtp_receive_timeout > 0) alarm(0); + +if (had_command_timeout) /* set by signal handler */ + smtp_command_timeout_exit(); /* does not return */ +if (had_command_sigterm) + smtp_command_sigterm_exit(); +if (had_data_timeout) + smtp_data_timeout_exit(); +if (had_data_sigint) + smtp_data_sigint_exit(); + +/* Timeouts do not get this far. A zero-byte return appears to mean that the +TLS session has been closed down, not that the socket itself has been closed +down. Revert to non-TLS handling. */ if (sigalrm_seen) { diff --git a/src/src/tls-openssl.c b/src/src/tls-openssl.c index bfdfe211f..fb59217da 100644 --- a/src/src/tls-openssl.c +++ b/src/src/tls-openssl.c @@ -2475,7 +2475,16 @@ if (smtp_receive_timeout > 0) alarm(smtp_receive_timeout); inbytes = SSL_read(server_ssl, CS ssl_xfer_buffer, MIN(ssl_xfer_buffer_size, lim)); error = SSL_get_error(server_ssl, inbytes); -alarm(0); +if (smtp_receive_timeout > 0) alarm(0); + +if (had_command_timeout) /* set by signal handler */ + smtp_command_timeout_exit(); /* does not return */ +if (had_command_sigterm) + smtp_command_sigterm_exit(); +if (had_data_timeout) + smtp_data_timeout_exit(); +if (had_data_sigint) + smtp_data_sigint_exit(); /* SSL_ERROR_ZERO_RETURN appears to mean that the SSL session has been closed down, not that the socket itself has been closed down. Revert to -- 2.30.2