From 53cc1417d804b27674f9e18fec09dee3badd080b Mon Sep 17 00:00:00 2001
From: Jeremy Harris <jgh146exb@wizmail.org>
Date: Thu, 24 Dec 2020 20:59:29 +0000
Subject: [PATCH] Convert more cases of list-walking to use self-assigned
 memory for the list-item

---
 src/src/auths/plaintext.c       |  2 +-
 src/src/bmi_spam.c              |  4 +++-
 src/src/daemon.c                |  1 +
 src/src/deliver.c               |  1 +
 src/src/exim.c                  |  1 +
 src/src/expand.c                | 10 ++++++++--
 src/src/filter.c                |  8 +++-----
 src/src/host.c                  |  7 ++-----
 src/src/lookups/ibase.c         |  3 +--
 src/src/lookups/oracle.c        |  3 +--
 src/src/match.c                 |  3 +--
 src/src/moan.c                  |  3 +--
 src/src/readconf.c              |  5 +++++
 src/src/receive.c               |  4 ++--
 src/src/route.c                 |  3 +--
 src/src/routers/dnslookup.c     |  2 ++
 src/src/routers/iplookup.c      |  1 +
 src/src/smtp_out.c              |  1 +
 src/src/srs.c                   |  3 +++
 src/src/string.c                |  3 +++
 src/src/transports/appendfile.c |  3 ++-
 21 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/src/src/auths/plaintext.c b/src/src/auths/plaintext.c
index 778e6c2c4..ab703daec 100644
--- a/src/src/auths/plaintext.c
+++ b/src/src/auths/plaintext.c
@@ -109,7 +109,7 @@ already been provided as part of the AUTH command. For the rest, send them
 out as prompts, and get a data item back. If the data item is "*", abandon the
 authentication attempt. Otherwise, split it into items as above. */
 
