redirect router: taint-enforce filenames
authorJeremy Harris <jgh146exb@wizmail.org>
Sat, 11 Jan 2020 21:49:10 +0000 (21:49 +0000)
committerJeremy Harris <jgh146exb@wizmail.org>
Sat, 11 Jan 2020 21:49:10 +0000 (21:49 +0000)
12 files changed:
doc/doc-docbook/spec.xfpt
doc/doc-txt/ChangeLog
src/src/parse.c
src/src/rda.c
test/confs/0153
test/confs/0251
test/confs/0306
test/confs/0307
test/confs/0452
test/confs/0586 [new file with mode: 0644]
test/scripts/0000-Basic/0586 [new file with mode: 0644]
test/stdout/0586 [new file with mode: 0644]

index 0e44b11..1d6fa53 100644 (file)
@@ -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"
index c803fdb..33381d5 100644 (file)
@@ -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.
 
 
index e64cb94..be70eff 100644 (file)
@@ -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)
index 201e82d..574b86c 100644 (file)
@@ -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);
index 69e02eb..d70a38e 100644 (file)
@@ -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:
index 180620f..ea6b78f 100644 (file)
@@ -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
index c8bd1f3..779e155 100644 (file)
@@ -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
index c201989..1f61ca3 100644 (file)
@@ -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
index 3608eee..7ae6a2a 100644 (file)
@@ -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
@@ -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
@@ -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
@@ -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
+