[2/2] Erase GLIBC_TUNABLES for setxid processes when tunables is disabled
Commit Message
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.
In addition to this, I'll post a patch for earlier releases (2.24 and
older) to add GLIBC_TUNABLES to unsecure_envvars (is unsecure even a
word?) so that they too don't end up passing on unsafe tunables.
* elf/dl-tunables.c (GLIBC_TUNABLES): Define as
TUNABLES_VALSTRING_ENVVAR.
(__tunables_init): Call tunables_delete_env.
(is_name): Move definition...
* elf/dl-tunables.h: ... here.
(TUNABLES_VALSTRING_ENVVAR): New macro.
(tunables_delete_env): New function.
[!HAVE_TUNABLES](__tunables_init): Call it.
---
elf/dl-tunables.c | 40 +++++-----------------------------------
elf/dl-tunables.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 52 insertions(+), 37 deletions(-)
Comments
On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
> In addition to this, I'll post a patch for earlier releases (2.24 and
> older) to add GLIBC_TUNABLES to unsecure_envvars (is unsecure even a
> word?) so that they too don't end up passing on unsafe tunables.
I expected a patch with a preprocessor condition for unsecvars.h for
glibc 2.25. Wouldn't this have the same effect than your tunables-based
changes?
Thanks,
Florian
On Tuesday 31 January 2017 06:52 PM, Florian Weimer wrote:
> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>> In addition to this, I'll post a patch for earlier releases (2.24 and
>> older) to add GLIBC_TUNABLES to unsecure_envvars (is unsecure even a
>> word?) so that they too don't end up passing on unsafe tunables.
>
> I expected a patch with a preprocessor condition for unsecvars.h for
> glibc 2.25. Wouldn't this have the same effect than your tunables-based
> changes?
It would, but I chose to limit the change to within the tunables code
base. I can do it your way if that's your preference.
Siddhesh
On 01/31/2017 02:29 PM, Siddhesh Poyarekar wrote:
> On Tuesday 31 January 2017 06:52 PM, Florian Weimer wrote:
>> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>>> In addition to this, I'll post a patch for earlier releases (2.24 and
>>> older) to add GLIBC_TUNABLES to unsecure_envvars (is unsecure even a
>>> word?) so that they too don't end up passing on unsafe tunables.
>>
>> I expected a patch with a preprocessor condition for unsecvars.h for
>> glibc 2.25. Wouldn't this have the same effect than your tunables-based
>> changes?
>
> It would, but I chose to limit the change to within the tunables code
> base. I can do it your way if that's your preference.
I'd suggest to keep the changes to a non-tunables configuration to a
minimum, and unsecvars.h is better at that goal, I think.
Thanks,
Florian
On Tuesday 31 January 2017 08:58 PM, Florian Weimer wrote:
> I'd suggest to keep the changes to a non-tunables configuration to a
> minimum, and unsecvars.h is better at that goal, I think.
Fair enough. I have to step out now, so I'll post a modified version of
the 2.24 patch for the 2/2 tomorrow. What do you think about 1/2?
Siddhesh
@@ -31,25 +31,9 @@
#include "dl-tunables.h"
#if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
-# define GLIBC_TUNABLES "GLIBC_TUNABLES"
+# define GLIBC_TUNABLES TUNABLES_VALSTRING_ENVVAR
#endif
-/* Compare environment or tunable names, bounded by the name hardcoded in
- glibc. */
-static bool
-is_name (const char *orig, const char *envname)
-{
- for (;*orig != '\0' && *envname != '\0'; envname++, orig++)
- if (*orig != *envname)
- break;
-
- /* The ENVNAME is immediately followed by a value. */
- if (*orig == '\0' && *envname == '=')
- return true;
- else
- return false;
-}
-
static char **
get_next_env (char **envp, char **name, size_t *namelen, char **val,
char ***prev_envp)
@@ -405,24 +389,10 @@ __tunables_init (char **envp)
{
if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
{
- /* Erase the environment variable. */
- char **ep = prev_envp;
-
- while (*ep != NULL)
- {
- if (is_name (name, *ep))
- {
- char **dp = ep;
-
- do
- dp[0] = dp[1];
- while (*dp++);
- }
- else
- ++ep;
- }
- /* Reset the iterator so that we read the environment again
- from the point we erased. */
+ /* Erase the environment variable and then reset the
+ iterator so that we read the environment again from
+ the point we erased. */
+ tunables_delete_env (name, prev_envp);
envp = prev_envp;
}
@@ -21,12 +21,57 @@
#ifndef _TUNABLES_H_
#define _TUNABLES_H_
+#define TUNABLES_VALSTRING_ENVVAR "GLIBC_TUNABLES"
+
+/* Compare environment or tunable names, bounded by the name hardcoded in
+ glibc. */
+static inline bool
+__always_inline
+is_name (const char *orig, const char *envname)
+{
+ for (;*orig != '\0' && *envname != '\0'; envname++, orig++)
+ if (*orig != *envname)
+ break;
+
+ /* The ENVNAME is immediately followed by a value. */
+ if (*orig == '\0' && *envname == '=')
+ return true;
+ else
+ return false;
+}
+
+static inline void
+__always_inline
+tunables_delete_env (const char *name, char **envp)
+{
+ if (envp == NULL)
+ return;
+
+ while (*envp != NULL)
+ {
+ if (is_name (name, *envp))
+ {
+ char **dp = envp;
+
+ do
+ dp[0] = dp[1];
+ while (*dp++);
+ }
+ else
+ ++envp;
+ }
+}
+
#if !HAVE_TUNABLES
static inline void
__always_inline
-__tunables_init (char **unused __attribute__ ((unused)))
+__tunables_init (char **envp)
{
- /* This is optimized out if tunables are not enabled. */
+ /* If tunables is not enabled, we want to make sure that we don't pass on
+ insecure tunables to child processes of setxid processes, so just drop
+ GLIBC_TUNABLES from the environment. */
+ if (__libc_enable_secure)
+ tunables_delete_env (TUNABLES_VALSTRING_ENVVAR, envp);
}
#else