[PATCHv2 0/2] Environment variable security and tunables

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHv2 0/2] Environment variable security and tunables

Siddhesh Poyarekar-9
Hi,

Here's take 2 of the patchset that fixes environment variable processing for
AT_SECURE processes.  The second patch removes GLIBC_TUNABLES from AT_SECURE
processes even when tunables are not built, to avoid passing on the variable
(and hence unsafe tunables) to child processes who may end up loading a glibc
with tunables enabled.

I also have a patch pending review that marks GLIBC_TUNABLES as insecure
for 2.24 and earlier:

https://sourceware.org/ml/libc-alpha/2017-01/msg00555.html

Changes:

 - Fixed the broken logic to erase tunables from the valstring
 - Added a couple of tests to verify the removal
 - Marked GLIBC_TUNABLES insecure in unsecvars.h

Siddhesh

Siddhesh Poyarekar (2):
  tunables: Fix environment variable processing for setuid binaries
  Drop GLIBC_TUNABLES for setxid programs when tunables is disabled

 elf/Makefile                  |   6 +-
 elf/dl-tunable-types.h        |  15 +++
 elf/dl-tunables.c             | 172 +++++++++++++++++++-------
 elf/dl-tunables.h             |  15 ++-
 elf/dl-tunables.list          |  16 ++-
 elf/tst-env-setuid-tunables.c |  69 +++++++++++
 elf/tst-env-setuid.c          | 282 ++++++++++++++++++++++++++++++++++++++++++
 scripts/gen-tunables.awk      |   8 +-
 sysdeps/generic/unsecvars.h   |   7 ++
 9 files changed, 531 insertions(+), 59 deletions(-)
 create mode 100644 elf/tst-env-setuid-tunables.c
 create mode 100644 elf/tst-env-setuid.c

--
2.7.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Drop GLIBC_TUNABLES for setxid programs when tunables is disabled

Siddhesh Poyarekar-9
A setxid program that uses a glibc with tunables disabled may pass on
GLIBC_TUNABLES as is to its child processes.  If the child process
ends up using a different glibc that has tunables enabled, it will end
up getting access to unsafe tunables.  To fix this, remove
GLIBC_TUNABLES from the environment for setxid process.

        * sysdeps/generic/unsecvars.h: Add GLIBC_TUNABLES.
        * elf/tst-env-setuid-tunables.c
        (test_child_tunables)[!HAVE_TUNABLES]: Verify that
        GLIBC_TUNABLES is removed in a setgid process.
---
 elf/tst-env-setuid-tunables.c | 9 +++++++++
 sysdeps/generic/unsecvars.h   | 7 +++++++
 2 files changed, 16 insertions(+)

diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
index a563f69..a5f0a81 100644
--- a/elf/tst-env-setuid-tunables.c
+++ b/elf/tst-env-setuid-tunables.c
@@ -36,6 +36,7 @@ test_child_tunables (void)
 {
   const char *val = getenv ("GLIBC_TUNABLES");
 
+#if HAVE_TUNABLES
   if (val != NULL && strcmp (val, CHILD_VALSTRING_VALUE) == 0)
     return 0;
 
@@ -43,6 +44,14 @@ test_child_tunables (void)
     printf ("Unexpected GLIBC_TUNABLES VALUE %s\n", val);
 
   return 1;
+#else
+  if (val != NULL)
+    {
+      printf ("GLIBC_TUNABLES not cleared\n");
+      return 1;
+    }
+  return 0;
+#endif
 }
 
 static int
diff --git a/sysdeps/generic/unsecvars.h b/sysdeps/generic/unsecvars.h
index d5b8119..a740837 100644
--- a/sysdeps/generic/unsecvars.h
+++ b/sysdeps/generic/unsecvars.h
@@ -1,9 +1,16 @@
+#if !HAVE_TUNABLES
+# define GLIBC_TUNABLES_ENVVAR "GLIBC_TUNABLES\0"
+#else
+# define GLIBC_TUNABLES_ENVVAR
+#endif
+
 /* Environment variable to be removed for SUID programs.  The names are
    all stuffed in a single string which means they have to be terminated
    with a '\0' explicitly.  */
 #define UNSECURE_ENVVARS \
   "GCONV_PATH\0"      \
   "GETCONF_DIR\0"      \
+  GLIBC_TUNABLES_ENVVAR      \
   "HOSTALIASES\0"      \
   "LD_AUDIT\0"      \
   "LD_DEBUG\0"      \
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Drop GLIBC_TUNABLES for setxid programs when tunables is disabled

