This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] rtld: Reject overly long LD_AUDIT path elements
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Florian Weimer <fweimer at redhat dot com>, libc-alpha at sourceware dot org
- Date: Mon, 19 Jun 2017 16:04:40 -0400
- Subject: Re: [PATCH] rtld: Reject overly long LD_AUDIT path elements
- Authentication-results: sourceware.org; auth=none
- References: <20170619161345.7CC73402AEC3E@oldenburg.str.redhat.com>
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:
>
--
Cheers,
Carlos.