From 36771878fa93a04ecf5bdd71ad3c3c380a16aa03 Mon Sep 17 00:00:00 2001 From: Phil Pennock Date: Thu, 29 Oct 2020 23:21:36 -0400 Subject: [PATCH] SECURITY: rework BDAT receive function handling (cherry picked from commit dd1b9b753bb7c42df2b8f48d726b82928b67940b) (cherry picked from commit 96fb195ebc2eb6790e6ad6dde46d478aee62198d) --- doc/doc-txt/ChangeLog | 6 ++++ src/src/globals.c | 1 + src/src/smtp_in.c | 67 +++++++++++++++++++++++++++++++------------ 3 files changed, 55 insertions(+), 19 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 9837d6c0f..0e008c985 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -288,6 +288,12 @@ PP/09 Fix security issue with too many recipients on a message (to remove a PP/10 Fix security issue in SMTP verb option parsing Fixes CVE-2020-EXOPT reported by Qualys. +PP/11 Fix security issue in BDAT state confusion. + Ensure we reset known-good where we know we need to not be reading BDAT + data, as a general case fix, and move the places where we switch to BDAT + mode until after various protocol state checks. + Fixes CVE-2020-BDATA reported by Qualys. + Exim version 4.94 ----------------- diff --git a/src/src/globals.c b/src/src/globals.c index bd874a789..e1837b67d 100644 --- a/src/src/globals.c +++ b/src/src/globals.c @@ -227,6 +227,7 @@ struct global_flags f = .authentication_local = FALSE, .background_daemon = TRUE, + .bdat_readers_wanted = FALSE, .chunking_offered = FALSE, .config_changed = FALSE, diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c index 4f16fd4b8..eb032bb52 100644 --- a/src/src/smtp_in.c +++ b/src/src/smtp_in.c @@ -624,9 +624,7 @@ for(;;) if (chunking_data_left > 0) return lwr_receive_getc(chunking_data_left--); - receive_getc = lwr_receive_getc; - receive_getbuf = lwr_receive_getbuf; - receive_ungetc = lwr_receive_ungetc; + bdat_pop_receive_functions(); #ifndef DISABLE_DKIM dkim_save = dkim_collect_input; dkim_collect_input = 0; @@ -730,9 +728,7 @@ next_cmd: goto repeat_until_rset; } - receive_getc = bdat_getc; - receive_getbuf = bdat_getbuf; /* r~getbuf is never actually used */ - receive_ungetc = bdat_ungetc; + bdat_push_receive_functions(); #ifndef DISABLE_DKIM dkim_collect_input = dkim_save; #endif @@ -765,9 +761,7 @@ while (chunking_data_left) if (!bdat_getbuf(&n)) break; } -receive_getc = lwr_receive_getc; -receive_getbuf = lwr_receive_getbuf; -receive_ungetc = lwr_receive_ungetc; +bdat_pop_receive_functions(); if (chunking_state != CHUNKING_LAST) { @@ -777,7 +771,35 @@ if (chunking_state != CHUNKING_LAST) } +void +bdat_push_receive_functions(void) +{ +/* push the current receive_* function on the "stack", and +replace them by bdat_getc(), which in turn will use the lwr_receive_* +functions to do the dirty work. */ +if (lwr_receive_getc == NULL) + { + lwr_receive_getc = receive_getc; + lwr_receive_getbuf = receive_getbuf; + lwr_receive_ungetc = receive_ungetc; + } +else + { + DEBUG(D_receive) debug_printf("chunking double-push receive functions\n"); + } +receive_getc = bdat_getc; +receive_ungetc = bdat_ungetc; +} + +void +bdat_pop_receive_functions(void) +{ +receive_getc = lwr_receive_getc; +receive_getbuf = lwr_receive_getbuf; +receive_ungetc = lwr_receive_ungetc; +lwr_receive_getc = lwr_receive_getbuf = lwr_receive_ungetc = NULL; +} /************************************************* * SMTP version of ungetc() * @@ -2528,6 +2550,7 @@ receive_ungetc = smtp_ungetc; receive_feof = smtp_feof; receive_ferror = smtp_ferror; receive_smtp_buffered = smtp_buffered; +lwr_receive_getc = lwr_receive_getbuf = lwr_receive_ungetc = NULL; smtp_inptr = smtp_inend = smtp_inbuffer; smtp_had_eof = smtp_had_error = 0; @@ -3954,6 +3977,14 @@ cmd_list[CMD_LIST_EHLO].is_mail_cmd = TRUE; cmd_list[CMD_LIST_STARTTLS].is_mail_cmd = TRUE; #endif +if (lwr_receive_getc != NULL) + { + /* This should have already happened, but if we've gotten confused, + force a reset here. */ + DEBUG(D_receive) debug_printf("WARNING: smtp_setup_msg had to restore receive functions to lowers\n"); + bdat_pop_receive_functions(); + } + /* Set the local signal handler for SIGTERM - it tries to end off tidily */ had_command_sigterm = 0; @@ -5288,16 +5319,7 @@ while (done <= 0) DEBUG(D_receive) debug_printf("chunking state %d, %d bytes\n", (int)chunking_state, chunking_data_left); - /* push the current receive_* function on the "stack", and - replace them by bdat_getc(), which in turn will use the lwr_receive_* - functions to do the dirty work. */ - lwr_receive_getc = receive_getc; - lwr_receive_getbuf = receive_getbuf; - lwr_receive_ungetc = receive_ungetc; - - receive_getc = bdat_getc; - receive_ungetc = bdat_ungetc; - + f.bdat_readers_wanted = TRUE; f.dot_ends = FALSE; goto DATA_BDAT; @@ -5306,6 +5328,7 @@ while (done <= 0) case DATA_CMD: HAD(SCH_DATA); f.dot_ends = TRUE; + f.bdat_readers_wanted = FALSE DATA_BDAT: /* Common code for DATA and BDAT */ #ifndef DISABLE_PIPE_CONNECT @@ -5334,7 +5357,10 @@ while (done <= 0) : US"valid RCPT command must precede BDAT"); if (chunking_state > CHUNKING_OFFERED) + { + bdat_push_receive_functions(); bdat_flush_data(); + } break; } @@ -5373,6 +5399,9 @@ while (done <= 0) } } + if (f.bdat_readers_wanted) + bdat_push_receive_functions(); + if (user_msg) smtp_user_msg(US"354", user_msg); else -- 2.30.2