From patchwork Thu Jun 29 19:05:52 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 782414 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wz8Jc27blz9s82 for ; Fri, 30 Jun 2017 05:06:07 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="FwY7ZtDm"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type; q=dns; s=default; b=QQ0o bDoO9idu6Kt6KShCzyoZuzs4Btd/fUdxbCTD22lsvXIfbi+Ingpyq9uTu1qlX7bG 3Ziytr70u61lz20Ci0mic89yjKfN05PU6X2DTxNAxviOi6WID+a4/za1te4s9te+ l2y4BOVNpUFMfhIhcDrcuBxauE66vJBZFtZjZSk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:cc:references:from:message-id:date :mime-version:in-reply-to:content-type; s=default; bh=JSqj6KZAhd p3+XDY7Kf+nqp08cQ=; b=FwY7ZtDmuVcx24D/Cni2Ebi8FjYC+R+l3kUye8CHOz SkWh7IPkbNj0OeNvxNvDSWjZ3ZI/cSvoGOebcQbmtd51N1W7NL3to8U6ZErEg/K1 sQpIMyv5Vei5zD/zu4rfyxL0F0BMGVQv1Mo0jhEBaeIP54D/YE1HjSRFoKgrFiHS M= Received: (qmail 61993 invoked by alias); 29 Jun 2017 19:05:59 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 61978 invoked by uid 89); 29 Jun 2017 19:05:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Tail X-HELO: mx1.redhat.com DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6111E32A880 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=fweimer@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6111E32A880 Subject: Re: [PATCH] rtld: Reject overly long LD_AUDIT path elements To: Andreas Schwab Cc: libc-alpha@sourceware.org References: <20170619161345.7CC73402AEC3E@oldenburg.str.redhat.com> <54c5074e-8b7c-7b3b-e5b1-02ee18a7cabe@redhat.com> <50beae35-82bd-ae09-064d-676fd26fc1c0@redhat.com> <6701f70e-f623-ef1a-1ef2-a4d47d7e11b0@redhat.com> <6fb6263e-6653-8f87-2aa6-e2b32437be94@redhat.com> From: Florian Weimer Message-ID: <34b57a2e-356b-3a92-1403-e6d16529b0eb@redhat.com> Date: Thu, 29 Jun 2017 21:05:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: On 06/26/2017 02:57 PM, Andreas Schwab wrote: > On Jun 26 2017, Florian Weimer wrote: > >> The goal is to prevent massaging the heap through LD_AUDIT variable >> contents. So it's purely hardening. > > Why is that needed? I'm not sure if it is needed. I am not an experienced exploit writer. I assume you want me to apply something like the attached patch, right? Thanks, Florian ld.so: Simplify processing of LD_AUDIT Return to the old scheme of calling process_dl_audit and allocating the audit list on the heap, but process the last environment variable only. This reverts most of commit 81b82fb966ffbd94353f793ad17116c6088dedd9. 2017-06-29 Florian Weimer * elf/rtld.c (process_dl_audit): Call dso_name_valid_for_suid. (audit_list_string): Remove variable. (struct audit_list_iter): Remove definition. (audit_list_iter_init, audit_list_iter_next): Remove functions. (dl_main): Remove iterator code and iterate over audit_list directly. (process_envvars): Process LD_AUDIT once using process_dl_audit. diff --git a/elf/rtld.c b/elf/rtld.c index 65647fb..3898257 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -129,10 +129,6 @@ 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 @@ -141,79 +137,6 @@ static struct audit_list 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[SECURE_NAME_LIMIT]; -}; - -/* 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 < sizeof (iter->fname)) - { - 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 @@ -1383,13 +1306,11 @@ 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. */ - bool need_security_init = true; - if (__glibc_unlikely (audit_list != NULL) - || __glibc_unlikely (audit_list_string != NULL)) + if (__glibc_unlikely (audit_list != NULL)) { + /* Iterate over all entries in the list. The order is important. */ struct audit_ifaces *last_audit = NULL; - struct audit_list_iter al_iter; - audit_list_iter_init (&al_iter); + struct audit_list *al = audit_list->next; /* Since we start using the auditing DSOs right away we need to initialize the data structures now. */ @@ -1400,14 +1321,9 @@ 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; - while (true) + do { - 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 @@ -1416,7 +1332,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 = name; + dlmargs.fname = al->name; dlmargs.map = NULL; const char *objname; @@ -1429,7 +1345,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", - name, err_str); + al->name, err_str); if (malloced) free ((char *) err_str); } @@ -1533,7 +1449,10 @@ 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. */ @@ -1797,7 +1716,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n", if (tcbp == NULL) tcbp = init_tls (); - if (__glibc_likely (need_security_init)) + if (__glibc_likely (audit_list == NULL)) /* Initialize security features. But only if we have not done it earlier. */ security_init (); @@ -2458,6 +2377,7 @@ process_envvars (enum mode *modep) char *envline; enum mode mode = normal; char *debug_output = NULL; + char *audit_list_string = NULL; /* This is the default place for profiling data file. */ GLRO(dl_profile_output) @@ -2623,6 +2543,9 @@ process_envvars (enum mode *modep) /* The caller wants this information. */ *modep = mode; + if (audit_list_string != NULL) + process_dl_audit (audit_list_string); + /* Extra security for SUID binaries. Remove all dangerous environment variables. */ if (__builtin_expect (__libc_enable_secure, 0))