redirect router: taint-enforce filenames

12 files changed, 81 insertions, 32 deletions

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
+