Taintcheck transport-process arguments

10 files changed, 108 insertions, 19 deletions

diff --git a/src/src/child.c b/src/src/child.c
index 4c0dbabd1..f0db2a3b4 100644
--- a/src/src/child.c
+++ b/src/src/child.c
@@ -346,6 +346,7 @@ pid_t pid;
 if (is_tainted(argv[0]))
   {
   log_write(0, LOG_MAIN | LOG_PANIC, "Attempt to exec tainted path: '%s'", argv[0]);
+  errno = EPERM;
   return (pid_t)(-1);
   }
 
diff --git a/src/src/deliver.c b/src/src/deliver.c
index 8f85626a0..029b7a59b 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -2390,7 +2390,7 @@ if ((pid = exim_fork(US"delivery-local")) == 0)
       {
       ok = transport_set_up_command(&transport_filter_argv,
         tp->filter_command,
-        TRUE, PANIC, addr, US"transport filter", NULL);
+        TRUE, PANIC, addr, FALSE, US"transport filter", NULL);
       transport_filter_timeout = tp->filter_timeout;
       }
     else transport_filter_argv = NULL;
diff --git a/src/src/expand.c b/src/src/expand.c
index 2d55a8846..12edd195c 100644
--- a/src/src/expand.c
+++ b/src/src/expand.c
@@ -5527,6 +5527,7 @@ while (*s)
       {
       FILE * f;
       const uschar * arg, ** argv;
+      BOOL late_expand = TRUE;
 
       if ((expand_forbid & RDO_RUN) != 0)
         {
@@ -5534,15 +5535,42 @@ while (*s)
         goto EXPAND_FAILED;
         }
 
+      /* Handle options to the "run" */
+
+      while (*s == ',')
+	{
+	if (Ustrncmp(++s, "preexpand", 9) == 0)
+	  { late_expand = FALSE; s += 9; }
+	else
+	  {
+	  const uschar * t = s;
+	  while (isalpha(*++t)) ;
+	  expand_string_message = string_sprintf("bad option '%.*s' for run",
+						  (int)(t-s), s);
+	  goto EXPAND_FAILED;
+	  }
+	}
       Uskip_whitespace(&s);
+
       if (*s != '{')					/*}*/
         {
 	expand_string_message = US"missing '{' for command arg of run";
 	goto EXPAND_FAILED_CURLY;			/*"}*/
 	}
-      if (!(arg = expand_string_internal(s+1, TRUE, &s, skipping, TRUE, &resetok)))
-	goto EXPAND_FAILED;
-      Uskip_whitespace(&s);
+      s++;
+
+      if (late_expand)		/* this is the default case */
+	{
+	int n = Ustrcspn(s, "}");
+	arg = skipping ? NULL : string_copyn(s, n);
+	s += n;
+	}
+      else
+	{
+	if (!(arg = expand_string_internal(s, TRUE, &s, skipping, TRUE, &resetok)))
+	  goto EXPAND_FAILED;
+	Uskip_whitespace(&s);
+	}
 							/*{*/
       if (*s++ != '}')
         {						/*{*/
@@ -5562,11 +5590,12 @@ while (*s)
 
         if (!transport_set_up_command(&argv,    /* anchor for arg list */
             arg,                                /* raw command */
-            FALSE,                              /* don't expand the arguments */
-            0,                                  /* not relevant when... */
-            NULL,                               /* no transporting address */
-            US"${run} expansion",               /* for error messages */
-            &expand_string_message))            /* where to put error message */
+	    late_expand,		/* expand args if not already done */
+            0,                          /* not relevant when... */
+            NULL,                       /* no transporting address */
+	    late_expand,		/* allow tainted args, when expand-after-split */
+            US"${run} expansion",       /* for error messages */
+            &expand_string_message))    /* where to put error message */
           goto EXPAND_FAILED;
 
         /* Create the child process, making it a group leader. */
diff --git a/src/src/functions.h b/src/src/functions.h
index 5ecef6ad0..e231ce204 100644
--- a/src/src/functions.h
+++ b/src/src/functions.h
@@ -616,7 +616,7 @@ extern BOOL    transport_pass_socket(const uschar *, const uschar *, const uscha
 			);
 extern uschar *transport_rcpt_address(address_item *, BOOL);
 extern BOOL    transport_set_up_command(const uschar ***, const uschar *,
-		 BOOL, int, address_item *, const uschar *, uschar **);
+		 BOOL, int, address_item *, BOOL, const uschar *, uschar **);
 extern void    transport_update_waiting(host_item *, uschar *);
 extern BOOL    transport_write_block(transport_ctx *, uschar *, int, BOOL);
 extern void    transport_write_reset(int);
