commit 4c6e0415ef206a595c62d5d37e3b9a821782c533 Author: Florian Weimer Date: Fri Apr 3 13:17:48 2020 +0200 elf: Simplify handling of lists of audit strings All list elements are colon-separated strings, and there is a hard upper limit for the number of audit modules, so it is possible to pre-allocate a fixed-size array of strings to which the LD_AUDIT environment variable and --audit arguments are added. Also eliminate the global variables for the audit list because the list is only needed briefly during startup. There is a slight behavior change: All duplicate LD_AUDIT environment variables are now processed, not just the last one as before. However, such environment vectors are invalid anyway. Reviewed-by: Carlos O'Donell diff --git a/elf/rtld.c b/elf/rtld.c index f755dc30331f799f..c39cb8f2cd4bb1cc 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -43,6 +43,7 @@ #include #include #include +#include #include @@ -107,8 +108,53 @@ static void print_missing_version (int errcode, const char *objname, /* Print the various times we collected. */ static void print_statistics (const hp_timing_t *total_timep); -/* Add audit objects. */ -static void process_dl_audit (char *str); +/* Length limits for names and paths, to protect the dynamic linker, + particularly when __libc_enable_secure is active. */ +#ifdef NAME_MAX +# define SECURE_NAME_LIMIT NAME_MAX +#else +# define SECURE_NAME_LIMIT 255 +#endif +#ifdef PATH_MAX +# define SECURE_PATH_LIMIT PATH_MAX +#else +# define SECURE_PATH_LIMIT 1024 +#endif + +/* Strings containing colon-separated lists of audit modules. */ +struct audit_list +{ + /* Array of strings containing colon-separated path lists. Each + audit module needs its own namespace, so pre-allocate the largest + possible list. */ + const char *audit_strings[DL_NNS]; + + /* Number of entries added to audit_strings. */ + size_t length; + + /* Index into the audit_strings array (for the iteration phase). */ + size_t current_index; + + /* Tail of audit_strings[current_index] which still needs + processing. */ + const char *current_tail; + + /* Scratch buffer for returning a name which is part of the strings + in audit_strings. */ + char fname[SECURE_NAME_LIMIT]; +}; + +/* Creates an empty audit list. */ +static void audit_list_init (struct audit_list *); + +/* Add a string to the end of the audit list, for later parsing. Must + not be called after audit_list_next. */ +static void audit_list_add_string (struct audit_list *, const char *); + +/* Extract the next audit module from the audit list. Only modules + for which dso_name_valid_for_suid is true are returned. Must be + called after all the audit_list_add_string calls. */ +static const char *audit_list_next (struct audit_list *); /* This is a list of all the modes the dynamic loader can be in. */ enum mode { normal, list, verify, trace }; @@ -116,7 +162,7 @@ enum mode { normal, list, verify, trace }; /* Process all environments variables the dynamic linker must recognize. Since all of them start with `LD_' we are a bit smarter while finding all the entries. */ -static void process_envvars (enum mode *modep); +static void process_envvars (enum mode *modep, struct audit_list *); #ifdef DL_ARGV_NOT_RELRO int _dl_argc attribute_hidden; @@ -144,19 +190,6 @@ uintptr_t __pointer_chk_guard_local strong_alias (__pointer_chk_guard_local, __pointer_chk_guard) #endif -/* Length limits for names and paths, to protect the dynamic linker, - particularly when __libc_enable_secure is active. */ -#ifdef NAME_MAX -# define SECURE_NAME_LIMIT NAME_MAX -#else -# define SECURE_NAME_LIMIT 255 -#endif -#ifdef PATH_MAX -# define SECURE_PATH_LIMIT PATH_MAX -#else -# define SECURE_PATH_LIMIT 1024 -#endif - /* Check that AT_SECURE=0, or that the passed name does not contain directories and is not overly long. Reject empty names unconditionally. */ @@ -174,89 +207,75 @@ dso_name_valid_for_suid (const char *p) return *p != '\0'; } -/* 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 +static void +audit_list_init (struct audit_list *list) { - const char *name; - struct audit_list *next; -} *audit_list; + list->length = 0; + list->current_index = 0; + list->current_tail = NULL; +} -/* Iterator for audit_list_string followed by audit_list. */ -struct audit_list_iter +static void +audit_list_add_string (struct audit_list *list, const char *string) { - /* Tail of audit_list_string still needing processing, or NULL. */ - const char *audit_list_tail; + /* Empty strings do not load anything. */ + if (*string == '\0') + return; - /* The list element returned in the previous iteration. NULL before - the first element. */ - struct audit_list *previous; + if (list->length == array_length (list->audit_strings)) + _dl_fatal_printf ("Fatal glibc error: Too many audit modules requested\n"); - /* Scratch buffer for returning a name which is part of - audit_list_string. */ - char fname[SECURE_NAME_LIMIT]; -}; + list->audit_strings[list->length++] = string; -/* 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; + /* Initialize processing of the first string for + audit_list_next. */ + if (list->length == 1) + list->current_tail = string; } -/* Iterate through both audit_list_string and audit_list. */ static const char * -audit_list_iter_next (struct audit_list_iter *iter) +audit_list_next (struct audit_list *list) { - if (iter->audit_list_tail != NULL) + if (list->current_tail == NULL) + return NULL; + + while (true) { - /* First iterate over audit_list_string. */ - while (*iter->audit_list_tail != '\0') + /* Advance to the next string in audit_strings if the current + string has been exhausted. */ + while (*list->current_tail == '\0') { - /* Split audit list at colon. */ - size_t len = strcspn (iter->audit_list_tail, ":"); - if (len > 0 && len < sizeof (iter->fname)) + ++list->current_index; + if (list->current_index == list->length) { - memcpy (iter->fname, iter->audit_list_tail, len); - iter->fname[len] = '\0'; + list->current_tail = NULL; + return NULL; } - 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. */ + list->current_tail = list->audit_strings[list->current_index]; } - /* 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; + /* Split the in-string audit list at the next colon colon. */ + size_t len = strcspn (list->current_tail, ":"); + if (len > 0 && len < sizeof (list->fname)) + { + memcpy (list->fname, list->current_tail, len); + list->fname[len] = '\0'; + } + else + /* Mark the name as unusable for dso_name_valid_for_suid. */ + list->fname[0] = '\0'; + + /* Skip over the substring and the following delimiter. */ + list->current_tail += len; + if (*list->current_tail == ':') + ++list->current_tail; + + /* If the name is valid, return it. */ + if (dso_name_valid_for_suid (list->fname)) + return list->fname; + + /* Otherwise wrap around to find the next list element. . */ } - if (iter->previous == audit_list) - /* Cyclic list wrap-around. */ - return NULL; - iter->previous = iter->previous->next; - return iter->previous->name; } /* Set nonzero during loading and initialization of executable and @@ -1060,15 +1079,13 @@ notify_audit_modules_of_loaded_object (struct link_map *map) /* Load all audit modules. */ static void -load_audit_modules (struct link_map *main_map) +load_audit_modules (struct link_map *main_map, struct audit_list *audit_list) { struct audit_ifaces *last_audit = NULL; - struct audit_list_iter al_iter; - audit_list_iter_init (&al_iter); while (true) { - const char *name = audit_list_iter_next (&al_iter); + const char *name = audit_list_next (audit_list); if (name == NULL) break; load_audit_module (name, &last_audit); @@ -1100,6 +1117,9 @@ dl_main (const ElfW(Phdr) *phdr, bool rtld_is_main = false; void *tcbp = NULL; + struct audit_list audit_list; + audit_list_init (&audit_list); + GL(dl_init_static_tls) = &_dl_nothread_init_static_tls; #if defined SHARED && defined _LIBC_REENTRANT \ @@ -1113,7 +1133,7 @@ dl_main (const ElfW(Phdr) *phdr, GL(dl_make_stack_executable_hook) = &_dl_make_stack_executable; /* Process the environment variable which control the behaviour. */ - process_envvars (&mode); + process_envvars (&mode, &audit_list); /* Set up a flag which tells we are just starting. */ _dl_starting_up = 1; @@ -1185,7 +1205,7 @@ dl_main (const ElfW(Phdr) *phdr, } else if (! strcmp (_dl_argv[1], "--audit") && _dl_argc > 2) { - process_dl_audit (_dl_argv[2]); + audit_list_add_string (&audit_list, _dl_argv[2]); _dl_skip_args += 2; _dl_argc -= 2; @@ -1612,8 +1632,7 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]); /* If we have auditing DSOs to load, do it now. */ bool need_security_init = true; - if (__glibc_unlikely (audit_list != NULL) - || __glibc_unlikely (audit_list_string != NULL)) + if (audit_list.length > 0) { /* Since we start using the auditing DSOs right away we need to initialize the data structures now. */ @@ -1626,7 +1645,7 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]); security_init (); need_security_init = false; - load_audit_modules (main_map); + load_audit_modules (main_map, &audit_list); } /* Keep track of the currently loaded modules to count how many @@ -2500,30 +2519,6 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n"); } } -static void -process_dl_audit (char *str) -{ - /* The parameter is a colon separated list of DSO names. */ - char *p; - - while ((p = (strsep) (&str, ":")) != NULL) - if (dso_name_valid_for_suid (p)) - { - /* This is using the local malloc, not the system malloc. The - memory can never be freed. */ - struct audit_list *newp = malloc (sizeof (*newp)); - newp->name = p; - - if (audit_list == NULL) - audit_list = newp->next = newp; - else - { - newp->next = audit_list->next; - audit_list = audit_list->next = newp; - } - } -} - /* Process all environments variables the dynamic linker must recognize. Since all of them start with `LD_' we are a bit smarter while finding all the entries. */ @@ -2531,7 +2526,7 @@ extern char **_environ attribute_hidden; static void -process_envvars (enum mode *modep) +process_envvars (enum mode *modep, struct audit_list *audit_list) { char **runp = _environ; char *envline; @@ -2571,7 +2566,7 @@ process_envvars (enum mode *modep) break; } if (memcmp (envline, "AUDIT", 5) == 0) - audit_list_string = &envline[6]; + audit_list_add_string (audit_list, &envline[6]); break; case 7: