Rewrite parse_config_file(). Among the old version's problems:

  No checking for lines that were too long.
  No checking that fgets returning NULL was actually due to EOF.
  Various whitespace handling inconsistencies.
  Bloat (switches and multiple identical function calls).
  Failure to check for trailing characters in some cases.
  Dynamicly allocated memory was not free()d on error.
Given that this controls suid/sgid behavior, the sloppy coding was
really inexcusable.  :-(

1 files changed, 220 insertions, 175 deletions

diff --git a/applets/applets.c b/applets/applets.c
index 4af569de3..f24679a1a 100644
--- a/applets/applets.c
+++ b/applets/applets.c
@@ -29,6 +29,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 #include "busybox.h"
 
 #undef APPLET
@@ -53,9 +54,7 @@ static void check_suid (struct BB_applet *app);
 #include "pwd_.h"
 #include "grp_.h"
 
-static int parse_config_file (void);
-
-static int config_ok;
+static void parse_config_file (void);
 
 #define CONFIG_FILE "/etc/busybox.conf"
 
@@ -127,7 +126,7 @@ run_applet_by_name (const char *name, int argc, char **argv)
 
 #ifdef CONFIG_FEATURE_SUID_CONFIG
   if (recurse_level == 0)
-        config_ok = parse_config_file ();
+        parse_config_file ();
 #endif
 
   recurse_level++;
@@ -193,7 +192,7 @@ check_suid (struct BB_applet *applet)
   uid_t rgid = getgid ();
 
 #ifdef CONFIG_FEATURE_SUID_CONFIG
-  if (config_ok) {
+  if (suid_config) {
         struct BB_suid_config *sct;
 
         for (sct = suid_config; sct; sct = sct->m_next) {
@@ -253,191 +252,237 @@ check_suid (struct BB_applet *applet)
 
 #ifdef CONFIG_FEATURE_SUID_CONFIG
 
+/* This should probably be a libbb routine.  In that case,
+ * I'd probably rename it to something like bb_trimmed_slice.
+ */
+static char *get_trimmed_slice(char *s, char *e)
+{
+        /* First, consider the value at e to be nul and back up until we
+         * reach a non-space char.  Set the char after that (possibly at
+         * the original e) to nul. */
+        while (e-- > s) {
+                if (!isspace(*e)) {
+                        break;
+                }
+        }
+        e[1] = 0;
+
+        /* Next, advance past all leading space and return a ptr to the
+         * first non-space char; possibly the terminating nul. */
+        return (char *) bb_skip_whitespace(s);
+}
+
 
 #define parse_error(x)  { err=x; goto pe_label; }
 
+/* Don't depend on the tools to combine strings. */
+static const char config_file[] = CONFIG_FILE;
+
+/* There are 4 chars + 1 nul for each of user/group/other. */
+static const char mode_chars[] = "Ssx-\0Ssx-\0Ttx-";
+
+/* We don't supply a value for the nul, so an index adjustment is
+ * necessary below.  Also, we use unsigned short here to save some
+ * space even though these are really mode_t values. */
+static const unsigned short mode_mask[] = {
+        /*  SST         sst                 xxx   --- */
+        S_ISUID,    S_ISUID|S_IXUSR,    S_IXUSR,    0,  /* user */
+        S_ISGID,    S_ISGID|S_IXGRP,    S_IXGRP,    0,  /* group */
+        0,          S_IXOTH,            S_IXOTH,    0   /* other */
+};
 
-int
-parse_config_file (void)
+static void parse_config_file(void)
 {
-  struct stat st;
-  char *err = 0;
-  FILE *f = 0;
-  int lc = 0;
-
-  suid_config = 0;
-
-  /* is there a config file ? */
-  if (stat (CONFIG_FILE, &st) == 0) {
-        /* is it owned by root with no write perm. for group and others ? */
-        if (S_ISREG (st.st_mode) && (st.st_uid == 0)
-                && (!(st.st_mode & (S_IWGRP | S_IWOTH)))) {
-          /* that's ok .. then try to open it */
-          f = fopen (CONFIG_FILE, "r");
-
-          if (f) {
-                char buffer[256];
-                int section = 0;
-
-                while (fgets (buffer, sizeof (buffer) - 1, f)) {
-                  char c = buffer[0];
-                  char *p;
-
-                  lc++;
-
-                  p = strchr (buffer, '#');
-                  if (p)
-                        *p = 0;
-                  p = buffer + bb_strlen (buffer);
-                  while ((p > buffer) && isspace (*--p))
-                        *p = 0;
-
-                  if (p == buffer)
-                        continue;
+        struct BB_suid_config *sct_head;
+        struct BB_suid_config *sct;
+        struct BB_applet *applet;
+        FILE *f;
+        char *err;
+        char *s;
+        char *e;
+        int i, lc, section;
+        char buffer[256];
+        struct stat st;
+
+        assert(!suid_config);           /* Should be set to NULL by bss init. */
+
+        if ((stat(config_file, &st) != 0)                       /* No config file? */
+                || !S_ISREG(st.st_mode)                                 /* Not a regular file? */
+                || (st.st_uid != 0)                                             /* Not owned by root? */
+                || (st.st_mode & (S_IWGRP | S_IWOTH))   /* Writable by non-root? */
+                || !(f = fopen(config_file, "r"))               /* Can not open? */
+                ) {
+                return;
+        }
+
+        sct_head = NULL;
+        section = lc = 0;
+
+        do {
+                s = buffer;
 
-                  if (c == '[') {
-                        p = strchr (buffer, ']');
+                if (!fgets(s, sizeof(buffer), f)) { /* Are we done? */
+                        if (ferror(f)) {   /* Make sure it wasn't a read error. */
+                                parse_error("reading");
+                        }
+                        fclose(f);
+                        suid_config = sct_head; /* Success, so set the pointer. */
+                        return;
+                }
 
-                        if (!p || (p == (buffer + 1)))  /* no matching ] or empty [] */
-                          parse_error ("malformed section header");
+                lc++;                                   /* Got a (partial) line. */
+
+                /* If a line is too long for our buffer, we consider it an error.
+                 * The following test does mistreat one corner case though.
+                 * If the final line of the file does not end with a newline and
+                 * yet exactly fills the buffer, it will be treated as too long
+                 * even though there isn't really a problem.  But it isn't really
+                 * worth adding code to deal with such an unlikely situation, and
+                 * we do err on the side of caution.  Besides, the line would be
+                 * too long if it did end with a newline. */
+                if (!strchr(s, '\n') && !feof(f)) {
+                        parse_error("line too long");
+                }
+
+                /* Trim leading and trailing whitespace, ignoring comments, and
+                 * check if the resulting string is empty. */
+                if (!*(s = get_trimmed_slice(s, strchrnul(s, '#')))) {
+                        continue;
+                }
 
-                        *p = 0;
+                /* Check for a section header. */
+
+                if (*s == '[') {
+                        /* Unlike the old code, we ignore leading and trailing
+                         * whitespace for the section name.  We also require that
+                         * there are no stray characters after the closing bracket. */
+                        if (!(e = strchr(s, ']'))       /* Missing right bracket? */
+                                || e[1]                                 /* Trailing characters? */
+                                || !*(s = get_trimmed_slice(s+1, e)) /* Missing name? */
+                                ) {
+                                parse_error("section header");
+                        }
+                        /* Right now we only have one section so just check it.
+                         * If more sections are added in the future, please don't
+                         * resort to cascading ifs with multiple strcasecmp calls.
+                         * That kind of bloated code is all too common.  A loop
+                         * and a string table would be a better choice unless the
+                         * number of sections is very small. */
+                        if (strcasecmp(s, "SUID") == 0) {
+                                section = 1;
+                                continue;
+                        }
+                        section = -1;   /* Unknown section so set to skip. */
+                        continue;
+                }
 
-                        if (strcasecmp (buffer + 1, "SUID") == 0)
-                          section = 1;
-                        else
-                          section = -1;         /* unknown section - just skip */
-                  } else if (section) {
-                        switch (section) {
-                        case 1:{                        /* SUID */
-                                int l;
-                                struct BB_applet *applet;
+                /* Process sections. */
 
-                                p = strchr (buffer, '=');       /* <key>[::space::]*=[::space::]*<value> */
+                if (section == 1) {             /* SUID */
+                        /* Since we trimmed leading and trailing space above, we're
+                         * now looking for strings of the form
+                         *    <key>[::space::]*=[::space::]*<value>
+                         * where both key and value could contain inner whitespace. */
 
-                                if (!p || (p == (buffer + 1)))  /* no = or key is empty */
-                                  parse_error ("malformed keyword");
+                        /* First get the key (an applet name in our case). */
+                        if (!!(e = strchr(s, '='))) {
+                                s = get_trimmed_slice(s, e);
+                        }
+                        if (!e || !*s) {        /* Missing '=' or empty key. */
+                                parse_error("keyword");
+                        }
 
-                                l = p - buffer;
-                                while (isspace (buffer[--l])) {
-                                        /* skip whitespace */
+                        /* Ok, we have an applet name.  Process the rhs if this
+                         * applet is currently built in and ignore it otherwise.
+                         * Note: This can hide config file bugs which only pop
+                         * up when the busybox configuration is changed. */
+                        if ((applet = find_applet_by_name(s))) {
+                                /* Note: We currently don't check for duplicates!
+                                 * The last config line for each applet will be the
+                                 * one used since we insert at the head of the list.
+                                 * I suppose this could be considered a feature. */
+                                sct = xmalloc(sizeof(struct BB_suid_config));
+                                sct->m_applet = applet;
+                                sct->m_mode = 0;
+                                sct->m_next = sct_head;
+                                sct_head = sct;
+
+                                /* Get the specified mode. */
+
+                                e = (char *) bb_skip_whitespace(e+1);
+
+                                for (i=0 ; i < 3 ; i++) {
+                                        const char *q;
+                                        if (!*(q = strchrnul(mode_chars + 5*i, *e++))) {
+                                                parse_error("mode");
+                                        }
+                                        /* Adjust by -i to account for nul. */
+                                        sct->m_mode |= mode_mask[(q - mode_chars) - i];
                                 }
 
-                                buffer[l + 1] = 0;
-
-                                if ((applet = find_applet_by_name (buffer))) {
-                                  struct BB_suid_config *sct =
-                                        xmalloc (sizeof (struct BB_suid_config));
-
-                                  sct->m_applet = applet;
-                                  sct->m_next = suid_config;
-                                  suid_config = sct;
-
-                                  while (isspace (*++p)) {
-                                        /* skip whitespace */
-                                  }
-
-                                  sct->m_mode = 0;
-
-                                  switch (*p++) {
-                                  case 'S':
-                                        sct->m_mode |= S_ISUID;
-                                        break;
-                                  case 's':
-                                        sct->m_mode |= S_ISUID;
-                                        /* no break */
-                                  case 'x':
-                                        sct->m_mode |= S_IXUSR;
-                                        break;
-                                  case '-':
-                                        break;
-                                  default:
-                                        parse_error ("invalid user mode");
-                                  }
-
-                                  switch (*p++) {
-                                  case 's':
-                                        sct->m_mode |= S_ISGID;
-                                        /* no break */
-                                  case 'x':
-                                        sct->m_mode |= S_IXGRP;
-                                        break;
-                                  case 'S':
-                                        break;
-                                  case '-':
-                                        break;
-                                  default:
-                                        parse_error ("invalid group mode");
-                                  }
-
-                                  switch (*p) {
-                                  case 't':
-                                  case 'x':
-                                        sct->m_mode |= S_IXOTH;
-                                        break;
-                                  case 'T':
-                                  case '-':
-                                        break;
-                                  default:
-                                        parse_error ("invalid other mode");
-                                  }
-
-                                  while (isspace (*++p)) {
-                                        /* skip whitespace */
-                                  }
-
-                                  if (isdigit (*p)) {
-                                        sct->m_uid = strtol (p, &p, 10);
-                                        if (*p++ != '.')
-                                          parse_error ("parsing <uid>.<gid>");
-                                  } else {
-                                        struct passwd *pwd;
-                                        char *p2 = strchr (p, '.');
-
-                                        if (!p2)
-                                          parse_error ("parsing <uid>.<gid>");
-
-                                        *p2 = 0;
-                                        pwd = getpwnam (p);
-
-                                        if (!pwd)
-                                          parse_error ("invalid user name");
-
-                                        sct->m_uid = pwd->pw_uid;
-                                        p = p2 + 1;
-                                  }
-                                  if (isdigit (*p))
-                                        sct->m_gid = strtol (p, &p, 10);
-                                  else {
-                                        struct group *grp = getgrnam (p);
-
-                                        if (!grp)
-                                          parse_error ("invalid group name");
-
-                                        sct->m_gid = grp->gr_gid;
-                                  }
+                                /* Now get the the user/group info. */
+                 
+                                s = (char *) bb_skip_whitespace(e);
+
+                                /* Note: We require whitespace between the mode and the
+                                 * user/group info. */
+                                if ((s == e) || !(e = strchr(s, '.'))) {
+                                        parse_error("<uid>.<gid>");
+                                }
+                                *e++ = 0;
+
+                                /* We can't use get_ug_id here since it would exit()
+                                 * if a uid or gid was not found.  Oh well... */
+                                {
+                                        char *e2;
+
+                                        sct->m_uid = strtoul(s, &e2, 10);
+                                        if (*e2 || (s == e2)) {
+                                                struct passwd *pwd;
+                                                if (!(pwd = getpwnam(s))) {
+                                                        parse_error("user");
+                                                }
+                                                sct->m_uid = pwd->pw_uid;
+                                        }
+
+                                        sct->m_gid = strtoul(e, &e2, 10);
+                                        if (*e2 || (e == e2)) {
+                                                struct group *grp;
+                                                if (!(grp = getgrnam(e))) {
+                                                        parse_error("group");
+                                                }
+                                                sct->m_gid = grp->gr_gid;
+                                        }
                                 }
-                                break;
-                          }
-                        default:                        /* unknown - skip */
-                          break;
                         }
-                  } else
-                        parse_error ("keyword not within section");
+                        continue;
                 }
-                fclose (f);
-                return 1;
-          }
-        }
-  }
-  return 0;     /* no config file or not readable (not an error) */
 
-pe_label:
-  fprintf (stderr, "Parse error in %s, line %d: %s\n", CONFIG_FILE, lc, err);
+                /* Unknown sections are ignored. */
 
-  if (f)
-        fclose (f);
-  return 0;
+                /* Encountering configuration lines prior to seeing a
+                 * section header is treated as an error.  This is how
+                 * the old code worked, but it may not be desireable.
+                 * We may want to simply ignore such lines in case they
+                 * are used in some future version of busybox. */
+                if (!section) {
+                        parse_error("keyword outside section");
+                }
+
+        } while (1);
+
+ pe_label:
+        fprintf(stderr, "Parse error in %s, line %d: %s\n",
+                        config_file, lc, err);
+
+        fclose(f);
+        /* Release any allocated memory before returning. */
+        while (sct_head) {
+                sct = sct_head->m_next;
+                free(sct_head);
+                sct_head = sct;
+        }
+        return;
 }
 
 #endif
@@ -446,9 +491,9 @@ pe_label:
 
 /* END CODE */
 /*
-Local Variables:
-c-file-style: "linux"
-c-basic-offset: 4
-tab-width: 4
+  Local Variables:
+  c-file-style: "linux"
+  c-basic-offset: 4
+  tab-width: 4
 End:
 */