diff --git a/src/src/routers/queryprogram.c b/src/src/routers/queryprogram.c
index 644025a61..0d03f1ea3 100644
--- a/src/src/routers/queryprogram.c
+++ b/src/src/routers/queryprogram.c
@@ -291,6 +291,7 @@ if (!transport_set_up_command(&argvptr, /* anchor for arg list */
     TRUE,                               /* expand the arguments */
     0,                                  /* not relevant when... */
     NULL,                               /* no transporting address */
+    FALSE,				/* args must be untainted */
     US"queryprogram router",            /* for error messages */
     &addr->message))                    /* where to put error message */
   return DEFER;
diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c
index 3fe3dab82..fa727ed1f 100644
--- a/src/src/smtp_in.c
+++ b/src/src/smtp_in.c
@@ -5834,7 +5834,7 @@ while (done <= 0)
 	etrn_command = smtp_etrn_command;
 	deliver_domain = smtp_cmd_data;
 	rc = transport_set_up_command(&argv, smtp_etrn_command, TRUE, 0, NULL,
-	  US"ETRN processing", &error);
+	  FALSE, US"ETRN processing", &error);
 	deliver_domain = NULL;
 	if (!rc)
 	  {
diff --git a/src/src/transport.c b/src/src/transport.c
index 428d522ad..105238c9c 100644
--- a/src/src/transport.c
+++ b/src/src/transport.c
@@ -2053,6 +2053,31 @@ else
 
 
 
+/* Enforce all args untainted, for consistency with a router-sourced pipe
+command, where (because the whole line is passed as one to the tpt) a
+tainted arg taints the executable name.  It's unclear also that letting an
+attacker supply command arguments is wise. */
+
+static BOOL
+arg_is_tainted(const uschar * s, int argn, address_item * addr,
+  const uschar * etext, uschar ** errptr)
+{
+if (is_tainted(s))
+  {
+  uschar * msg = string_sprintf("Tainted arg %d for %s command: '%s'",
+				argn, etext, s);
+  if (addr)
+    {
+    addr->transport_return = FAIL;
+    addr->message = msg;
+    }
+  else *errptr = msg;
+  return TRUE;
+  }
+return FALSE;
+}
+
+
 /*************************************************
 *          Set up direct (non-shell) command     *
 *************************************************/
@@ -2070,6 +2095,7 @@ Arguments:
   expand_failed      error value to set if expansion fails; not relevant if
                      addr == NULL
   addr               chain of addresses, or NULL
+  allow_tainted_args as it says; used for ${run}
   etext              text for use in error messages
   errptr             where to put error message if addr is NULL;
                      otherwise it is put in the first address
@@ -2080,8 +2106,8 @@ Returns:             TRUE if all went well; otherwise an error will be
 
 BOOL
 transport_set_up_command(const uschar *** argvptr, const uschar * cmd,
-  BOOL expand_arguments, int expand_failed, address_item *addr,
-  const uschar * etext, uschar ** errptr)
+  BOOL expand_arguments, int expand_failed, address_item * addr,
+  BOOL allow_tainted_args, const uschar * etext, uschar ** errptr)
 {
 const uschar ** argv, * s;
 int address_count = 0, argcount = 0, max_args;
@@ -2186,6 +2212,16 @@ if (expand_arguments)
 
       for (address_item * ad = addr; ad; ad = ad->next)
         {
+	/* $pipe_addresses is spefically not checked for taint, because there is
+	a testcase (321) depending on it.  It's unclear if the exact thing being
+	done really needs to be legitimate, though I suspect it reflects an
+	actual use-case that showed up a bug.
+	This is a hole in the taint-pretection, mitigated only in that
+	shell-syntax metachars cannot be injected via this route. */
+
+	DEBUG(D_transport) if (is_tainted(ad->address))
+	  debug_printf("tainted element '%s' from $pipe_addresses\n", ad->address);
+
 	argv[i++] = ad->address;
 	argcount++;
 	}
@@ -2292,7 +2328,11 @@ if (expand_arguments)
       for (int address_pipe_i = 0;
            address_pipe_argv[address_pipe_i];
            address_pipe_i++, argcount++)
-        argv[i++] = address_pipe_argv[address_pipe_i];
+	{
+        uschar * s = address_pipe_argv[address_pipe_i];
+	if (arg_is_tainted(s, i, addr, etext, errptr)) return FALSE;
+        argv[i++] = s;
+	}
 
       /* Subtract one since we replace $address_pipe */
       argcount--;
@@ -2321,6 +2361,17 @@ if (expand_arguments)
         else *errptr = msg;
         return FALSE;
         }