Florian Weimer-5
On 02/01/2017 12:37 PM, Siddhesh Poyarekar wrote:
> * sysdeps/generic/unsecvars.h: Add GLIBC_TUNABLES.
> * elf/tst-env-setuid-tunables.c
> (test_child_tunables)[!HAVE_TUNABLES]: Verify that
> GLIBC_TUNABLES is removed in a setgid process.

Looks reasonable.  Thanks.

Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

Florian Weimer-5
In reply to this post by Siddhesh Poyarekar-9
On 02/01/2017 12:37 PM, Siddhesh Poyarekar wrote:

> * elf/dl-tunable-types.h (tunable_seclevel_t): New enum.
> * elf/dl-tunables.c (tunables_strdup): Remove.
> (get_next_env): Also return the previous envp.
> (copy_to_heap): New function.
> (parse_tunables): Erase tunables of category
> TUNABLES_SECLEVEL_SXID_ERASE.
> (maybe_enable_malloc_check): Make MALLOC_CHECK_
> TUNABLE_SECLEVEL_NONE if /etc/setuid-debug is accessible.
> (__tunables_init)[TUNABLES_FRONTEND ==
> TUNABLES_FRONTEND_valstring]: Update GLIBC_TUNABLES envvar
> after parsing.
> [TUNABLES_FRONTEND != TUNABLES_FRONTEND_valstring]: Erase
> tunable envvars of category TUNABLES_SECLEVEL_SXID_ERASE.
> * elf/dl-tunables.h (struct _tunable): Change member is_secure
> to security_level.
> * elf/dl-tunables.list: Add security_level annotations for all
> tunables.
> * scripts/gen-tunables.awk: Recognize and generate enum values
> for security_level.
> * elf/tst-env-setuid.c: New test case.
> * elf/tst-env-setuid-tunables: new test case.
> * elf/Makefile (tests-static): Add them.

Changelog needs reference to bug 21073.

>  #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
> +# define ALLOC_SIZE 4096
> +/* Allocate bytes on heap to store tunable values copied over from the
> +   valstring.  We use a hardcoded ALLOC_SIZE to avoid querying the page size,
> +   since it may not be available this early in the startup process.  */

I still don't think this micro-optimization is worthwhile.  If we want
to avoid allocations, we should avoid the copies (they would only be
needed for strings and for variable rewriting under AT_SECURE).

> -      tunable_initialize (cur, value);
> +      /* If we are in a secure context (AT_SECURE) then ignore the tunable
> + unless it is explicitly marked as secure.  Tunable values take
> + precendence over their envvar aliases.  */
> +      if (__libc_enable_secure)
> + {
> +  if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> +    {
> +      if (p[len] == '\0')
> + {
> +  /* Last tunable in the valstring.  Null-terminate and
> +     return.  */
> +  *name = '\0';
> +  return;
> + }
> +      else
> + {
> +  char *q = &p[len + 1];
> +  p = name;
> +  while (*q != '\0')
> +    *name++ = *q++;
> +  name[0] = '\0';
> +  len = 0;
> + }
> +    }
> +
> +  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> +    break;
> + }
> +
> +      char *val = copy_to_heap (value, len);
> +      if (val != NULL)
> + tunable_initialize (cur, val);
>        break;
>      }
>   }
>
> -      if (end == ':')
> +      if (p[len] == '\0')
> + return;
> +      else
>   p += len + 1;
> -      else
> - return;
>      }
>  }

I believe this correctly rewrites the string.  But reusing the “name”
variable in the move-forward loop is confusing.  Maybe also add a
comment to explain what the loop is doing.

> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> new file mode 100644
> index 0000000..7def663
> --- /dev/null
> +++ b/elf/tst-env-setuid.c
> @@ -0,0 +1,282 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.


Copyright should start in, uh, 2012 because …

> +/* Copies the executable into a restricted directory, so that we can
> +   safely make it SGID with the TARGET group ID.  Then runs the
> +   executable.  */
> +static int
> +run_executable_sgid (gid_t target)

… this code looks familiar.

We should probably consolidate this functionality in support/ for 2.26.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

Siddhesh Poyarekar-9
On Wednesday 01 February 2017 11:56 PM, Florian Weimer wrote:

