rtld: Reject overly long LD_AUDIT path elements
Commit Message
Also only process the last LD_AUDIT entry.
2017-06-19 Florian Weimer <fweimer@redhat.com>
* elf/rtld.c (audit_list_string): New variable.
(audit_list): Update comment.
(struct audit_list_iter): Define.
(audit_list_iter_init, audit_list_iter_next): New function.
(dl_main): Use struct audit_list_iter to process audit modules.
(process_dl_audit): Call dso_name_valid_for_suid.
(process_envvars): Set audit_list_string instead of calling
process_dl_audit.
Comments
On 06/19/2017 12:13 PM, Florian Weimer wrote:
> Also only process the last LD_AUDIT entry.
>
> 2017-06-19 Florian Weimer <fweimer@redhat.com>
>
> * elf/rtld.c (audit_list_string): New variable.
> (audit_list): Update comment.
> (struct audit_list_iter): Define.
> (audit_list_iter_init, audit_list_iter_next): New function.
> (dl_main): Use struct audit_list_iter to process audit modules.
> (process_dl_audit): Call dso_name_valid_for_suid.
> (process_envvars): Set audit_list_string instead of calling
> process_dl_audit.
The same rationale for PATH_MAX applies here as it did for the review
of the LD_LIBRARY_PATH patch.
For now I think the community should adopt a measured position for
SUID binaries, for all GNU operating systems including GNU/Hurd, and
place a small but useful limit for PATH_MAX. This value is used only
inside glibc and only for SUID binaries.
OK to checkin IMO as long as you define PATH_MAX for operating systems
that don't define it, and you would have already done that for the
earlier LD_LIBRARY_PATH patch, so this patch would be safe to use PATH_MAX.
Until we have a better understanding of how we are going to fix this with
gcc -fstack-check, we need these heuristics in upstream.
> diff --git a/elf/rtld.c b/elf/rtld.c
> index c801ee5..74147c3 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -116,13 +116,91 @@ dso_name_valid_for_suid (const char *p)
> return *p != '\0';
> }
>
> -/* List of auditing DSOs. */
> +/* LD_AUDIT variable contents. Must be processed before the
> + audit_list below. */
> +const char *audit_list_string;
> +
OK.
> +/* Cyclic list of auditing DSOs. audit_list->next is the first
> + element. */
> static struct audit_list
> {
> const char *name;
> struct audit_list *next;
> } *audit_list;
>
> +/* Iterator for audit_list_string followed by audit_list. */
> +struct audit_list_iter
> +{
> + /* Tail of audit_list_string still needing processing, or NULL. */
> + const char *audit_list_tail;
> +
> + /* The list element returned in the previous iteration. NULL before
> + the first element. */
> + struct audit_list *previous;
> +
> + /* Scratch buffer for returning a name which is part of
> + audit_list_string. */
> + char fname[PATH_MAX];
> +};
OK.
> +
> +/* Initialize an audit list iterator. */
> +static void
> +audit_list_iter_init (struct audit_list_iter *iter)
> +{
> + iter->audit_list_tail = audit_list_string;
> + iter->previous = NULL;
> +}
> +
OK.
> +/* Iterate through both audit_list_string and audit_list. */
> +static const char *
> +audit_list_iter_next (struct audit_list_iter *iter)
> +{
> + if (iter->audit_list_tail != NULL)
> + {
> + /* First iterate over audit_list_string. */
> + while (*iter->audit_list_tail != '\0')
> + {
> + /* Split audit list at colon. */
> + size_t len = strcspn (iter->audit_list_tail, ":");
> + if (len > 0 && len < PATH_MAX)
> + {
> + memcpy (iter->fname, iter->audit_list_tail, len);
> + iter->fname[len] = '\0';
> + }
> + else
> + /* Do not return this name to the caller. */
> + iter->fname[0] = '\0';
> +
> + /* Skip over the substring and the following delimiter. */
> + iter->audit_list_tail += len;
> + if (*iter->audit_list_tail == ':')
> + ++iter->audit_list_tail;
> +
> + /* If the name is valid, return it. */
> + if (dso_name_valid_for_suid (iter->fname))
> + return iter->fname;
> + /* Otherwise, wrap around and try the next name. */
> + }
> + /* Fall through to the procesing of audit_list. */
> + }
> +
> + if (iter->previous == NULL)
> + {
> + if (audit_list == NULL)
> + /* No pre-parsed audit list. */
> + return NULL;
> + /* Start of audit list. The first list element is at
> + audit_list->next (cyclic list). */
> + iter->previous = audit_list->next;
> + return iter->previous->name;
> + }
> + if (iter->previous == audit_list)
> + /* Cyclic list wrap-around. */
> + return NULL;
> + iter->previous = iter->previous->next;
> + return iter->previous->name;
> +}
OK.
> +
> #ifndef HAVE_INLINED_SYSCALLS
> /* Set nonzero during loading and initialization of executable and
> libraries, cleared before the executable's entry point runs. This
> @@ -1292,11 +1370,13 @@ of this helper program; chances are you did not intend to run this program.\n\
> GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid ();
>
> /* If we have auditing DSOs to load, do it now. */
> - if (__glibc_unlikely (audit_list != NULL))
> + bool need_security_init = true;
> + if (__glibc_unlikely (audit_list != NULL)
> + || __glibc_unlikely (audit_list_string != NULL))
OK.
> {
> - /* Iterate over all entries in the list. The order is important. */
> struct audit_ifaces *last_audit = NULL;
> - struct audit_list *al = audit_list->next;
> + struct audit_list_iter al_iter;
> + audit_list_iter_init (&al_iter);
OK.
>
> /* Since we start using the auditing DSOs right away we need to
> initialize the data structures now. */
> @@ -1307,9 +1387,14 @@ of this helper program; chances are you did not intend to run this program.\n\
> use different values (especially the pointer guard) and will
> fail later on. */
> security_init ();
> + need_security_init = false;
>
> - do
> + while (true)
> {
> + const char *name = audit_list_iter_next (&al_iter);
> + if (name == NULL)
> + break;
OK.
> +
> int tls_idx = GL(dl_tls_max_dtv_idx);
>
> /* Now it is time to determine the layout of the static TLS
> @@ -1318,7 +1403,7 @@ of this helper program; chances are you did not intend to run this program.\n\
> no DF_STATIC_TLS bit is set. The reason is that we know
> glibc will use the static model. */
> struct dlmopen_args dlmargs;
> - dlmargs.fname = al->name;
> + dlmargs.fname = name;
> dlmargs.map = NULL;
>
> const char *objname;
> @@ -1331,7 +1416,7 @@ of this helper program; chances are you did not intend to run this program.\n\
> not_loaded:
> _dl_error_printf ("\
> ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> - al->name, err_str);
> + name, err_str);
> if (malloced)
> free ((char *) err_str);
> }
> @@ -1435,10 +1520,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> goto not_loaded;
> }
> }
> -
> - al = al->next;
> }
> - while (al != audit_list->next);
>
> /* If we have any auditing modules, announce that we already
> have two objects loaded. */
> @@ -1702,7 +1784,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> if (tcbp == NULL)
> tcbp = init_tls ();
>
> - if (__glibc_likely (audit_list == NULL))
> + if (__glibc_likely (need_security_init))
OK.
> /* Initialize security features. But only if we have not done it
> earlier. */
> security_init ();
> @@ -2333,9 +2415,7 @@ process_dl_audit (char *str)
> char *p;
>
> while ((p = (strsep) (&str, ":")) != NULL)
> - if (p[0] != '\0'
> - && (__builtin_expect (! __libc_enable_secure, 1)
> - || strchr (p, '/') == NULL))
> + if (dso_name_valid_for_suid (p))
OK.
> {
> /* This is using the local malloc, not the system malloc. The
> memory can never be freed. */
> @@ -2399,7 +2479,7 @@ process_envvars (enum mode *modep)
> break;
> }
> if (memcmp (envline, "AUDIT", 5) == 0)
> - process_dl_audit (&envline[6]);
> + audit_list_string = &envline[6];
OK.
> break;
>
> case 7:
>
On Jun 19 2017, fweimer@redhat.com (Florian Weimer) wrote:
> diff --git a/elf/rtld.c b/elf/rtld.c
> index c801ee5..74147c3 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -116,13 +116,91 @@ dso_name_valid_for_suid (const char *p)
> return *p != '\0';
> }
>
> -/* List of auditing DSOs. */
> +/* LD_AUDIT variable contents. Must be processed before the
> + audit_list below. */
> +const char *audit_list_string;
> +
> +/* Cyclic list of auditing DSOs. audit_list->next is the first
> + element. */
> static struct audit_list
> {
> const char *name;
> struct audit_list *next;
> } *audit_list;
>
> +/* Iterator for audit_list_string followed by audit_list. */
> +struct audit_list_iter
> +{
> + /* Tail of audit_list_string still needing processing, or NULL. */
> + const char *audit_list_tail;
> +
> + /* The list element returned in the previous iteration. NULL before
> + the first element. */
> + struct audit_list *previous;
> +
> + /* Scratch buffer for returning a name which is part of
> + audit_list_string. */
> + char fname[PATH_MAX];
> +};
> +
> +/* Initialize an audit list iterator. */
> +static void
> +audit_list_iter_init (struct audit_list_iter *iter)
> +{
> + iter->audit_list_tail = audit_list_string;
> + iter->previous = NULL;
> +}
> +
> +/* Iterate through both audit_list_string and audit_list. */
> +static const char *
> +audit_list_iter_next (struct audit_list_iter *iter)
> +{
> + if (iter->audit_list_tail != NULL)
> + {
> + /* First iterate over audit_list_string. */
> + while (*iter->audit_list_tail != '\0')
> + {
> + /* Split audit list at colon. */
> + size_t len = strcspn (iter->audit_list_tail, ":");
> + if (len > 0 && len < PATH_MAX)
> + {
> + memcpy (iter->fname, iter->audit_list_tail, len);
> + iter->fname[len] = '\0';
> + }
> + else
> + /* Do not return this name to the caller. */
> + iter->fname[0] = '\0';
> +
> + /* Skip over the substring and the following delimiter. */
> + iter->audit_list_tail += len;
> + if (*iter->audit_list_tail == ':')
> + ++iter->audit_list_tail;
> +
> + /* If the name is valid, return it. */
> + if (dso_name_valid_for_suid (iter->fname))
> + return iter->fname;
> + /* Otherwise, wrap around and try the next name. */
> + }
> + /* Fall through to the procesing of audit_list. */
> + }
> +
> + if (iter->previous == NULL)
> + {
> + if (audit_list == NULL)
> + /* No pre-parsed audit list. */
> + return NULL;
> + /* Start of audit list. The first list element is at
> + audit_list->next (cyclic list). */
> + iter->previous = audit_list->next;
> + return iter->previous->name;
> + }
> + if (iter->previous == audit_list)
> + /* Cyclic list wrap-around. */
> + return NULL;
> + iter->previous = iter->previous->next;
> + return iter->previous->name;
> +}
> +
Why do you need all that complexity?
Andreas.
On 06/26/2017 10:47 AM, Andreas Schwab wrote:
> Why do you need all that complexity?
Do you mean the external iterator instead of just writing a loop?
I did this so that the patch can be backported without changes.
Otherwise, we would have to reindent the audit module process loop (or
move it to separate function for clarity), and this will introduce a lot
of conflicts because there have been quite a few trivial changes to the
loop body.
Does this answer your question?
Thanks,
Florian
On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/26/2017 10:47 AM, Andreas Schwab wrote:
>
>> Why do you need all that complexity?
>
> Do you mean the external iterator instead of just writing a loop?
>
> I did this so that the patch can be backported without changes.
> Otherwise, we would have to reindent the audit module process loop (or
> move it to separate function for clarity), and this will introduce a lot
> of conflicts because there have been quite a few trivial changes to the
> loop body.
That's not a good reason for overly complex code.
Andreas.
On 06/26/2017 11:40 AM, Andreas Schwab wrote:
> On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
>
>> On 06/26/2017 10:47 AM, Andreas Schwab wrote:
>>
>>> Why do you need all that complexity?
>>
>> Do you mean the external iterator instead of just writing a loop?
>>
>> I did this so that the patch can be backported without changes.
>> Otherwise, we would have to reindent the audit module process loop (or
>> move it to separate function for clarity), and this will introduce a lot
>> of conflicts because there have been quite a few trivial changes to the
>> loop body.
>
> That's not a good reason for overly complex code.
I had hoped to clean this up after the new dl-minimal malloc went in,
but I can do so now if you want that.
Thanks,
Florian
On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
> I had hoped to clean this up after the new dl-minimal malloc went in,
> but I can do so now if you want that.
Please do.
Andreas.
@@ -116,13 +116,91 @@ dso_name_valid_for_suid (const char *p)
return *p != '\0';
}
-/* List of auditing DSOs. */
+/* LD_AUDIT variable contents. Must be processed before the
+ audit_list below. */
+const char *audit_list_string;
+
+/* Cyclic list of auditing DSOs. audit_list->next is the first
+ element. */
static struct audit_list
{
const char *name;
struct audit_list *next;
} *audit_list;
+/* Iterator for audit_list_string followed by audit_list. */
+struct audit_list_iter
+{
+ /* Tail of audit_list_string still needing processing, or NULL. */
+ const char *audit_list_tail;
+
+ /* The list element returned in the previous iteration. NULL before
+ the first element. */
+ struct audit_list *previous;
+
+ /* Scratch buffer for returning a name which is part of
+ audit_list_string. */
+ char fname[PATH_MAX];
+};
+
+/* Initialize an audit list iterator. */
+static void
+audit_list_iter_init (struct audit_list_iter *iter)
+{
+ iter->audit_list_tail = audit_list_string;
+ iter->previous = NULL;
+}
+
+/* Iterate through both audit_list_string and audit_list. */
+static const char *
+audit_list_iter_next (struct audit_list_iter *iter)
+{
+ if (iter->audit_list_tail != NULL)
+ {
+ /* First iterate over audit_list_string. */
+ while (*iter->audit_list_tail != '\0')
+ {
+ /* Split audit list at colon. */
+ size_t len = strcspn (iter->audit_list_tail, ":");
+ if (len > 0 && len < PATH_MAX)
+ {
+ memcpy (iter->fname, iter->audit_list_tail, len);
+ iter->fname[len] = '\0';
+ }
+ else
+ /* Do not return this name to the caller. */
+ iter->fname[0] = '\0';
+
+ /* Skip over the substring and the following delimiter. */
+ iter->audit_list_tail += len;
+ if (*iter->audit_list_tail == ':')
+ ++iter->audit_list_tail;
+
+ /* If the name is valid, return it. */
+ if (dso_name_valid_for_suid (iter->fname))
+ return iter->fname;
+ /* Otherwise, wrap around and try the next name. */
+ }
+ /* Fall through to the procesing of audit_list. */
+ }
+
+ if (iter->previous == NULL)
+ {
+ if (audit_list == NULL)
+ /* No pre-parsed audit list. */
+ return NULL;
+ /* Start of audit list. The first list element is at
+ audit_list->next (cyclic list). */
+ iter->previous = audit_list->next;
+ return iter->previous->name;
+ }
+ if (iter->previous == audit_list)
+ /* Cyclic list wrap-around. */
+ return NULL;
+ iter->previous = iter->previous->next;
+ return iter->previous->name;
+}
+
#ifndef HAVE_INLINED_SYSCALLS
/* Set nonzero during loading and initialization of executable and
libraries, cleared before the executable's entry point runs. This
@@ -1292,11 +1370,13 @@ of this helper program; chances are you did not intend to run this program.\n\
GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid ();
/* If we have auditing DSOs to load, do it now. */
- if (__glibc_unlikely (audit_list != NULL))
+ bool need_security_init = true;
+ if (__glibc_unlikely (audit_list != NULL)
+ || __glibc_unlikely (audit_list_string != NULL))
{
- /* Iterate over all entries in the list. The order is important. */
struct audit_ifaces *last_audit = NULL;
- struct audit_list *al = audit_list->next;
+ struct audit_list_iter al_iter;
+ audit_list_iter_init (&al_iter);
/* Since we start using the auditing DSOs right away we need to
initialize the data structures now. */
@@ -1307,9 +1387,14 @@ of this helper program; chances are you did not intend to run this program.\n\
use different values (especially the pointer guard) and will
fail later on. */
security_init ();
+ need_security_init = false;
- do
+ while (true)
{
+ const char *name = audit_list_iter_next (&al_iter);
+ if (name == NULL)
+ break;
+
int tls_idx = GL(dl_tls_max_dtv_idx);
/* Now it is time to determine the layout of the static TLS
@@ -1318,7 +1403,7 @@ of this helper program; chances are you did not intend to run this program.\n\
no DF_STATIC_TLS bit is set. The reason is that we know
glibc will use the static model. */
struct dlmopen_args dlmargs;
- dlmargs.fname = al->name;
+ dlmargs.fname = name;
dlmargs.map = NULL;
const char *objname;
@@ -1331,7 +1416,7 @@ of this helper program; chances are you did not intend to run this program.\n\
not_loaded:
_dl_error_printf ("\
ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
- al->name, err_str);
+ name, err_str);
if (malloced)
free ((char *) err_str);
}
@@ -1435,10 +1520,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
goto not_loaded;
}
}
-
- al = al->next;
}
- while (al != audit_list->next);
/* If we have any auditing modules, announce that we already
have two objects loaded. */
@@ -1702,7 +1784,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
if (tcbp == NULL)
tcbp = init_tls ();
- if (__glibc_likely (audit_list == NULL))
+ if (__glibc_likely (need_security_init))
/* Initialize security features. But only if we have not done it
earlier. */
security_init ();
@@ -2333,9 +2415,7 @@ process_dl_audit (char *str)
char *p;
while ((p = (strsep) (&str, ":")) != NULL)
- if (p[0] != '\0'
- && (__builtin_expect (! __libc_enable_secure, 1)
- || strchr (p, '/') == NULL))
+ if (dso_name_valid_for_suid (p))
{
/* This is using the local malloc, not the system malloc. The
memory can never be freed. */
@@ -2399,7 +2479,7 @@ process_envvars (enum mode *modep)
break;
}
if (memcmp (envline, "AUDIT", 5) == 0)
- process_dl_audit (&envline[6]);
+ audit_list_string = &envline[6];
break;
case 7: