Gitweb:
https://git.exim.org/exim.git/commitdiff/7d99cba1d36af854760c35100b29f0331f619fca
Commit: 7d99cba1d36af854760c35100b29f0331f619fca
Parent: 9214d2e4dfd9d4f29e9cb7a0eea8a0758ed1b34a
Author: Jeremy Harris <jgh146exb@???>
AuthorDate: Sat Jan 11 21:49:10 2020 +0000
Committer: Jeremy Harris <jgh146exb@???>
CommitDate: Sat Jan 11 21:49:10 2020 +0000
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(-)
diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt
index 0e44b11..1d6fa53 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 c803fdb..33381d5 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 e64cb94..be70eff 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 201e82d..574b86c 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 69e02eb..d70a38e 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 180620f..ea6b78f 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@???
- 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 c8bd1f3..779e155 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 c201989..1f61ca3 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 3608eee..7ae6a2a 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 0000000..1b2d835
--- /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 0000000..0e48328
--- /dev/null
+++ b/test/scripts/0000-Basic/0586
@@ -0,0 +1,4 @@
+# tainted data for filter filename
+1
+exim -bv abcd@???
+****
diff --git a/test/stdout/0586 b/test/stdout/0586
new file mode 100644
index 0000000..0fe0617
--- /dev/null
+++ b/test/stdout/0586
@@ -0,0 +1,2 @@
+abcd@??? cannot be resolved at this time: Tainted name 'TESTSUITE/aux-fixed/0586.list.abcd' for file read not permitted
+
