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 B79DD389367A for ; Mon, 12 Apr 2021 03:30:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B79DD389367A Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-54-yXp7waRSPm6oYalpsgFL1w-1; Sun, 11 Apr 2021 23:30:56 -0400 X-MC-Unique: yXp7waRSPm6oYalpsgFL1w-1 Received: by mail-qk1-f199.google.com with SMTP id h19so7899469qkk.4 for ; Sun, 11 Apr 2021 20:30:56 -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=VtDPtlLPWfcGTrDoZgaeT0a7draNOKByampV0uL3w7Q=; b=cOjZRm/7LsKHl1MzB59kEonrM9ib/TVHXGihhjZ7VN4BCqjgVd4Y4IW/U7sSVlj6VT TIBRlXjRJtlbRBkcje+eMj7EwbOX0YVU9RYUuyC8wfTzQIntm9p4rjWL5qcHWEUZ6isP r2pqBBFyM36LrFhbgTpFlgvsws7j+WtzlESyjocMYBRZqgHS8+VhYBSLN+kdjZSrUp9q 6Ir3UPWqsaAH5aXbsbMPfWZD9hWGfB6Jm6iXAD0Z+TUAfp30AvMkRorH/BVktglPKFZy tKnaNZscodDTguRQ/bBv+FiHpvNAOR0r2Cn8S+JTNU9R3pn/tCRdytZFsknngdpr74Hg mx+A== X-Gm-Message-State: AOAM532VYs5osWtwyNBAVY2hX7O5usM9NlQK/KMEFxt7ha4hjwZ8LVU3 2AQBHxF5VfHLMeV7OQT4YqDzDM+R6Ceaw+cjGv0nMWIFibHQkpqYHNcAqF5DXakRKRFIDWBYyN7 ZRGulHwlJfjsqo3vLRfN1YAzLQvHm0chtPcfemBvSNTeQTgHmsKZwnHo8UQaOQEdFjVZoww== X-Received: by 2002:a37:911:: with SMTP id 17mr8154280qkj.328.1618198255821; Sun, 11 Apr 2021 20:30:55 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxrgCsZOdFZZDPpKoWKwF6OspcrPN86Rn7ShKq/ytfToPVitKmIkQdHOtXWRRE+0vnVDTMEFA== X-Received: by 2002:a37:911:: with SMTP id 17mr8154269qkj.328.1618198255521; Sun, 11 Apr 2021 20:30:55 -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 s19sm7071155qks.130.2021.04.11.20.30.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 11 Apr 2021 20:30:55 -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: Sun, 11 Apr 2021 23:30:54 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 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=-13.7 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=unavailable 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: Mon, 12 Apr 2021 03:31:00 -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 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 > --- > 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 > -- Cheers, Carlos.