From: Jeremy Harris Date: Sat, 11 Jan 2020 18:07:10 +0000 (+0000) Subject: $local_part_verified X-Git-Url: https://git.exim.org/users/jgh/exim.git/commitdiff_plain/163144aab02a47427340d0ecc75e2abde675f4c9?hp=1ea7f48754621db22ec40b6362823433d54bda62 $local_part_verified --- diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt index 241540cfd..254ed69cc 100644 --- a/doc/doc-docbook/spec.xfpt +++ b/doc/doc-docbook/spec.xfpt @@ -6362,7 +6362,7 @@ All other options are defaulted. .code local_delivery: driver = appendfile - file = /var/mail/$home + file = /var/mail/$local_part_verified delivery_date_add envelope_to_add return_path_add @@ -6370,7 +6370,17 @@ local_delivery: # mode = 0660 .endd This &(appendfile)& transport is used for local delivery to user mailboxes in -traditional BSD mailbox format. By default it runs under the uid and gid of the +traditional BSD mailbox format. + +.new +We prefer to avoid using &$local_part$& directly to define the mailbox filename, +as it is provided by a potential bad actor. +Instead we use &$local_part_verified$&, +the result of looking up &$local_part$& in the user database +(done by using &%check_local_user%& in the the router). +.wen + +By default &(appendfile)& runs under the uid and gid of the local user, which requires the sticky bit to be set on the &_/var/mail_& directory. Some systems use the alternative approach of running mail deliveries under a particular group instead of using the sticky bit. The commented options @@ -12202,6 +12212,7 @@ the complete argument of the ETRN command (see section &<>&). .cindex "tainted data" If the origin of the data is an incoming message, the result of expanding this variable is tainted. +See also &$domain_verified$&. .wen @@ -12402,15 +12413,18 @@ once. If the origin of the data is an incoming message, the result of expanding this variable is tainted. -&*Warning*&: the content of this variable is usually provided by a potential attacker. +&*Warning*&: the content of this variable is usually provided by a potential +attacker. Consider carefully the implications of using it unvalidated as a name for file access. This presents issues for users' &_.forward_& and filter files. -For traditional full user accounts, use &%check_local_users%& and the &$home$& -variable rather than this one. +For traditional full user accounts, use &%check_local_users%& and the +&$local_part_verified$& variable rather than this one. For virtual users, store a suitable pathname component in the database which is used for account name validation, and use that retrieved value rather than this variable. +If needed, use a router &%address_data%& or &%set%& option for +the retrieved data. .wen .vindex "&$local_part_prefix$&" @@ -12479,6 +12493,14 @@ When an address is being routed or delivered, and a specific suffix for the local part was recognized, it is available in this variable, having been removed from &$local_part$&. +.new +.vitem &$local_part_verified$& +.vindex "&$local_part_verified$&" +If the router generic option &%check_local_part%& has run successfully, +this variable has the user database version of &$local_part$&. +Such values are not tainted and hence usable for building file names. +.wen + .vitem &$local_scan_data$& .vindex "&$local_scan_data$&" This variable contains the text returned by the &[local_scan()]& function when diff --git a/doc/doc-txt/NewStuff b/doc/doc-txt/NewStuff index 6b163b8b2..8a00bfc67 100644 --- a/doc/doc-txt/NewStuff +++ b/doc/doc-txt/NewStuff @@ -21,6 +21,9 @@ Version 4.94 driver for PLAIN; only against itself for SCRAM-SHA-1 and SCRAM-SHA-1-PLUS methods. + 5. Variable $local_part_verified, set by the router check_local_part condition + with untainted data. + Version 4.93 ------------ diff --git a/src/src/acl.c b/src/src/acl.c index 799af39c3..7284831a6 100644 --- a/src/src/acl.c +++ b/src/src/acl.c @@ -866,11 +866,10 @@ while ((s = (*func)()) != NULL) { uschar *endptr; - if (Ustrncmp(s, "acl_c", 5) != 0 && - Ustrncmp(s, "acl_m", 5) != 0) + if (Ustrncmp(s, "acl_c", 5) != 0 && Ustrncmp(s, "acl_m", 5) != 0) { *error = string_sprintf("invalid variable name after \"set\" in ACL " - "modifier \"set %s\" (must start \"acl_c\" or \"acl_m\")", s); + "modifier \"set %s\" (must start \"acl_c\" or \"acl_m\")", s); return NULL; } @@ -878,19 +877,19 @@ while ((s = (*func)()) != NULL) if (!isdigit(*endptr) && *endptr != '_') { *error = string_sprintf("invalid variable name after \"set\" in ACL " - "modifier \"set %s\" (digit or underscore must follow acl_c or acl_m)", - s); + "modifier \"set %s\" (digit or underscore must follow acl_c or acl_m)", + s); return NULL; } - while (*endptr != 0 && *endptr != '=' && !isspace(*endptr)) + while (*endptr && *endptr != '=' && !isspace(*endptr)) { if (!isalnum(*endptr) && *endptr != '_') - { - *error = string_sprintf("invalid character \"%c\" in variable name " - "in ACL modifier \"set %s\"", *endptr, s); - return NULL; - } + { + *error = string_sprintf("invalid character \"%c\" in variable name " + "in ACL modifier \"set %s\"", *endptr, s); + return NULL; + } endptr++; } @@ -3634,15 +3633,12 @@ for (; cb; cb = cb->next) sender_address_cache, -1, 0, CUSS &sender_data); break; - /* Connection variables must persist forever */ + /* Connection variables must persist forever; message variables not */ case ACLC_SET: { int old_pool = store_pool; - if ( cb->u.varname[0] == 'c' -#ifndef DISABLE_DKIM - || cb->u.varname[0] == 'd' -#endif + if ( cb->u.varname[0] != 'm' #ifndef DISABLE_EVENT || event_name /* An event is being delivered */ #endif diff --git a/src/src/configure.default b/src/src/configure.default index 08f5a9d10..93aa1678d 100644 --- a/src/src/configure.default +++ b/src/src/configure.default @@ -863,7 +863,7 @@ smarthost_smtp: local_delivery: driver = appendfile - file = /var/mail/$home + file = /var/mail/$local_part_verified delivery_date_add envelope_to_add return_path_add diff --git a/src/src/expand.c b/src/src/expand.c index 366cd737a..be6cd6162 100644 --- a/src/src/expand.c +++ b/src/src/expand.c @@ -587,6 +587,7 @@ static var_entry var_table[] = { { "local_part_data", vtype_stringptr, &deliver_localpart_data }, { "local_part_prefix", vtype_stringptr, &deliver_localpart_prefix }, { "local_part_suffix", vtype_stringptr, &deliver_localpart_suffix }, + { "local_part_verified", vtype_stringptr, &deliver_localpart_verified }, #ifdef HAVE_LOCAL_SCAN { "local_scan_data", vtype_stringptr, &local_scan_data }, #endif diff --git a/src/src/globals.c b/src/src/globals.c index 15ad4e932..edd1edbbc 100644 --- a/src/src/globals.c +++ b/src/src/globals.c @@ -817,6 +817,7 @@ uschar *deliver_localpart_orig = NULL; uschar *deliver_localpart_parent = NULL; uschar *deliver_localpart_prefix = NULL; uschar *deliver_localpart_suffix = NULL; +uschar *deliver_localpart_verified = NULL; uschar *deliver_out_buffer = NULL; int deliver_queue_load_max = -1; address_item *deliver_recipients = NULL; diff --git a/src/src/globals.h b/src/src/globals.h index bb66cb239..9dfa4a768 100644 --- a/src/src/globals.h +++ b/src/src/globals.h @@ -488,6 +488,7 @@ extern uschar *deliver_localpart_orig; /* The original local part for delivery * extern uschar *deliver_localpart_parent; /* The parent local part for delivery */ extern uschar *deliver_localpart_prefix; /* The stripped prefix, if any */ extern uschar *deliver_localpart_suffix; /* The stripped suffix, if any */ +extern uschar *deliver_localpart_verified; /* de-tainted by check_local_part */ extern uschar *deliver_out_buffer; /* Buffer for copying file */ extern int deliver_queue_load_max; /* Different value for queue running */ extern address_item *deliver_recipients; /* Current set of addresses */ diff --git a/src/src/route.c b/src/src/route.c index c6119eed0..16ce72c2b 100644 --- a/src/src/route.c +++ b/src/src/route.c @@ -927,7 +927,7 @@ if ((rc = route_check_dls(r->name, US"local_parts", r->local_parts, login of a local user. Note: the third argument to route_finduser() must be NULL here, to prevent a numeric string being taken as a numeric uid. If the user is found, set deliver_home to the home directory, and also set -local_user_{uid,gid}. */ +local_user_{uid,gid} and local_part_verified. */ if (r->check_local_user) { @@ -938,6 +938,7 @@ if (r->check_local_user) r->name, addr->local_part); return SKIP; } + deliver_localpart_verified = string_copy(US (*pw)->pw_name); deliver_home = string_copy(US (*pw)->pw_dir); local_user_gid = (*pw)->pw_gid; local_user_uid = (*pw)->pw_uid; @@ -1670,6 +1671,7 @@ for (r = addr->start_router ? addr->start_router : routers; r; r = nextr) the local part sorted. */ router_name = r->name; + deliver_localpart_verified = NULL; deliver_set_expansions(addr); /* For convenience, the pre-router checks are in a separate function, which diff --git a/src/src/routers/rf_get_errors_address.c b/src/src/routers/rf_get_errors_address.c index 858c806f1..9ef46801e 100644 --- a/src/src/routers/rf_get_errors_address.c +++ b/src/src/routers/rf_get_errors_address.c @@ -88,13 +88,13 @@ else const uschar *address_expansions_save[ADDRESS_EXPANSIONS_COUNT]; address_item *snew = deliver_make_addr(s, FALSE); - if (sender_address != NULL) + if (sender_address) { save1 = sender_address[0]; sender_address[0] = 0; } - for (i = 0, p = address_expansions; *p != NULL;) + for (i = 0, p = address_expansions; *p;) address_expansions_save[i++] = **p++; f.address_test_mode = FALSE; @@ -119,10 +119,10 @@ else debug_printf("------ End verifying errors address %s ------\n", s); f.address_test_mode = save_address_test_mode; - for (i = 0, p = address_expansions; *p != NULL;) + for (i = 0, p = address_expansions; *p; ) **p++ = address_expansions_save[i++]; - if (sender_address != NULL) sender_address[0] = save1; + if (sender_address) sender_address[0] = save1; } return OK; diff --git a/test/confs/0005 b/test/confs/0005 index 7c0689c16..77b79100c 100644 --- a/test/confs/0005 +++ b/test/confs/0005 @@ -52,7 +52,7 @@ local_delivery: driver = appendfile delivery_date_add envelope_to_add - file = DIR/test-mail/$local_part + file = DIR/test-mail/$local_part_verified headers_add = "X-body-linecount: $body_linecount\n\ X-message-linecount: $message_linecount\n\ X-received-count: $received_count"