From 7d99cba1d36af854760c35100b29f0331f619fca Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Sat, 11 Jan 2020 21:49:10 +0000 Subject: [PATCH] redirect router: taint-enforce filenames --- doc/doc-docbook/spec.xfpt | 9 +++++++ doc/doc-txt/ChangeLog | 6 +++-- src/src/parse.c | 17 +++++++++---- src/src/rda.c | 48 +++++++++++++++++++++--------------- test/confs/0153 | 2 +- test/confs/0251 | 2 +- test/confs/0306 | 2 +- test/confs/0307 | 2 +- test/confs/0452 | 2 +- test/confs/0586 | 17 +++++++++++++ test/scripts/0000-Basic/0586 | 4 +++ test/stdout/0586 | 2 ++ 12 files changed, 81 insertions(+), 32 deletions(-) create mode 100644 test/confs/0586 create mode 100644 test/scripts/0000-Basic/0586 create mode 100644 test/stdout/0586 diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt index 0e44b119b..1d6fa536b 100644 --- a/doc/doc-docbook/spec.xfpt +++ b/doc/doc-docbook/spec.xfpt @@ -20579,6 +20579,10 @@ yield empty addresses, for example, items containing only RFC 2822 address comments. .new +.cindex "tainted data" "in filenames" +.cindex redirect "tainted data" +Tainted data may not be used for a filename. + &*Warning*&: It is unwise to use &$local_part$& or &$domain$& directly for redirection, as they are provided by a potential attacker. @@ -20812,6 +20816,11 @@ It must be given as .code list1: :include:/opt/lists/list1 .endd +.new +.cindex "tainted data" "in filenames" +.cindex redirect "tainted data" +Tainted data may not be used for a filename. +.wen .next .cindex "address redirection" "to black hole" .cindex "delivery" "discard" diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index c803fdb7e..33381d558 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -87,8 +87,10 @@ JH/19 Bug 2507: Modules: on handling a dynamic-module (lookups) open failure, were used, and the second one (for mainlog/paniclog) retrieved null information. -JH/20 Taint checking: disallow use of tainted data for the appendfile transport - file and directory options, and for the pipe transport command. +JH/20 Taint checking: disallow use of tainted data for + - the appendfile transport file and directory options + - the pipe transport command + - file names used by the redirect router (including filter files) Previously this was permitted. diff --git a/src/src/parse.c b/src/src/parse.c index e64cb94a1..be70effe9 100644 --- a/src/src/parse.c +++ b/src/src/parse.c @@ -1277,10 +1277,10 @@ for (;;) However, if the list is empty only because syntax errors were skipped, we return FF_DELIVERED. */ - if (*s == 0) + if (!*s) { - return (count > 0 || (syntax_errors != NULL && *syntax_errors != NULL))? - FF_DELIVERED : FF_NOTDELIVERED; + return (count > 0 || (syntax_errors && *syntax_errors)) + ? FF_DELIVERED : FF_NOTDELIVERED; /* This previous code returns FF_ERROR if nothing is generated but a syntax error has been skipped. I now think it is the wrong approach, but @@ -1411,7 +1411,7 @@ for (;;) /* Insist on absolute path */ - if (filename[0]!= '/') + if (filename[0] != '/') { *error = string_sprintf("included file \"%s\" is not an absolute path", filename); @@ -1420,12 +1420,19 @@ for (;;) /* Check if include is permitted */ - if ((options & RDO_INCLUDE) != 0) + if (options & RDO_INCLUDE) { *error = US"included files not permitted"; return FF_ERROR; } + if (is_tainted(filename)) + { + *error = string_sprintf("Tainted name '%s' for included file not permitted\n", + filename); + return FF_ERROR; + } + /* Check file name if required */ if (directory) diff --git a/src/src/rda.c b/src/src/rda.c index 201e82d8b..574b86cdd 100644 --- a/src/src/rda.c +++ b/src/src/rda.c @@ -3,6 +3,7 @@ *************************************************/ /* Copyright (c) University of Cambridge 1995 - 2018 */ +/* Copyright (c) The Exim maintainers 2020 */ /* See the file NOTICE for conditions of use and distribution. */ /* This module contains code for extracting addresses from a forwarding list @@ -175,6 +176,17 @@ BOOL uid_ok = !rdata->check_owner; BOOL gid_ok = !rdata->check_group; struct stat statbuf; +/* Reading a file is a form of expansion; we wish to deny attackers the +capability to specify the file name. */ + +if (is_tainted(filename)) + { + *error = string_sprintf("Tainted name '%s' for file read not permitted\n", + filename); + *yield = FF_ERROR; + return NULL; + } + /* Attempt to open the file. If it appears not to exist, check up on the containing directory by statting it. If the directory does not exist, we treat this situation as an error (which will cause delivery to defer); otherwise we @@ -195,20 +207,20 @@ if (!(fwd = Ufopen(filename, "rb"))) switch(errno) return NULL; case ENOTDIR: /* Something on the path isn't a directory */ - if ((options & RDO_ENOTDIR) == 0) goto DEFAULT_ERROR; + if (!(options & RDO_ENOTDIR)) goto DEFAULT_ERROR; DEBUG(D_route) debug_printf("non-directory on path %s: file assumed not to " "exist\n", filename); *yield = FF_NONEXIST; return NULL; case EACCES: /* Permission denied */ - if ((options & RDO_EACCES) == 0) goto DEFAULT_ERROR; + if (!(options & RDO_EACCES)) goto DEFAULT_ERROR; DEBUG(D_route) debug_printf("permission denied for %s: file assumed not to " "exist\n", filename); *yield = FF_NONEXIST; return NULL; - DEFAULT_ERROR: +DEFAULT_ERROR: default: *error = string_open_failed(errno, "%s", filename); *yield = FF_ERROR; @@ -361,7 +373,7 @@ if (*filtertype != FILTER_FORWARD) /* RDO_FILTER is an "allow" bit */ - if ((options & RDO_FILTER) == 0) + if (!(options & RDO_FILTER)) { *error = US"filtering not enabled"; return FF_ERROR; @@ -383,7 +395,7 @@ if (*filtertype != FILTER_FORWARD) } else { - if ((options & RDO_SIEVE_FILTER) != 0) + if (options & RDO_SIEVE_FILTER) { *error = US"Sieve filtering not enabled"; return FF_ERROR; @@ -575,11 +587,9 @@ if (!ugid->uid_set || /* Either there's no uid, or */ (!rdata->isfile && /* We've got the data, and */ rda_is_filter(data) == FILTER_FORWARD && /* It's not a filter script, */ Ustrstr(data, ":include:") == NULL)) /* and there's no :include: */ - { return rda_extract(rdata, options, include_directory, sieve_vacation_directory, sieve_enotify_mailto_owner, sieve_useraddress, sieve_subaddress, generated, error, eblockp, filtertype); - } /* We need to run the processing code in a sub-process. However, if we can determine the non-existence of a file first, we can decline without having to @@ -911,17 +921,17 @@ if (yield == FF_DELIVERED || yield == FF_NOTDELIVERED || sizeof(int) || read(fd,&(addr->reply->once_repeat),sizeof(time_t)) != sizeof(time_t) || - !rda_read_string(fd, &(addr->reply->to)) || - !rda_read_string(fd, &(addr->reply->cc)) || - !rda_read_string(fd, &(addr->reply->bcc)) || - !rda_read_string(fd, &(addr->reply->from)) || - !rda_read_string(fd, &(addr->reply->reply_to)) || - !rda_read_string(fd, &(addr->reply->subject)) || - !rda_read_string(fd, &(addr->reply->headers)) || - !rda_read_string(fd, &(addr->reply->text)) || - !rda_read_string(fd, &(addr->reply->file)) || - !rda_read_string(fd, &(addr->reply->logfile)) || - !rda_read_string(fd, &(addr->reply->oncelog))) + !rda_read_string(fd, &addr->reply->to) || + !rda_read_string(fd, &addr->reply->cc) || + !rda_read_string(fd, &addr->reply->bcc) || + !rda_read_string(fd, &addr->reply->from) || + !rda_read_string(fd, &addr->reply->reply_to) || + !rda_read_string(fd, &addr->reply->subject) || + !rda_read_string(fd, &addr->reply->headers) || + !rda_read_string(fd, &addr->reply->text) || + !rda_read_string(fd, &addr->reply->file) || + !rda_read_string(fd, &addr->reply->logfile) || + !rda_read_string(fd, &addr->reply->oncelog)) goto DISASTER; } } @@ -932,13 +942,11 @@ reading end of the pipe, and we are done. */ WAIT_EXIT: while ((rc = wait(&status)) != pid) - { if (rc < 0 && errno == ECHILD) /* Process has vanished */ { log_write(0, LOG_MAIN, "redirection process %d vanished unexpectedly", pid); goto FINAL_EXIT; } - } DEBUG(D_route) debug_printf("rda_interpret: subprocess yield=%d error=%s\n", yield, *error); diff --git a/test/confs/0153 b/test/confs/0153 index 69e02ebcf..d70a38e7b 100644 --- a/test/confs/0153 +++ b/test/confs/0153 @@ -23,7 +23,7 @@ list: driver = redirect domains = list.test.ex file = ${if exists{DIR/aux-fixed/TESTNUM.list.${bless:$local_part}} \ - {DIR/aux-fixed/TESTNUM.list.$local_part}fail} + {DIR/aux-fixed/TESTNUM.list.${bless:$local_part}}fail} no_more real: diff --git a/test/confs/0251 b/test/confs/0251 index 180620f11..ea6b78f5e 100644 --- a/test/confs/0251 +++ b/test/confs/0251 @@ -32,7 +32,7 @@ exeter_listr: no_check_local_user domains = listr.test.ex errors_to = ${local_part}-request@test.ex - file = DIR/aux-fixed/TESTNUM.list.${local_part} + file = DIR/aux-fixed/TESTNUM.list.${bless:$local_part} forbid_file forbid_pipe one_time diff --git a/test/confs/0306 b/test/confs/0306 index c8bd1f362..779e155fc 100644 --- a/test/confs/0306 +++ b/test/confs/0306 @@ -27,7 +27,7 @@ r1: driver = redirect domains = lists.test.ex local_part_suffix = -request - file = DIR/aux-fixed/TESTNUM/${bless:$local_part}$local_part_suffix + file = DIR/aux-fixed/TESTNUM/${bless:$local_part$local_part_suffix} r2: driver = redirect diff --git a/test/confs/0307 b/test/confs/0307 index c2019893a..1f61ca3cb 100644 --- a/test/confs/0307 +++ b/test/confs/0307 @@ -24,7 +24,7 @@ r1: ${if exists {DIR/aux-fixed/TESTNUM/$local_part}\ {lsearch;DIR/aux-fixed/TESTNUM/$local_part}{*}}\ }} - file = DIR/aux-fixed/TESTNUM/${bless:$local_part}$local_part_suffix + file = DIR/aux-fixed/TESTNUM/${bless:$local_part$local_part_suffix} forbid_pipe forbid_file one_time diff --git a/test/confs/0452 b/test/confs/0452 index 3608eeed2..7ae6a2ad0 100644 --- a/test/confs/0452 +++ b/test/confs/0452 @@ -17,7 +17,7 @@ begin routers r1: driver = redirect allow_filter - file = DIR/aux-fixed/TESTNUM.filter-$h_fno: + file = DIR/aux-fixed/TESTNUM.${bless:filter-$h_fno:} reply_transport = t2 user = CALLER diff --git a/test/confs/0586 b/test/confs/0586 new file mode 100644 index 000000000..1b2d83571 --- /dev/null +++ b/test/confs/0586 @@ -0,0 +1,17 @@ +# Exim test configuration 0586 + +.include DIR/aux-var/std_conf_prefix + + +# ----- Main settings ----- + + +# ----- Routers ----- + +begin routers + +list: + driver = redirect + file = DIR/aux-fixed/TESTNUM.list.$local_part + +# End diff --git a/test/scripts/0000-Basic/0586 b/test/scripts/0000-Basic/0586 new file mode 100644 index 000000000..0e48328ce --- /dev/null +++ b/test/scripts/0000-Basic/0586 @@ -0,0 +1,4 @@ +# tainted data for filter filename +1 +exim -bv abcd@test.ex +**** diff --git a/test/stdout/0586 b/test/stdout/0586 new file mode 100644 index 000000000..0fe061759 --- /dev/null +++ b/test/stdout/0586 @@ -0,0 +1,2 @@ +abcd@test.ex cannot be resolved at this time: Tainted name 'TESTSUITE/aux-fixed/0586.list.abcd' for file read not permitted + -- 2.25.1