redirect router: taint-enforce filenames

diff --git a/doc/doc-docbook/spec.xfpt b/doc/doc-docbook/spec.xfpt
index 0e44b119bca1e7af29c51edca5af04c774b5e983..1d6fa536b45352f7fdb1fe31acc71a2d5a45b3f4 100644 (file)
--- 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 c803fdb7edb1765250ad89de76afa56146096ec6..33381d5583258e8ce1b0590a1fad1536adb989f9 100644 (file)
--- 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 e64cb94a12f8d689a900a43af5bdcb5af608e73e..be70effe9fbda10bc21ed00c72c810971fec65a6 100644 (file)
--- 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 201e82d8b8a0bee54fd1344f9d622f1c0eba9d18..574b86cdd57b959de52d7f1498ad7afdcd00de70 100644 (file)
--- 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 69e02ebcf657fc13d41579aee9701969967c7d87..d70a38e7b85943d2c36806219317c96476cafeb3 100644 (file)
--- 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 180620f11663f6af640c74ef472c5d0e777ec7ef..ea6b78f5ead28465876fda230f38626013c48557 100644 (file)
--- 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 c8bd1f362f5bf2696a5e5d159ccda8e012bc6a98..779e155fc86e7601d8f5c63ca0dbc68e175626e7 100644 (file)
--- 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 c2019893a1fc065f879f5987955e14b85ccc70a1..1f61ca3cbbe3d09d50391728f64ade966e59c5b1 100644 (file)
--- 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 3608eeed2403868046d4877eb004e2902b90aa63..7ae6a2ad0139666b1c970cd063ae16cabd40f2a8 100644 (file)
--- 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 (file)
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 (file)
index 0000000..0e48328
--- /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 (file)
index 0000000..0fe0617
--- /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
+