From a151887254d10c7ae64030a410b39f069d02ca79 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Sat, 29 Aug 2020 20:02:19 +0100 Subject: [PATCH] taint: allow appenfile create_file option to specify a de-tainting safe path --- doc/doc-docbook/spec.xfpt | 33 +++++- src/src/transports/appendfile.c | 182 +++++++++++++----------------- test/confs/0584 | 13 ++- test/log/0584 | 7 +- test/msglog/0584.10HmaX-0005vi-00 | 2 +- test/paniclog/0584 | 2 +- test/scripts/0000-Basic/0584 | 14 ++- test/stderr/0584 | 2 +- test/stdout/0584 | 6 + 9 files changed, 149 insertions(+), 112 deletions(-) diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt index 37bfeb3f3..4a22b2e5f 100644 --- a/doc/doc-docbook/spec.xfpt +++ b/doc/doc-docbook/spec.xfpt @@ -22867,6 +22867,11 @@ If &%file%& or &%directory%& is set for a delivery from a redirection, it is used to determine the file or directory name for the delivery. Normally, the contents of &$address_file$& are used in some way in the string expansion. .endlist +If the &%create_file%& option is set to a path which +matches (see the option definition below for details) +a file or directory name +for the delivery, that name becomes de-tainted. + .cindex "tainted data" "in filenames" .cindex appendfile "tainted data" Tainted data may not be used for a file or directory name. @@ -23014,14 +23019,30 @@ directories defined by the &%directory%& option. In the case of maildir delivery, it applies to the top level directory, not the maildir directories beneath. +.new The option must be set to one of the words &"anywhere"&, &"inhome"&, or -&"belowhome"&. In the second and third cases, a home directory must have been +&"belowhome"&, or to an absolute path. +.wen + +In the second and third cases, a home directory must have been set for the transport. This option is not useful when an explicit filename is given for normal mailbox deliveries. It is intended for the case when filenames are generated from users' &_.forward_& files. These are usually handled by an &(appendfile)& transport called &%address_file%&. See also &%file_must_exist%&. +.new +In the fourth case, +the value given for this option must be an absolute path for an +existing directory. +The expansion of either the &%directory%& or &%file%& +option is checked for being a strict (possibly potential) descendant, +in the filesystem, of the value given. +.cindex "tainted data" "de-tainting" +If the check passes then the path checked becomes de-tainted. +If the check fails then the transport returns failure. +.wen + .option directory appendfile string&!! unset This option is mutually exclusive with the &%file%& option, but one of &%file%& @@ -23034,6 +23055,11 @@ appended to a single mailbox file. A number of different formats are provided (see &%maildir_format%& and &%mailstore_format%&), and see section &<>& for further details of this form of delivery. +.new +The result of expansion must not be tainted, unless the &%create_file%& option +specifies a path. +.wen + .option directory_file appendfile string&!! "see below" .cindex "base62" @@ -23066,6 +23092,11 @@ specifies a single file, to which the message is appended. One or more of &%use_fcntl_lock%&, &%use_flock_lock%&, or &%use_lockfile%& must be set with &%file%&. +.new +The result of expansion must not be tainted, unless the &%create_file%& option +specifies a path. +.wen + .cindex "NFS" "lock file" .cindex "locking files" .cindex "lock files" diff --git a/src/src/transports/appendfile.c b/src/src/transports/appendfile.c index 8ef1a564e..908fd8ad9 100644 --- a/src/src/transports/appendfile.c +++ b/src/src/transports/appendfile.c @@ -2,7 +2,7 @@ * Exim - an Internet mail transport agent * *************************************************/ -/* Copyright (c) University of Cambridge 1995 - 2018 */ +/* Copyright (c) University of Cambridge 1995 - 2020 */ /* Copyright (c) The Exim maintainers 2020 */ /* See the file NOTICE for conditions of use and distribution. */ @@ -112,70 +112,27 @@ BOOL appendfile_transport_entry(transport_instance *tblock, address_item *addr) /* Default private options block for the appendfile transport. */ appendfile_transport_options_block appendfile_transport_option_defaults = { - NULL, /* filename */ - NULL, /* dirname */ - US"q${base62:$tod_epoch}-$inode", /* dirfilename */ - NULL, /* message_prefix (default reset in init if not bsmtp) */ - NULL, /* message_suffix (ditto) */ - US"anywhere", /* create_file_string (string value for create_file) */ - NULL, /* quota */ - NULL, /* quota_directory */ - NULL, /* quota_filecount */ - NULL, /* quota_size_regex */ - NULL, /* quota_warn_threshold */ - NULL, /* mailbox_size_string */ - NULL, /* mailbox_filecount_string */ - NULL, /* expand_maildir_use_size_file */ - US"^(?:cur|new|\\..*)$", /* maildir_dir_regex */ - NULL, /* maildir_tag */ - NULL, /* maildirfolder_create_regex */ - NULL, /* mailstore_prefix */ - NULL, /* mailstore_suffix */ - NULL, /* check_string (default changed for non-bsmtp file)*/ - NULL, /* escape_string (ditto) */ - NULL, /* file_format */ - 0, /* quota_value */ - 0, /* quota_warn_threshold_value */ - -1, /* mailbox_size_value */ - -1, /* mailbox_filecount_value */ - 0, /* quota_filecount_value */ - APPENDFILE_MODE, /* mode */ - APPENDFILE_DIRECTORY_MODE, /* dirmode */ - APPENDFILE_LOCKFILE_MODE, /* lockfile_mode */ - 30*60, /* lockfile_timeout */ - 0, /* lock_fcntl_timeout */ - 0, /* lock_flock_timeout */ - 10, /* lock_retries */ - 3, /* lock_interval */ - 10, /* maildir_retries */ - create_anywhere,/* create_file */ - 0, /* options */ - FALSE, /* allow_fifo */ - FALSE, /* allow_symlink */ - FALSE, /* check_group */ - TRUE, /* check_owner */ - TRUE, /* create_directory */ - FALSE, /* notify_comsat */ - TRUE, /* use_lockfile */ - FALSE, /* set_use_lockfile */ - TRUE, /* use_fcntl */ - FALSE, /* set_use_fcntl */ - FALSE, /* use_flock */ - FALSE, /* set_use_flock */ - FALSE, /* use_mbx_lock */ - FALSE, /* set_use_mbx_lock */ - FALSE, /* use_bsmtp */ - FALSE, /* use_crlf */ - FALSE, /* file_must_exist */ - TRUE, /* mode_fail_narrower */ - FALSE, /* maildir_format */ - FALSE, /* maildir_use_size_file */ - FALSE, /* mailstore_format */ - FALSE, /* mbx_format */ - FALSE, /* quota_warn_threshold_is_percent */ - TRUE, /* quota_is_inclusive */ - FALSE, /* quota_no_check */ - FALSE /* quota_filecount_no_check */ + /* all non-mentioned members zero/null/false */ + .dirfilename = US"q${base62:$tod_epoch}-$inode", + .create_file_string = US"anywhere", + .maildir_dir_regex = US"^(?:cur|new|\\..*)$", + .mailbox_size_value = -1, + .mailbox_filecount_value = -1, + .mode = APPENDFILE_MODE, + .dirmode = APPENDFILE_DIRECTORY_MODE, + .lockfile_mode = APPENDFILE_LOCKFILE_MODE, + .lockfile_timeout = 30*60, + .lock_retries = 10, + .lock_interval = 3, + .maildir_retries = 10, + .create_file = create_anywhere, + .check_owner = TRUE, + .create_directory = TRUE, + .notify_comsat = FALSE, + .use_lockfile = TRUE, + .use_fcntl = TRUE, + .mode_fail_narrower = TRUE, + .quota_is_inclusive = TRUE, }; @@ -235,17 +192,15 @@ mailbox_filecount */ for (int i = 0; i < 5; i++) { - double d; + double d = default_value; int no_check = 0; uschar *which = NULL; - if (q == NULL) d = default_value; - else + if (q) { - uschar *rest; - uschar *s = expand_string(q); + uschar * rest, * s; - if (!s) + if (!(s = expand_string(q))) { *errmsg = string_sprintf("Expansion of \"%s\" in %s transport failed: " "%s", q, tblock->name, expand_string_message); @@ -315,8 +270,8 @@ for (int i = 0; i < 5; i++) break; case 2: - if (d >= 2.0*1024.0*1024.0*1024.0 && sizeof(off_t) <= 4) - which = US"quota_warn_threshold"; + if (d >= 2.0*1024.0*1024.0*1024.0 && sizeof(off_t) <= 4) + which = US"quota_warn_threshold"; ob->quota_warn_threshold_value = (off_t)d; q = ob->mailbox_size_string; default_value = -1.0; @@ -362,6 +317,7 @@ appendfile_transport_init(transport_instance *tblock) { appendfile_transport_options_block *ob = (appendfile_transport_options_block *)(tblock->options_block); +uschar * s; /* Set up the setup entry point, to be called in the privileged state */ @@ -460,20 +416,17 @@ if (tblock->uid_set && !tblock->gid_set && !tblock->expand_gid) /* If "create_file" is set, check that a valid option is given, and set the integer variable. */ -if (ob->create_file_string) +if ((s = ob->create_file_string ) && *s) { - int value = 0; - if (Ustrcmp(ob->create_file_string, "anywhere") == 0) - value = create_anywhere; - else if (Ustrcmp(ob->create_file_string, "belowhome") == 0) - value = create_belowhome; - else if (Ustrcmp(ob->create_file_string, "inhome") == 0) - value = create_inhome; + int val = 0; + if (Ustrcmp(s, "anywhere") == 0) val = create_anywhere; + else if (*s == '/' || Ustrcmp(s, "belowhome") == 0) val = create_belowhome; + else if (Ustrcmp(s, "inhome") == 0) val = create_inhome; else log_write(0, LOG_PANIC_DIE|LOG_CONFIG, - "invalid value given for \"file_create\" for the %s transport: %s", - tblock->name, ob->create_file_string); - ob->create_file = value; + "invalid value given for \"file_create\" for the %s transport: '%s'", + tblock->name, s); + ob->create_file = val; } /* If quota_warn_threshold is set, set up default for warn_message. It may @@ -950,13 +903,12 @@ if (deliver_home && create_file != create_anywhere) uschar *file = filename; while (file[0] == '/' && file[1] == '/') file++; - if (Ustrncmp(file, deliver_home, len) != 0 || file[len] != '/' || - ( Ustrchr(file+len+2, '/') != NULL && - ( - create_file != create_belowhome || - Ustrstr(file+len, "/../") != NULL - ) - ) + if ( Ustrncmp(file, deliver_home, len) != 0 + || file[len] != '/' + || Ustrchr(file+len+2, '/') != NULL + && ( create_file != create_belowhome + || Ustrstr(file+len, "/../") != NULL + ) ) yield = FALSE; /* If yield is TRUE, the file name starts with the home directory, and does @@ -1281,12 +1233,6 @@ if (!(path = expand_string(fdname))) expand_string_message); goto ret_panic; } -if (is_tainted(path)) - { - addr->message = string_sprintf("Tainted '%s' (file or directory " - "name for %s transport) not permitted", path, tblock->name); - goto ret_panic; - } if (path[0] != '/') { @@ -1363,6 +1309,12 @@ if (f.dont_deliver) return FALSE; } +/* If an absolute path was given for create_file the it overrides deliver_home +(here) and de-taints the filename (below, after check_creation() */ + +if (*ob->create_file_string == '/') + deliver_home = ob->create_file_string; + /* Handle the case of a file name. If the file name is /dev/null, we can save ourselves some effort and just give a success return right away. */ @@ -1379,10 +1331,21 @@ if (!isdirectory) } /* Set the name of the file to be opened, and the file to which the data - is written, and find out if we are permitted to create a non-existent file. */ + is written, and find out if we are permitted to create a non-existent file. + If the create_file option is an absolute path and the file was within it, + de-taint. Chaeck for a tainted path. */ +/*XXX could we just de-taint on belowhome? */ + + if ( (allow_creation_here = check_creation(path, ob->create_file)) + && *ob->create_file_string == '/') + if (is_tainted(path)) + { + DEBUG(D_transport) debug_printf("de-tainting path '%s'\n", path); + path = string_copy_taint(path, FALSE); + } + if (is_tainted(path)) goto tainted_ret_panic; dataname = filename = path; - allow_creation_here = check_creation(filename, ob->create_file); /* If ob->create_directory is set, attempt to create the directories in which this mailbox lives, but only if we are permitted to create the file @@ -1398,7 +1361,7 @@ if (!isdirectory) addr->basic_errno = errno; addr->message = string_sprintf("failed to create directories for %s: %s", path, - strerror(errno)); + exim_errstr(errno)); DEBUG(D_transport) debug_printf("%s transport: %s\n", tblock->name, path); return FALSE; } @@ -2199,7 +2162,7 @@ scanning is expensive; for maildirs some fudges have been invented: else { - uschar *check_path = path; /* Default quota check path */ + uschar *check_path; /* Default quota check path */ const pcre *regex = NULL; /* Regex for file size from file name */ if (!check_creation(string_sprintf("%s/any", path), ob->create_file)) @@ -2210,6 +2173,20 @@ else goto RETURN; } + /* If the create_file option is an absolute path and the file was within + it, de-taint. Otherwise check for taint. */ + + if (is_tainted(path)) + if (*ob->create_file_string = '/') + { + DEBUG(D_transport) debug_printf("de-tainting path '%s'\n", path); + path = string_copy_taint(path, FALSE); + } + else + goto tainted_ret_panic; + + check_path = path; + #ifdef SUPPORT_MAILDIR /* For a maildir delivery, ensure that all the relevant directories exist, and a maildirfolder file if necessary. */ @@ -3318,6 +3295,9 @@ put in the first address of a batch. */ return FALSE; +tainted_ret_panic: + addr->message = string_sprintf("Tainted '%s' (file or directory " + "name for %s transport) not permitted", path, tblock->name); ret_panic: addr->transport_return = PANIC; return FALSE; diff --git a/test/confs/0584 b/test/confs/0584 index 654ecbfb9..cca109f00 100644 --- a/test/confs/0584 +++ b/test/confs/0584 @@ -1,5 +1,7 @@ # Exim test configuration 0005 +OPT= + .include DIR/aux-var/std_conf_prefix @@ -25,16 +27,17 @@ check_recipient: begin routers localuser: - driver = accept - check_local_user - transport = local_delivery + driver = accept + transport = local_delivery + errors_to = # ----- Transports ----- begin transports local_delivery: - driver = appendfile - file = DIR/test-mail/$local_part + driver = appendfile + file = DIR/test-mail/$local_part + create_file = OPT # End diff --git a/test/log/0584 b/test/log/0584 index f3ab6ea12..de70c2806 100644 --- a/test/log/0584 +++ b/test/log/0584 @@ -1,2 +1,7 @@ 1999-03-02 09:44:33 10HmaX-0005vi-00 <= someone@some.domain U=CALLER P=local-smtp S=sss -1999-03-02 09:44:33 10HmaX-0005vi-00 == CALLER@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/CALLER' (file or directory name for local_delivery transport) not permitted +1999-03-02 09:44:33 10HmaX-0005vi-00 == fred@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/fred' (file or directory name for local_delivery transport) not permitted +1999-03-02 09:44:33 10HmaY-0005vi-00 <= someone@some.domain U=CALLER P=local-smtp S=sss +1999-03-02 09:44:33 10HmaY-0005vi-00 == bill@the.local.host.name R=localuser T=local_delivery defer (EEE): Permission denied: creating lock file hitching post TESTSUITE/test-mail/bill.lock.test.ex.dddddddd.pppppppp (euid=EXIM_UID egid=EXIM_GID) +1999-03-02 09:44:33 10HmaY-0005vi-00 ** bill@the.local.host.name: retry timeout exceeded +1999-03-02 09:44:33 10HmaY-0005vi-00 bill@the.local.host.name: error ignored +1999-03-02 09:44:33 10HmaY-0005vi-00 Completed diff --git a/test/msglog/0584.10HmaX-0005vi-00 b/test/msglog/0584.10HmaX-0005vi-00 index 540b68351..fa93f2509 100644 --- a/test/msglog/0584.10HmaX-0005vi-00 +++ b/test/msglog/0584.10HmaX-0005vi-00 @@ -1,2 +1,2 @@ 1999-03-02 09:44:33 Received from someone@some.domain U=CALLER P=local-smtp S=sss -1999-03-02 09:44:33 CALLER@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/CALLER' (file or directory name for local_delivery transport) not permitted +1999-03-02 09:44:33 fred@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/fred' (file or directory name for local_delivery transport) not permitted diff --git a/test/paniclog/0584 b/test/paniclog/0584 index af4f47118..c91120311 100644 --- a/test/paniclog/0584 +++ b/test/paniclog/0584 @@ -1 +1 @@ -1999-03-02 09:44:33 10HmaX-0005vi-00 == CALLER@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/CALLER' (file or directory name for local_delivery transport) not permitted +1999-03-02 09:44:33 10HmaX-0005vi-00 == fred@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/fred' (file or directory name for local_delivery transport) not permitted diff --git a/test/scripts/0000-Basic/0584 b/test/scripts/0000-Basic/0584 index 54ca506cc..ebcd44ebe 100644 --- a/test/scripts/0000-Basic/0584 +++ b/test/scripts/0000-Basic/0584 @@ -1,7 +1,19 @@ # tainted data for appendfile file option +# +# This should trap exim -bs -odi mail from:someone@some.domain -rcpt to:CALLER@HOSTNAME +rcpt to:fred@HOSTNAME +data +. +quit +**** +# +# taint trap defeated by using create_file +# goes on to fail on perms +exim -bs -odi -DOPT=DIR/test-mail +mail from:someone@some.domain +rcpt to:bill@HOSTNAME data . quit diff --git a/test/stderr/0584 b/test/stderr/0584 index af4f47118..c91120311 100644 --- a/test/stderr/0584 +++ b/test/stderr/0584 @@ -1 +1 @@ -1999-03-02 09:44:33 10HmaX-0005vi-00 == CALLER@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/CALLER' (file or directory name for local_delivery transport) not permitted +1999-03-02 09:44:33 10HmaX-0005vi-00 == fred@the.local.host.name R=localuser T=local_delivery defer (-1): Tainted 'TESTSUITE/test-mail/fred' (file or directory name for local_delivery transport) not permitted diff --git a/test/stdout/0584 b/test/stdout/0584 index 4e4673def..2b1941f58 100644 --- a/test/stdout/0584 +++ b/test/stdout/0584 @@ -4,3 +4,9 @@ 354 Enter message, ending with "." on a line by itself 250 OK id=10HmaX-0005vi-00 221 the.local.host.name closing connection +220 the.local.host.name ESMTP Exim x.yz Tue, 2 Mar 1999 09:44:33 +0000 +250 OK +250 Accepted +354 Enter message, ending with "." on a line by itself +250 OK id=10HmaY-0005vi-00 +221 the.local.host.name closing connection -- 2.30.2