From patchwork Fri Apr 3 12:02:58 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 38721 Return-Path: X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from us-smtp-delivery-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.81]) by sourceware.org (Postfix) with ESMTP id 39CB0385B833 for ; Fri, 3 Apr 2020 12:03:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 39CB0385B833 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-473-b0eo8aoaOhCnVoIHIi5A4w-1; Fri, 03 Apr 2020 08:03:04 -0400 X-MC-Unique: b0eo8aoaOhCnVoIHIi5A4w-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 13421802C80 for ; Fri, 3 Apr 2020 12:03:01 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-113-15.ams2.redhat.com [10.36.113.15]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 06ACE1147F3 for ; Fri, 3 Apr 2020 12:02:59 +0000 (UTC) From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH 2/3] elf: Simplify handling of lists of audit strings In-Reply-To: References: Message-Id: <044c8e2c7af16bf2f48bc3d740fc5b5b14df8009.1585914979.git.fweimer@redhat.com> Date: Fri, 03 Apr 2020 14:02:58 +0200 User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-26.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Apr 2020 12:03:09 -0000 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 --- elf/rtld.c | 230 ++++++++++++++++++++++++++--------------------------- 1 file changed, 113 insertions(+), 117 deletions(-) diff --git a/elf/rtld.c b/elf/rtld.c index 51dfaf966a..fe4dfbec67 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -45,6 +45,7 @@ #include #include #include +#include #include @@ -109,8 +110,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 }; @@ -118,7 +164,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; @@ -146,19 +192,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. */ @@ -176,89 +209,76 @@ 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; + /* Grow the array if necessary. */ + if (list->length == array_length (list->audit_strings)) + _dl_fatal_printf ("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; } #ifndef HAVE_INLINED_SYSCALLS @@ -1062,15 +1082,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); @@ -1102,6 +1120,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 \ @@ -1115,7 +1136,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); #ifndef HAVE_INLINED_SYSCALLS /* Set up a flag which tells we are just starting. */ @@ -1189,7 +1210,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; @@ -1619,8 +1640,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. */ @@ -1633,7 +1653,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 @@ -2507,30 +2527,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. */ @@ -2538,7 +2534,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; @@ -2578,7 +2574,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: