Cleaning the linker environment as we initialize it requires less API.

Change-Id: I612fd699e46833a411589478564a1f859223c380

2 files changed, 59 insertions, 88 deletions

diff --git a/linker/linker_environ.cpp b/linker/linker_environ.cpp
index 357be6d58..8ae5a9d1d 100644
--- a/linker/linker_environ.cpp
+++ b/linker/linker_environ.cpp
@@ -40,63 +40,76 @@ bool get_AT_SECURE() {
   return _AT_SECURE_value;
 }
 
-/* Returns 1 if 'str' points to a valid environment variable definition.
- * For now, we check that:
- *  - It is smaller than MAX_ENV_LEN (to detect non-zero terminated strings)
- *  - It contains at least one equal sign that is not the first character
- */
-static int _is_valid_definition(const char* str) {
-  int pos = 0;
-  int first_equal_pos = -1;
+static void __init_AT_SECURE(unsigned* auxv) {
+  // Check auxv for AT_SECURE first to see if program is setuid, setgid,
+  // has file caps, or caused a SELinux/AppArmor domain transition.
+  for (unsigned* v = auxv; v[0]; v += 2) {
+    if (v[0] == AT_SECURE) {
+      // Kernel told us whether to enable secure mode.
+      _AT_SECURE_value = v[1];
+      return;
+    }
+  }
+
+  // We don't support ancient kernels.
+  const char* msg = "FATAL: kernel did not supply AT_SECURE\n";
+  write(2, msg, strlen(msg));
+  exit(EXIT_FAILURE);
+}
 
+// Check if the environment variable definition at 'envstr'
+// starts with '<name>=', and if so return the address of the
+// first character after the equal sign. Otherwise return NULL.
+static const char* env_match(const char* envstr, const char* name) {
+  size_t i = 0;
+
+  while (envstr[i] == name[i] && name[i] != '\0') {
+    ++i;
+  }
+
+  if (name[i] == '\0' && envstr[i] == '=') {
+    return envstr + i + 1;
+  }
+
+  return NULL;
+}
+
+static bool __is_valid_environment_variable(const char* name) {
   // According to its sources, the kernel uses 32*PAGE_SIZE by default
   // as the maximum size for an env. variable definition.
   const int MAX_ENV_LEN = 32*4096;
 
-  if (str == NULL) {
-    return 0;
+  if (name == NULL) {
+    return false;
   }
 
   // Parse the string, looking for the first '=' there, and its size.
+  int pos = 0;
+  int first_equal_pos = -1;
   while (pos < MAX_ENV_LEN) {
-    if (str[pos] == '\0') {
+    if (name[pos] == '\0') {
       break;
     }
-    if (str[pos] == '=' && first_equal_pos < 0) {
+    if (name[pos] == '=' && first_equal_pos < 0) {
       first_equal_pos = pos;
     }
     pos++;
   }
 
+  // Check that it's smaller than MAX_ENV_LEN (to detect non-zero terminated strings).
   if (pos >= MAX_ENV_LEN) {
-    return 0; // Too large.
+    return false;
   }
 
+  // Check that it contains at least one equal sign that is not the first character
   if (first_equal_pos < 1) {
-    return 0; // No equals sign, or it's the first character.
-  }
-
-  return 1;
-}
-
-static void __init_AT_SECURE(unsigned* auxv) {
-  // Check auxv for AT_SECURE first to see if program is setuid, setgid,
-  // has file caps, or caused a SELinux/AppArmor domain transition.
-  for (unsigned* v = auxv; v[0]; v += 2) {
-    if (v[0] == AT_SECURE) {
-      // Kernel told us whether to enable secure mode.
-      _AT_SECURE_value = v[1];
-      return;
-    }
+    return false;
   }
 
-  // We don't support ancient kernels.
-  const char* msg = "FATAL: kernel did not supply AT_SECURE\n";
-  write(2, msg, strlen(msg));
-  exit(EXIT_FAILURE);
+  return true;
 }
 
-static void __remove_unsafe_environment_variables() {
+static bool __is_unsafe_environment_variable(const char* name) {
   // None of these should be allowed in setuid programs.
   static const char* const UNSAFE_VARIABLE_NAMES[] = {
       "GCONV_PATH",
@@ -127,17 +140,24 @@ static void __remove_unsafe_environment_variables() {
       NULL
   };
   for (size_t i = 0; UNSAFE_VARIABLE_NAMES[i] != NULL; ++i) {
-    linker_env_unset(UNSAFE_VARIABLE_NAMES[i]);
+    if (env_match(name, UNSAFE_VARIABLE_NAMES[i]) != NULL) {
+      return true;
+    }
   }
+  return false;
 }
 
-static void __remove_invalid_environment_variables() {
+static void __sanitize_environment_variables() {
   char** src  = _envp;
   char** dst = _envp;
   for (; src[0] != NULL; ++src) {
-    if (!_is_valid_definition(src[0])) {
+    if (!__is_valid_environment_variable(src[0])) {
       continue;
     }
+    // Remove various unsafe environment variables if we're loading a setuid program.
+    if (get_AT_SECURE() && __is_unsafe_environment_variable(src[0])) {
+        continue;
+    }
     dst[0] = src[0];
     ++dst;
   }
@@ -156,42 +176,19 @@ unsigned* linker_env_init(unsigned* environment_and_aux_vectors) {
   }
   ++aux_vectors;
 
-  __remove_invalid_environment_variables();
   __init_AT_SECURE(aux_vectors);
-
-  // Sanitize environment if we're loading a setuid program.
-  if (get_AT_SECURE()) {
-    __remove_unsafe_environment_variables();
-  }
+  __sanitize_environment_variables();
 
   return aux_vectors;
 }
 
-/* Check if the environment variable definition at 'envstr'
- * starts with '<name>=', and if so return the address of the
- * first character after the equal sign. Otherwise return NULL.
- */
-static char* env_match(char* envstr, const char* name) {
-  size_t i = 0;
-
-  while (envstr[i] == name[i] && name[i] != '\0') {
-    ++i;
-  }
-
-  if (name[i] == '\0' && envstr[i] == '=') {
-    return envstr + i + 1;
-  }
-
-  return NULL;
-}
-
 const char* linker_env_get(const char* name) {
   if (name == NULL || name[0] == '\0') {
     return NULL;
   }
 
   for (char** p = _envp; p[0] != NULL; ++p) {
-    char* val = env_match(p[0], name);
+    const char* val = env_match(p[0], name);
     if (val != NULL) {
       if (val[0] == '\0') {
         return NULL; // Return NULL for empty strings.
@@ -201,22 +198,3 @@ const char* linker_env_get(const char* name) {
   }
   return NULL;
 }
-
-void linker_env_unset(const char* name) {
-  char** readp = _envp;
-  char** writep = readp;
-
-  if (name == NULL || name[0] == '\0') {
-    return;
-  }
-
-  for ( ; readp[0] != NULL; readp++ ) {
-    if (env_match(readp[0], name)) {
-      continue;
-    }
-    writep[0] = readp[0];
-    writep++;
-  }
-  /* end list with a NULL */
-  writep[0] = NULL;
-}
diff --git a/linker/linker_environ.h b/linker/linker_environ.h
index a0bd69f57..d808728c7 100644
--- a/linker/linker_environ.h
+++ b/linker/linker_environ.h
@@ -34,15 +34,8 @@
 // returns the start of the aux vectors after the environment block.
 extern unsigned* linker_env_init(unsigned* environment_and_aux_vectors);
 
-// Unset a given environment variable. In case the variable is defined
-// multiple times, unset all instances. This modifies the environment
-// block, so any pointer returned by linker_env_get() after this call
-// might become invalid.
-extern void linker_env_unset(const char* name);
-
 // Returns the value of environment variable 'name' if defined and not
-// empty, or NULL otherwise. Note that the returned pointer may become
-// invalid if linker_env_unset() is called after this function.
+// empty, or NULL otherwise.
 extern const char* linker_env_get(const char* name);
 
 // Returns the value of this program's AT_SECURE variable.