> On 02/01/2017 12:37 PM, Siddhesh Poyarekar wrote:
>
>>     * elf/dl-tunable-types.h (tunable_seclevel_t): New enum.
>>     * elf/dl-tunables.c (tunables_strdup): Remove.
>>     (get_next_env): Also return the previous envp.
>>     (copy_to_heap): New function.
>>     (parse_tunables): Erase tunables of category
>>     TUNABLES_SECLEVEL_SXID_ERASE.
>>     (maybe_enable_malloc_check): Make MALLOC_CHECK_
>>     TUNABLE_SECLEVEL_NONE if /etc/setuid-debug is accessible.
>>     (__tunables_init)[TUNABLES_FRONTEND ==
>>     TUNABLES_FRONTEND_valstring]: Update GLIBC_TUNABLES envvar
>>     after parsing.
>>     [TUNABLES_FRONTEND != TUNABLES_FRONTEND_valstring]: Erase
>>     tunable envvars of category TUNABLES_SECLEVEL_SXID_ERASE.
>>     * elf/dl-tunables.h (struct _tunable): Change member is_secure
>>     to security_level.
>>     * elf/dl-tunables.list: Add security_level annotations for all
>>     tunables.
>>     * scripts/gen-tunables.awk: Recognize and generate enum values
>>     for security_level.
>>     * elf/tst-env-setuid.c: New test case.
>>     * elf/tst-env-setuid-tunables: new test case.
>>     * elf/Makefile (tests-static): Add them.
>
> Changelog needs reference to bug 21073.

OK.  A bug is not necessary if the defect was introduced in unreleased
code btw.

> I still don't think this micro-optimization is worthwhile.  If we want
> to avoid allocations, we should avoid the copies (they would only be
> needed for strings and for variable rewriting under AT_SECURE).

OK.

> I believe this correctly rewrites the string.  But reusing the “name”
> variable in the move-forward loop is confusing.  Maybe also add a
> comment to explain what the loop is doing.

I'll add a descriptive comment that includes the reason why we're using
NAME there.

>> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
>> new file mode 100644
>> index 0000000..7def663
>> --- /dev/null
>> +++ b/elf/tst-env-setuid.c
>> @@ -0,0 +1,282 @@
>> +/* Copyright (C) 2017 Free Software Foundation, Inc.
>
>
> Copyright should start in, uh, 2012 because …
>
>> +/* Copies the executable into a restricted directory, so that we can
>> +   safely make it SGID with the TARGET group ID.  Then runs the
>> +   executable.  */
>> +static int
>> +run_executable_sgid (gid_t target)
>
> … this code looks familiar.

Yes, it is lifted from tst-secure-getenv.c.  I didn't know there was a
custom of passing on copyright dates that way.  I don't mind changing it
though.

> We should probably consolidate this functionality in support/ for 2.26.

Agreed.

Siddhesh

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

Siddhesh Poyarekar-8
Here's an updated patch based on your review.  I have gone back to
tunables_strdup and I have used an easier way to save memory - mark the
values in the original envvar string since it is not going to be in use
anyway.

Siddhesh

0001-tunables-Fix-environment-variable-processing-for-set.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

Florian Weimer-5
On 02/02/2017 08:39 AM, Siddhesh Poyarekar wrote:

> [BZ #21073]
> * elf/dl-tunable-types.h (tunable_seclevel_t): New enum.
> * elf/dl-tunables.c (tunables_strdup): Remove.
> (get_next_env): Also return the previous envp.
> (parse_tunables): Erase tunables of category
> TUNABLES_SECLEVEL_SXID_ERASE.
> (maybe_enable_malloc_check): Make MALLOC_CHECK_
> TUNABLE_SECLEVEL_NONE if /etc/setuid-debug is accessible.
> (__tunables_init)[TUNABLES_FRONTEND ==
> TUNABLES_FRONTEND_valstring]: Update GLIBC_TUNABLES envvar
> after parsing.
> [TUNABLES_FRONTEND != TUNABLES_FRONTEND_valstring]: Erase
> tunable envvars of category TUNABLES_SECLEVEL_SXID_ERASE.
> * elf/dl-tunables.h (struct _tunable): Change member is_secure
> to security_level.
> * elf/dl-tunables.list: Add security_level annotations for all
> tunables.
> * scripts/gen-tunables.awk: Recognize and generate enum values
> for security_level.
> * elf/tst-env-setuid.c: New test case.
> * elf/tst-env-setuid-tunables: new test case.
> * elf/Makefile (tests-static): Add them.

I think this is okay.  Reviewed and tested with an installed glibc.

> +  tunable_id_t id = TUNABLE_ENUM_NAME(glibc, malloc, check);

Nowadays, we use a space even after macro names, I think.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries

Siddhesh Poyarekar-9
On Thursday 02 February 2017 03:39 PM, Florian Weimer wrote:
> Nowadays, we use a space even after macro names, I think.

We do, thanks :)

Siddhesh