+
+      if ( f.running_in_test_harness && is_tainted(expanded_arg)
+	 && Ustrcmp(etext, "queryprogram router") == 0)
+	{			/* hack, would be good to not need it */
+	DEBUG(D_transport)
+	  debug_printf("SPECIFIC TESTSUITE EXEMPTION: tainted arg '%s'\n",
+		      expanded_arg);
+	}
+      else if (  !allow_tainted_args
+	      && arg_is_tainted(expanded_arg, i, addr, etext, errptr))
+	return FALSE;
       argv[i] = expanded_arg;
       }
     }
@@ -2329,7 +2380,10 @@ if (expand_arguments)
     {
     debug_printf("direct command after expansion:\n");
     for (int i = 0; argv[i]; i++)
-      debug_printf("  argv[%d] = %s\n", i, string_printing(argv[i]));
+      {
+      debug_printf("  argv[%d] = '%s'\n", i, string_printing(argv[i]));
+      debug_print_taint(argv[i]);
+      }
     }
   }
 
diff --git a/src/src/transports/lmtp.c b/src/src/transports/lmtp.c
index 424210d39..638b52849 100644
--- a/src/src/transports/lmtp.c
+++ b/src/src/transports/lmtp.c
@@ -489,8 +489,8 @@ if (ob->cmd)
   {
   DEBUG(D_transport) debug_printf("using command %s\n", ob->cmd);
   sprintf(CS buffer, "%.50s transport", tblock->name);
-  if (!transport_set_up_command(&argv, ob->cmd, TRUE, PANIC, addrlist, buffer,
-       NULL))
+  if (!transport_set_up_command(&argv, ob->cmd, TRUE, PANIC, addrlist, FALSE,
+	buffer, NULL))
     return FALSE;
 
   /* If the -N option is set, can't do any more. Presume all has gone well. */
diff --git a/src/src/transports/pipe.c b/src/src/transports/pipe.c
index 39875b3de..ed16c4c70 100644
--- a/src/src/transports/pipe.c
+++ b/src/src/transports/pipe.c
@@ -304,7 +304,7 @@ the items if necessary. If it fails, this function fails (error information
 is in the addresses). */
 
 if (!transport_set_up_command(argvptr, cmd, expand_arguments, expand_fail,
-      addr, string_sprintf("%.50s transport", tname), NULL))
+      addr, FALSE, string_sprintf("%.50s transport", tname), NULL))
   return FALSE;
 
 /* Point to the set-up arguments. */
@@ -451,6 +451,9 @@ if (expand_arguments)
 
     for (address_item * ad = addr; ad; ad = ad->next)
       {
+      DEBUG(D_transport) if (is_tainted(ad->address))
+	debug_printf("tainted element '%s' from $pipe_addresses\n", ad->address);
+
       /*XXX string_append_listele() ? */
       if (ad != addr) g = string_catn(g, US" ", 1);
       g = string_cat(g, ad->address);
@@ -575,6 +578,7 @@ if (!cmd || !*cmd)
   }
 if (is_tainted(cmd))
   {
+  DEBUG(D_transport) debug_printf("cmd '%s' is tainted\n", cmd);
   addr->message = string_sprintf("Tainted '%s' (command "
     "for %s transport) not permitted", cmd, tblock->name);
   addr->transport_return = PANIC;
diff --git a/src/src/transports/smtp.c b/src/src/transports/smtp.c
index e38ea1502..19b06d8a9 100644
--- a/src/src/transports/smtp.c
+++ b/src/src/transports/smtp.c
@@ -3765,7 +3765,7 @@ if (tblock->filter_command)
   yield ERROR. */
 
   if (!transport_set_up_command(&transport_filter_argv,
-	tblock->filter_command, TRUE, DEFER, addrlist,
+	tblock->filter_command, TRUE, DEFER, addrlist, FALSE,
 	string_sprintf("%.50s transport filter", tblock->name), NULL))
     {
     set_errno_nohost(addrlist->next, addrlist->basic_errno, addrlist->message, DEFER,