Message ID | 20210316070755.330084-5-siddhesh@sourceware.org |
---|---|
State | Committed |
Commit | 2ed18c5b534d9e92fc006202a5af0df6b72e7aca |
Delegated to: | Carlos O'Donell |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C1288385482C; Tue, 16 Mar 2021 07:08:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C1288385482C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1615878496; bh=zPsAKF611O2eh86VBbmlJSfdoKL9EqTcqgX/FQCkAxo=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=WnIOysRlKbjYqJe9+bdwjFacyT5BlvaMhFYDlcqmyFMno86a1gi4FnJvGf/097qBs p9amO2GdP9C/cHOYtz/r/VZXQpIGKqlEBwutAXuOkKebEV5dnwlX+/J+hmWIlBEEZb 9FLSBuxBx3EUVCYQZ1Oq4D+91olDkJoAd1XZdVoU= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from dog.elm.relay.mailchannels.net (dog.elm.relay.mailchannels.net [23.83.212.48]) by sourceware.org (Postfix) with ESMTPS id 44EC3385AC12 for <libc-alpha@sourceware.org>; Tue, 16 Mar 2021 07:08:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 44EC3385AC12 X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 689C334100D; Tue, 16 Mar 2021 07:08:12 +0000 (UTC) Received: from pdx1-sub0-mail-a59.g.dreamhost.com (100-96-17-75.trex.outbound.svc.cluster.local [100.96.17.75]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 029F1342291; Tue, 16 Mar 2021 07:08:11 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a59.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384) by 100.96.17.75 (trex/6.1.1); Tue, 16 Mar 2021 07:08:12 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Plucky-Chemical: 194c4029278fafb9_1615878492257_3949697286 X-MC-Loop-Signature: 1615878492257:2311013066 X-MC-Ingress-Time: 1615878492257 Received: from pdx1-sub0-mail-a59.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a59.g.dreamhost.com (Postfix) with ESMTP id B93568A69D; Tue, 16 Mar 2021 00:08:11 -0700 (PDT) Received: from rhbox.redhat.com (unknown [1.186.101.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a59.g.dreamhost.com (Postfix) with ESMTPSA id 4B29B8A69E; Tue, 16 Mar 2021 00:08:09 -0700 (PDT) X-DH-BACKEND: pdx1-sub0-mail-a59 To: libc-alpha@sourceware.org Subject: [PATCH 4/4] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Date: Tue, 16 Mar 2021 12:37:55 +0530 Message-Id: <20210316070755.330084-5-siddhesh@sourceware.org> X-Mailer: git-send-email 2.29.2 In-Reply-To: <20210316070755.330084-1-siddhesh@sourceware.org> References: <20210316070755.330084-1-siddhesh@sourceware.org> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3495.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, 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 <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Siddhesh Poyarekar via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Siddhesh Poyarekar <siddhesh@sourceware.org> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
tunables and setxid programs
|
|
Commit Message
Siddhesh Poyarekar
March 16, 2021, 7:07 a.m. UTC
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. 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. - Unrecognized tunables get scrubbed off from the environment and subsequently from the child environment. - 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. --- elf/dl-tunables.c | 56 ++++++++++++++++------------------- elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++ 2 files changed, 52 insertions(+), 30 deletions(-)
Comments
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 >
On 4/7/21 1:17 AM, Carlos O'Donell wrote: > 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? That comment was based on a previous consensus[1] where we agreed that we eventually port all SXID_IGNORE tunables into SXID_ERASE but I never actually got around to doing it. However you seem to arguing the opposite case, i.e. making tunables SXID_IGNORE by default and porting SXID_ERASE over to it. As I've mentioned in the bug report, I don't see miscategorization of tunables resulting in any *new* security issues; at worst they may provide additional control over processes across setxid boundaries that may otherwise not be possible. In that sense, sxid categorization is a security hardening feature and the discussion is about whether the hardening is going too far. > 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? There was one namespace renaming from glibc.tune to glibc.cpu, but it was aarch64-specific. It defaults to SXID_ERASE, as do most tunables. Only tunables that were ported from environment variables, e.g. glibc.malloc.mmap_threshold are SXID_IGNORE. > (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? To summarize my understanding, you argue that there may be a legitimate need to be able to control environment of processes across the sxid boundary. While I agree that there ought to be ways to control processes across sxid boundaries I don't think it should be as trivial as setting the program environment of an sxid program. Maybe the use case you're referring to is better served by systemwide and userwide tunables. For example, if I had to control the environment of httpd, it should be through $HTTPD_HOME/.glibc-tunables or /etc/glibc-tunables and not through $SIDDHESH_HOME/.glibc-tunables or the environment of user siddhesh. This may not work for legacy tunables if the older environment doesn't support systemwide or userwide tunables but that shouldn't cause problems in practice. Siddhesh [1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html
On 4/8/21 12:38 AM, Siddhesh Poyarekar wrote: > On 4/7/21 1:17 AM, Carlos O'Donell wrote: >> 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? > > That comment was based on a previous consensus[1] where we agreed > that we eventually port all SXID_IGNORE tunables into SXID_ERASE but > I never actually got around to doing it. However you seem to arguing > the opposite case, i.e. making tunables SXID_IGNORE by default and > porting SXID_ERASE over to it. If a deprecated SXID_IGNORE variable existed, which it doesn't, then I would have expected it to cross the boundary. I'm not arguing about defaults just behaviour of existing code and how it might be used by users. My argument is that it is not "wrong" for tunables to cross the SXID boundary if they were marked as being allowed to cross that boundary. However, I need to review this patch again because: * By default we set tunables to SXID_ERASE. * We have no deprecated SXID_IGNORE tunables that I know about. If we had the latter then we would need to do more work to keep them, but we don't so your patch might be OK. > As I've mentioned in the bug report, I don't see miscategorization of > tunables resulting in any *new* security issues; at worst they may > provide additional control over processes across setxid boundaries > that may otherwise not be possible. In that sense, sxid > categorization is a security hardening feature and the discussion is > about whether the hardening is going too far. The hardening, SXID_ERASE, should follow the documented behaviour for a given variable, including deprecated ones. >> 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? > > There was one namespace renaming from glibc.tune to glibc.cpu, but it > was aarch64-specific. It defaults to SXID_ERASE, as do most > tunables. Only tunables that were ported from environment variables, > e.g. glibc.malloc.mmap_threshold are SXID_IGNORE. Right, which is why today your patch should be OK, and I need to redo the review. > >> (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? > > To summarize my understanding, you argue that there may be a > legitimate need to be able to control environment of processes across > the sxid boundary. While I agree that there ought to be ways to > control processes across sxid boundaries I don't think it should be > as trivial as setting the program environment of an sxid program. I think it should. Setting values in a file for a container or cross-mount-namespace requires considerable amount of work. In k8s and even Red Hat OCP there is *significant* use of environment variables to express tunables for containers. Consider a read-only runtime... how do you impact the behaviour? How do you test a tuning to a hardware feature that only the container runtime knows about? In practice I'd like the tuning to be automatic e.g. ifunc, but some options are difficult and workload specific, and so may need to pass to a child process across an SXID boundary. It is *more* dangerous IMO to agree to mount a read-write filesystem into which you have to write configuration data. However, I don't think this discussion should or needs to be resolved today since we have no deprecated SXID_IGNORE env vars that would need special handling. > Maybe the use case you're referring to is better served by systemwide > and userwide tunables. For example, if I had to control the > environment of httpd, it should be through > $HTTPD_HOME/.glibc-tunables or /etc/glibc-tunables and not through > $SIDDHESH_HOME/.glibc-tunables or the environment of user siddhesh. > This may not work for legacy tunables if the older environment > doesn't support systemwide or userwide tunables but that shouldn't > cause problems in practice. Some tunables I think can be set in that way because you have: * A known workload tuning. * A known parameter for your runtime. But there are much harder parameters to set that depend on the runtime knowing something more about the host. Today this happens to be environment variables. > [1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html
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 am reviewing this a second time after our downthread discussion that there are no SXID_IGNORE tunables that are deprecated and that would actually need to be handled in a special way. Any deprecated SXID_ERASE variables would be unknown to a later runtime and be erased anyway matching the SXID_ERASE requirement. > 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. > > - Unrecognized tunables get scrubbed off from the environment and > subsequently from the child environment. > > - 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. Re-review in the context of a limited change to fix the bug. OK to commit. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > 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; OK. > > 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; OK. Terminate the tunable if we are secure. > + } > > /* 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. */ OK. For today there are no SXID_IGNORE tunables that have been deprecated, and so all currently deprecated tunables were always SXID_ERASE and so this doesn't change the current behaviour. Future changes will need to take this into account. > 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]; OK. > } > > 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') OK. > 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", > + "", > + "", > + "", > + "", > + "", > + "", OK. Adds many more tests, which is great! > }; > > static int >
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