-while (  (s = string_nextinlist(&prompts, &sep, big_buffer, big_buffer_size))
+while (  (s = string_nextinlist(&prompts, &sep, NULL, 0))
       && expand_nmax < EXPAND_MAXN)
   if (number++ > expand_nmax)
     if ((rc = auth_prompt(CUS s)) != OK)
diff --git a/src/src/bmi_spam.c b/src/src/bmi_spam.c
index 6651de5ad..6972bc3a7 100644
--- a/src/src/bmi_spam.c
+++ b/src/src/bmi_spam.c
@@ -448,9 +448,11 @@ int bmi_check_rule(uschar *base64_verdict, uschar *option_list) {
   }
 
   /* loop through numbers */
+  /* option_list doesn't seem to be expanded so cannot be tainted.  If it ever is we
+  will trap here */
   rule_ptr = option_list;
   while ((rule_num = string_nextinlist(&rule_ptr, &sep,
-                                       rule_buffer, 32)) != NULL) {
+                                       rule_buffer, sizeof(rule_buffer)))) {
     int rule_int = -1;
 
     /* try to translate to int */
diff --git a/src/src/daemon.c b/src/src/daemon.c
index 83131fa1d..21807f64f 100644
--- a/src/src/daemon.c
+++ b/src/src/daemon.c
@@ -1426,6 +1426,7 @@ if (f.daemon_listen && !f.inetd_wait_mode)
 
   list = tls_in.on_connect_ports;
   sep = 0;
+  /* the list isn't expanded so cannot be tainted.  If it ever is we will trap here */
   while ((s = string_nextinlist(&list, &sep, big_buffer, big_buffer_size)))
     if (!isdigit(*s))
       {
diff --git a/src/src/deliver.c b/src/src/deliver.c
index ec5990cc1..cba8651e9 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -3184,6 +3184,7 @@ const uschar *listptr = remote_sort_domains;
 uschar *pattern;
 uschar patbuf[256];
 
+/*XXX The list is used before expansion. Not sure how that ties up with the docs */
 while (  *aptr
       && (pattern = string_nextinlist(&listptr, &sep, patbuf, sizeof(patbuf)))
       )
diff --git a/src/src/exim.c b/src/src/exim.c
index 60a44bb09..8f33bde26 100644
--- a/src/src/exim.c
+++ b/src/src/exim.c
@@ -2373,6 +2373,7 @@ on the second character (the one after '-'), to save some effort. */
       int len = Ustrlen(ALT_CONFIG_PREFIX);
       const uschar *list = argrest;
       uschar *filename;
+      /* The argv is untainted, so big_buffer (also untainted) is ok to use */
       while((filename = string_nextinlist(&list, &sep, big_buffer,
              big_buffer_size)))
         if (  (  Ustrlen(filename) < len
diff --git a/src/src/expand.c b/src/src/expand.c
index 839821ef7..e8e97dab8 100644
--- a/src/src/expand.c
+++ b/src/src/expand.c
@@ -7251,7 +7251,9 @@ while (*s)
 	uschar * item;
 	uschar * suffix = US"";
 	BOOL needsep = FALSE;
-	uschar buffer[256];
+#define LISTNAMED_BUF_SIZE 256
+	uschar b[LISTNAMED_BUF_SIZE];
+	uschar * buffer = b;
 
 	if (*sub == '+') sub++;
 	if (!arg)		/* no-argument version */
@@ -7286,7 +7288,11 @@ while (*s)
 
 	list = ((namedlist_block *)(t->data.ptr))->string;
 
-	while ((item = string_nextinlist(&list, &sep, buffer, sizeof(buffer))))
+	/* The list could be quite long so we (re)use a buffer for each element
+	rather than getting each in new memory */
+
+	if (is_tainted(list)) buffer = store_get(LISTNAMED_BUF_SIZE, TRUE);
+	while ((item = string_nextinlist(&list, &sep, buffer, LISTNAMED_BUF_SIZE)))
 	  {
 	  uschar * buf = US" : ";
 	  if (needsep)
diff --git a/src/src/filter.c b/src/src/filter.c
index 59c08f882..3897ae0f9 100644
--- a/src/src/filter.c
+++ b/src/src/filter.c
@@ -2007,11 +2007,9 @@ while (commands)
 	else if (subtype == FALSE)
 	  {
 	  int sep = 0;
-	  uschar *ss;
-	  const uschar *list = s;
-	  uschar buffer[128];
-	  while ((ss = string_nextinlist(&list, &sep, buffer, sizeof(buffer)))
-		 != NULL)
+	  const uschar * list = s;
+
+	  for (uschar * ss; ss = string_nextinlist(&list, &sep, NULL, 0); )
 	    header_remove(0, ss);
 	  }
 
diff --git a/src/src/host.c b/src/src/host.c
index 4e1cb8a45..a31c09b65 100644
--- a/src/src/host.c
+++ b/src/src/host.c
@@ -1232,14 +1232,11 @@ BOOL
 host_is_tls_on_connect_port(int port)
 {
 int sep = 0;
-uschar buffer[32];
-const uschar *list = tls_in.on_connect_ports;
-uschar *s;
-uschar *end;
+const uschar * list = tls_in.on_connect_ports;
 
 if (tls_in.on_connect) return TRUE;
 
-while ((s = string_nextinlist(&list, &sep, buffer, sizeof(buffer))))
+for (uschar * s, * end; s = string_nextinlist(&list, &sep, NULL, 0); )
   if (Ustrtol(s, &end, 10) == port)
     return TRUE;
 
diff --git a/src/src/lookups/ibase.c b/src/src/lookups/ibase.c
index 4789b6cb7..72a27c010 100644
--- a/src/src/lookups/ibase.c
+++ b/src/src/lookups/ibase.c
@@ -457,11 +457,10 @@ ibase_find(void * handle, const uschar * filename, uschar * query, int length,
 int sep = 0;
 uschar *server;
 uschar *list = ibase_servers;
-uschar buffer[512];
 
 DEBUG(D_lookup) debug_printf_indent("Interbase query: %s\n", query);
 
-while ((server = string_nextinlist(&list, &sep, buffer, sizeof(buffer))))
+while ((server = string_nextinlist(&list, &sep, NULL, 0)))
   {
   BOOL defer_break = FALSE;
   int rc = perform_ibase_search(query, server, result, errmsg, &defer_break);
diff --git a/src/src/lookups/oracle.c b/src/src/lookups/oracle.c
index 0712c89e3..136971590 100644
--- a/src/src/lookups/oracle.c
+++ b/src/src/lookups/oracle.c
@@ -510,13 +510,12 @@ oracle_find(void * handle, const uschar * filename, uschar * query, int length,
 int sep = 0;
 uschar *server;
 uschar *list = oracle_servers;
-uschar buffer[512];
 
 do_cache = do_cache;   /* Placate picky compilers */
 
 DEBUG(D_lookup) debug_printf_indent("ORACLE query: %s\n", query);
 
-while ((server = string_nextinlist(&list, &sep, buffer, sizeof(buffer))))
+while ((server = string_nextinlist(&list, &sep, NULL, 0)))
   {
   BOOL defer_break;
   int rc = perform_oracle_search(query, server, result, errmsg, &defer_break);
diff --git a/src/src/match.c b/src/src/match.c
index bf8cb3b98..597b633fe 100644
--- a/src/src/match.c
+++ b/src/src/match.c
@@ -1061,7 +1061,6 @@ if (pattern[0] == '@' && pattern[1] == '@')
   {
   int watchdog = 50;
   uschar *list, *ss;
-  uschar buffer[1024];
 
   if (sdomain == subject + 1 && *subject == '*') return FAIL;
 
@@ -1092,7 +1091,7 @@ if (pattern[0] == '@' && pattern[1] == '@')
     /* Look up the local parts provided by the list; negation is permitted.
     If a local part has to begin with !, a regex can be used. */
 
-    while ((ss = string_nextinlist(CUSS &list, &sep, buffer, sizeof(buffer))))
+    while ((ss = string_nextinlist(CUSS &list, &sep, NULL, 0)))
       {
       int local_yield;
 
diff --git a/src/src/moan.c b/src/src/moan.c
index 4e7fbd607..bf5483ce3 100644
--- a/src/src/moan.c
+++ b/src/src/moan.c
@@ -718,7 +718,6 @@ moan_check_errorcopy(uschar *recipient)
 uschar *item, *localpart, *domain;
 const uschar *listptr = errors_copy;
 uschar *yield = NULL;
-uschar buffer[256];
 int sep = 0;
 int llen;
 
@@ -734,7 +733,7 @@ llen = domain++ - recipient;
 
 /* Scan through the configured items */
 
-while ((item = string_nextinlist(&listptr, &sep, buffer, sizeof(buffer))))
+while ((item = string_nextinlist(&listptr, &sep, NULL, 0)))
   {
   const uschar *newaddress = item;
   const uschar *pattern = string_dequote(&newaddress);
diff --git a/src/src/readconf.c b/src/src/readconf.c
index 0b78a88b9..7f808def8 100644
--- a/src/src/readconf.c
+++ b/src/src/readconf.c
@@ -1848,6 +1848,7 @@ switch (type)
         flagptr = (int *)ol3->v.value;
         }
 
+      /* This will trap if sptr is tainted. Not sure if that can happen */
       while ((p = string_nextinlist(CUSS &sptr, &sep, big_buffer, BIG_BUFFER_SIZE)))
         {
         rewrite_rule *next = readconf_one_rewrite(p, flagptr, FALSE);
@@ -1992,6 +1993,7 @@ switch (type)
       while (count-- > 1)
         {
         int sep = 0;
+	/* If p is tainted we trap.  Not sure that can happen */
         (void)string_nextinlist(&p, &sep, big_buffer, BIG_BUFFER_SIZE);
         if (!route_finduser(big_buffer, NULL, &uid))
           log_write(0, LOG_PANIC_DIE|LOG_CONFIG_IN, "user %s was not found",
@@ -2033,6 +2035,7 @@ switch (type)
       while (count-- > 1)
         {
         int sep = 0;
+	/* If p is tainted we trap.  Not sure that can happen */
         (void)string_nextinlist(&p, &sep, big_buffer, BIG_BUFFER_SIZE);
         if (!route_findgroup(big_buffer, &gid))
           log_write(0, LOG_PANIC_DIE|LOG_CONFIG_IN, "group %s was not found",
@@ -3115,6 +3118,7 @@ const uschar *list = config_main_filelist;
 
 /* Loop through the possible file names */
 
+/* Should never be a tainted list */
 while((filename = string_nextinlist(&list, &sep, big_buffer, big_buffer_size)))
   {
 
@@ -3428,6 +3432,7 @@ if (*log_file_path)
       "\"%s\": %s", log_file_path, expand_string_message);
 
   ss = s;
+  /* should never be a tainted list */
   while ((sss = string_nextinlist(&ss, &sep, big_buffer, big_buffer_size)))
     {
     uschar *t;
diff --git a/src/src/receive.c b/src/src/receive.c
index 95c44c01c..00c431fc8 100644
--- a/src/src/receive.c
+++ b/src/src/receive.c
@@ -176,6 +176,7 @@ else
   empty item in a list. */
 
   if (*p == 0) p = US":";
+  /* should never be a tainted list */
   while ((path = string_nextinlist(&p, &sep, buffer, sizeof(buffer))))
     if (Ustrcmp(path, "syslog") != 0)
       break;
@@ -1228,9 +1229,8 @@ if (acl_removed_headers)
     const uschar * list = acl_removed_headers;
     int sep = ':';         /* This is specified as a colon-separated list */
     uschar *s;
-    uschar buffer[128];
 
-    while ((s = string_nextinlist(&list, &sep, buffer, sizeof(buffer))))
+    while ((s = string_nextinlist(&list, &sep, NULL, 0)))
       if (header_testname(h, s, Ustrlen(s), FALSE))
 	{
 	h->type = htype_old;
diff --git a/src/src/route.c b/src/src/route.c
index a5f5feeaf..1d87b079a 100644
--- a/src/src/route.c
+++ b/src/src/route.c
@@ -607,14 +607,13 @@ gid_t gid = 0;            /* For picky compilers */
 BOOL ugid_set = FALSE;
 const uschar *listptr;
 uschar *check;
-uschar buffer[1024];
 
 if (!s) return OK;
 
 DEBUG(D_route) debug_printf("checking require_files\n");
 
 listptr = s;
-while ((check = string_nextinlist(&listptr, &sep, buffer, sizeof(buffer))))
+while ((check = string_nextinlist(&listptr, &sep, NULL, 0)))
   {
   int rc;
   int eacces_code = 0;
diff --git a/src/src/routers/dnslookup.c b/src/src/routers/dnslookup.c
index a836c6147..41a5ad78d 100644
--- a/src/src/routers/dnslookup.c
+++ b/src/src/routers/dnslookup.c
@@ -204,6 +204,7 @@ if (  ob->widen_domains
    && (verify != v_sender || !ob->rewrite_headers || addr->parent))
   {
   listptr = ob->widen_domains;
+  /* not expanded so should never be tainted */
   widen = string_nextinlist(&listptr, &widen_sep, widen_buffer,
     sizeof(widen_buffer));
 
@@ -233,6 +234,7 @@ for (;;)
   else if (widen)
     {
     h.name = string_sprintf("%s.%s", addr->domain, widen);
+    /* not expanded so should never be tainted */
     widen = string_nextinlist(&listptr, &widen_sep, widen_buffer,
       sizeof(widen_buffer));
     DEBUG(D_route) debug_printf("%s router widened %s to %s\n", rblock->name,
diff --git a/src/src/routers/iplookup.c b/src/src/routers/iplookup.c
index 4412c625e..3035b8863 100644
--- a/src/src/routers/iplookup.c
+++ b/src/src/routers/iplookup.c
@@ -200,6 +200,7 @@ response it received. Initialization insists on the port being set and there
 being a host list. */
 
 listptr = ob->hosts;
+/* not expanded so should never be tainted */
 while ((hostname = string_nextinlist(&listptr, &sep, host_buffer,
        sizeof(host_buffer))))
   {
diff --git a/src/src/smtp_out.c b/src/src/smtp_out.c
index 911dd537e..0bf619795 100644
--- a/src/src/smtp_out.c
+++ b/src/src/smtp_out.c
@@ -67,6 +67,7 @@ if (is_tainted(expint))
 Uskip_whitespace(&expint);
 if (!*expint) return TRUE;
 
+/* we just tested to ensure no taint, so big_buffer is ok */
 while ((iface = string_nextinlist(&expint, &sep, big_buffer,
           big_buffer_size)))
   {
diff --git a/src/src/srs.c b/src/src/srs.c
index 657cd1771..5cca182da 100644
--- a/src/src/srs.c
+++ b/src/src/srs.c
@@ -50,6 +50,7 @@ if (srs == NULL)
   co = 0;
   if (srs_config != NULL)
     {
+    /* looks like list not expanded, so cannot be tainted */
     secret = string_nextinlist(&list, &co, secret_buf, SRS_MAX_SECRET_LENGTH);
 
     if ((sbufp = string_nextinlist(&list, &co, sbuf, sizeof(sbuf))))
@@ -72,6 +73,7 @@ if (srs == NULL)
   co = 0;
   list = srs_secrets;
   if (secret == NULL || *secret == '\0')
+    /* looks like list not expanded so cannot be tainted */
     if (!(secret = string_nextinlist(&list, &co, secret_buf, SRS_MAX_SECRET_LENGTH)))
       {
       log_write(0, LOG_MAIN | LOG_PANIC,
@@ -104,6 +106,7 @@ if (srs == NULL)
   srs_set_option(srs, SRS_OPTION_USEHASH, usehash);
 
   /* Extra secrets? */
+  /* looks like list not expanded so cannot be tainted */
   while((secret = string_nextinlist(&list, &co, secret_buf, SRS_MAX_SECRET_LENGTH)))
       srs_add_secret(srs, secret,
        	Ustrlen(secret) > SRS_MAX_SECRET_LENGTH ? SRS_MAX_SECRET_LENGTH :  Ustrlen(secret));
diff --git a/src/src/string.c b/src/src/string.c
index 53ff0a834..4726fbe4e 100644
--- a/src/src/string.c
+++ b/src/src/string.c
@@ -858,6 +858,9 @@ Arguments:
   separator  a pointer to the separator character in an int (see above)
   buffer     where to put a copy of the next string in the list; or
                NULL if the next string is returned in new memory
+	     Note that if the list is tainted then a provided buffer must be
+	     also (else we trap, with a message referencing the callsite).
+	     If we do the allocation, taint is handled there.
   buflen     when buffer is not NULL, the size of buffer; otherwise ignored
 
 Returns:     pointer to buffer, containing the next substring,
diff --git a/src/src/transports/appendfile.c b/src/src/transports/appendfile.c
index 8ef1a564e..93c89a450 100644
--- a/src/src/transports/appendfile.c
+++ b/src/src/transports/appendfile.c
@@ -644,7 +644,8 @@ if (len == 0) return tblock;
 
 /* Search the formats for a match */
 
-while ((s = string_nextinlist(&format,&sep,big_buffer,big_buffer_size)))
+/* not expanded so cannot be tainted */
+while ((s = string_nextinlist(&format, &sep, big_buffer, big_buffer_size)))
   {
   int slen = Ustrlen(s);
   BOOL match = len >= slen && Ustrncmp(data, s, slen) == 0;
-- 
2.30.2