From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id A67E13857823 for ; Tue, 6 Apr 2021 19:47:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A67E13857823 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-414-WlikXVS3PWqF0arjwoCRTA-1; Tue, 06 Apr 2021 15:47:46 -0400 X-MC-Unique: WlikXVS3PWqF0arjwoCRTA-1 Received: by mail-qt1-f199.google.com with SMTP id t3so10793231qtp.23 for ; Tue, 06 Apr 2021 12:47:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=1MeDFmg7tZ74mhCQqu2a24/JQCoLDv3IyGDH2YMq9W0=; b=oCjcXe3G+utrZp5C1uwFq8F7oaxda7Eq2DKoSxvbD2KU34e9ztEgt4LOSnoSbnsvqq zTDrGIWi/kHDveYjKfmalQnhMR5I+Rb9LCmNEuQ7s9yhjQ3xA27PSF0dIyo828U1S8KQ TdHeX418nLxSC1FACmg9SdVEllUVEAFWCUYfB1nf34muIVTEN1AD6PXhE477vO3f9EA7 dJekJSXAhWw9k6uve7moi4/T2Iap0kAwJ8Tzh5EFCh46kMyBFt6nIQe7IZ8nqx/NttB2 D/J3MkaoqX5UXIRP4eedA6Tejqx76Kd8PkvnHVdgA7WTBWOYdRYy5Jjs4d8sy59bYRQB kVYQ== X-Gm-Message-State: AOAM531RBe6y2eFm3F5eWg786qS/sOodZtiMYM03LNgI8X5UtLZs3l57 iZa5t3/PW7fyml0aSgdEQ6Cjb+QXVGj5VhqcPSBFvqFKWNAcX3qjDuGVdEhx3mGucEwm7A6J42a Y3Hn+00w93XNmVqZrESTNOUJcnQolqqAfCWivgwmRL8PFp3tLZfS1hiNYf7Yt4cYL0MsuNg== X-Received: by 2002:ae9:f80e:: with SMTP id x14mr32194341qkh.1.1617738465426; Tue, 06 Apr 2021 12:47:45 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzWhHirBBXEYy2A07YKcvzuKgmsRwDb0igueyKSm4Zy+ks3X1dKgo2995YYqCaOfvWg15t9qA== X-Received: by 2002:ae9:f80e:: with SMTP id x14mr32194317qkh.1.1617738465118; Tue, 06 Apr 2021 12:47:45 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id 73sm16595630qkk.131.2021.04.06.12.47.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 06 Apr 2021 12:47:44 -0700 (PDT) Subject: Re: [PATCH 4/4] Fix SXID_ERASE behavior in setuid programs (BZ #27471) To: Siddhesh Poyarekar , libc-alpha@sourceware.org References: <20210316070755.330084-1-siddhesh@sourceware.org> <20210316070755.330084-5-siddhesh@sourceware.org> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Tue, 6 Apr 2021 15:47:43 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210316070755.330084-5-siddhesh@sourceware.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Tue, 06 Apr 2021 19:47:49 -0000 On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote: > When parse_tunables tries to erase a tunable marked as SXID_ERASE for > setuid programs, it ends up setting the envvar string iterator > incorrectly, because of which it may parse the next tunable > incorrectly. Given that currently the implementation allows malformed > and unrecognized tunables pass through, it may even allow SXID_ERASE > tunables to go through. I have read through bug 27471, thanks for referencing the bug. I'm worried about the fact that we have existing examples from DJ's nsswitch changes with / changing, that there are real-world examples where we actively run a child process in a potentially newer glibc and stripping away tunables for the current hardware means the child process needs a way to add it back, and doing that isn't entirely clear e.g. sudo with hardcoded env. This makes testing and producing tuning difficult or impossible and I that means to me that we should be more conservative with what we process. > This change revamps the SXID_ERASE implementation so that: > > - Only valid tunables are written back to the tunestr string, because > of which children of SXID programs will only inherit a clean list of > identified tunables that are not SXID_ERASE. This prevents passing a tunable through a program to a child that runs with a newer glibc and accepts different tunables. > - Unrecognized tunables get scrubbed off from the environment and > subsequently from the child environment. I think we need to be conservative here and allow unknown tunables to be "passed through" to the child without modification. > - This has the side-effect that a tunable that is not identified by > the setxid binary, will not be passed on to a non-setxid child even > if the child could have identified that tunable. This may break > applications that expect this behaviour but expecting such tunables > to cross the SXID boundary is wrong. Why is it wrong? We can have instances where we chroot or unshare into another glibc version and we may want the env vars to pass through because they are related to hardware changes. If I step back and look at the bigger picture I see the following: (1) Tunables we have removed. - Did we record these? - Did we record their security setting? - Are we avoiding reusing them by accident? - Are we cleaning legacy tunables based on their security setting to keep the environment clean for a child running with an old glibc? (2) Tunables we don't know about. - The child process may need to consume these because it runs a newer glibc. - We lack /var/glibc.tunables handling of these tunables, or the child may be a generic container, or generic chroot, maybe env vars are the only way to pass this information. (3) Tunables we know about. - These are the straight forward fix, we just fix the code. The patch handles (1) and (2) and (3) but picks an easier to audit and verify solution for (3) at the expense of (1) and (2). I don't know if that is a good tradeoff. With my developer hat on I expect the following: - All old legacy tunables are handled as their old security settings. - All new tunables are handled as their current security settings. - Unknown tunables are left alone. Have I justified my expectation? > --- > elf/dl-tunables.c | 56 ++++++++++++++++------------------- > elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++ > 2 files changed, 52 insertions(+), 30 deletions(-) > > diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c > index a2be9cde2f..1aedb9bd36 100644 > --- a/elf/dl-tunables.c > +++ b/elf/dl-tunables.c > @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring) > return; > > char *p = tunestr; > + size_t off = 0; > > while (true) > { > @@ -184,7 +185,11 @@ parse_tunables (char *tunestr, char *valstring) > /* If we reach the end of the string before getting a valid name-value > pair, bail out. */ > if (p[len] == '\0') > - return; > + { > + if (__libc_enable_secure) > + tunestr[off] = '\0'; > + return; > + } > > /* We did not find a valid name-value pair before encountering the > colon. */ > @@ -210,35 +215,28 @@ parse_tunables (char *tunestr, char *valstring) > > if (tunable_is_name (cur->name, name)) > { > - /* If we are in a secure context (AT_SECURE) then ignore the tunable > - unless it is explicitly marked as secure. Tunable values take > - precedence over their envvar aliases. */ > + /* If we are in a secure context (AT_SECURE) then ignore the > + tunable unless it is explicitly marked as secure. Tunable > + values take precedence over their envvar aliases. We write > + the tunables that are not SXID_ERASE back to TUNESTR, thus > + dropping all SXID_ERASE tunables and any invalid or > + unrecognized tunables. */ > if (__libc_enable_secure) > { > - if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE) > + if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE) > { > - if (p[len] == '\0') > - { > - /* Last tunable in the valstring. Null-terminate and > - return. */ > - *name = '\0'; > - return; > - } > - else > - { > - /* Remove the current tunable from the string. We do > - this by overwriting the string starting from NAME > - (which is where the current tunable begins) with > - the remainder of the string. We then have P point > - to NAME so that we continue in the correct > - position in the valstring. */ > - char *q = &p[len + 1]; > - p = name; > - while (*q != '\0') > - *name++ = *q++; > - name[0] = '\0'; > - len = 0; > - } > + if (off > 0) > + tunestr[off++] = ':'; > + > + const char *n = cur->name; > + > + while (*n != '\0') > + tunestr[off++] = *n++; > + > + tunestr[off++] = '='; > + > + for (size_t j = 0; j < len; j++) > + tunestr[off++] = value[j]; > } > > if (cur->security_level != TUNABLE_SECLEVEL_NONE) > @@ -251,9 +249,7 @@ parse_tunables (char *tunestr, char *valstring) > } > } > > - if (p[len] == '\0') > - return; > - else > + if (p[len] != '\0') > p += len + 1; > } > } > diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c > index 3d523875b1..05619c9adc 100644 > --- a/elf/tst-env-setuid-tunables.c > +++ b/elf/tst-env-setuid-tunables.c > @@ -45,11 +45,37 @@ > const char *teststrings[] = > { > "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2", > + "glibc.malloc.perturb=0x800", > + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", > + "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096", > + "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2", > + "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096", > + ":glibc.malloc.garbage=2:glibc.malloc.check=1", > + "glibc.malloc.check=1:glibc.malloc.check=2", > + "not_valid.malloc.check=2", > + "glibc.not_valid.check=2", > }; > > const char *resultstrings[] = > { > "glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.perturb=0x800", > + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=4096", > + "glibc.malloc.mmap_threshold=4096", > + "", > + "", > + "", > + "", > + "", > + "", > }; > > static int > -- Cheers, Carlos.