From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from butterfly.birch.relay.mailchannels.net (butterfly.birch.relay.mailchannels.net [23.83.209.27]) by sourceware.org (Postfix) with ESMTPS id 564383858414 for ; Mon, 2 Oct 2023 18:20:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 564383858414 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=gotplt.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gotplt.org 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 B872B4202D; Mon, 2 Oct 2023 18:20:05 +0000 (UTC) Received: from pdx1-sub0-mail-a248.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 42A7240F21; Mon, 2 Oct 2023 18:20:05 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1696270805; a=rsa-sha256; cv=none; b=LyxyrdFKTy8yjmjXjfeO6yK1kAj5qBdaceE2LMq1JtvmaUp8JWcOss+pMiyk1dSN1hC6oq AW82RZRKMVESTUpQjRuzc/TxF+TTTPDbnZeU0rU+LQxffPYyx/cOvBBlHnAmeN2ZDx3sda 5DLQsEpASRERN+R8kZmsq/OZcfSkT10rO+5IYLr/G0ysC/l4OhgoqqmY31EI61qL60mtgr vA7ynf0Sx7mnR8LaiHldrqQ5WjRb37avLKYHuiZqnkSWtAijE075k3nZ3W18zMptxqxuX8 wV1nrIa5TLUF+/rX1HgGiBeAlw4NQHvPDyi4I1RafcwNV7DqxK9M9aP7WFR+xw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1696270805; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9vgNed8ptPDMo7jpDTMfvlLvHFEEu6oQf0AQGeKvL/c=; b=dB/x/qG+oKC/Hm0wmsY9/NetK4HvNO0kE1l+KxX284XzEnz+vWuYqdM7hWos3oqFJ+dmNQ /KMsDbvfqAMuKw0g8aFvgezeruE0TmwxgTiv3/ek11BbNX+aVANjtaqqZolxz32KGMKWCu S5RUjY/Fez4os4Ep5OzTbcuXQA7ujXFl9JeAhV9N5hCsyVrs6LDy/P6hXarB73yi2M298m GDoSCuIn80xqDfNTEkkpWV8gmGbf1NQmOlykUiyO5pRiI2FYcHwM7h6DE/xiHcXB9rpiNL d0FL82hGTlLYJQ8Hs5YMNJLtcXfOoP8/9Q9BchwThtbdaopbKE9L8gF48mGS9A== ARC-Authentication-Results: i=1; rspamd-7d5dc8fd68-5l42m; auth=pass smtp.auth=dreamhost smtp.mailfrom=siddhesh@gotplt.org X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Versed-Troubled: 227b78414809998c_1696270805518_1958500451 X-MC-Loop-Signature: 1696270805518:2196735666 X-MC-Ingress-Time: 1696270805518 Received: from pdx1-sub0-mail-a248.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.124.45.11 (trex/6.9.1); Mon, 02 Oct 2023 18:20:05 +0000 Received: from [192.168.2.12] (bras-vprn-toroon4834w-lp130-02-142-113-138-41.dsl.bell.ca [142.113.138.41]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a248.dreamhost.com (Postfix) with ESMTPSA id 4Rzq4N60d5zC2; Mon, 2 Oct 2023 11:20:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gotplt.org; s=dreamhost; t=1696270805; bh=9vgNed8ptPDMo7jpDTMfvlLvHFEEu6oQf0AQGeKvL/c=; h=Date:Subject:To:From:Content-Type:Content-Transfer-Encoding; b=lmMVJtPubIDNSs8h8ht/YSAERr7Lm+eCM3OgImWZNxrDPa7dorEzDeF7f+/7H7gYD 6+PAgIjXkojLG6coSxWxngJ09RytIbaf8Vi8N2zob1WjrSq+qLZ1i6yLQF9Gr1iSeg GSwgmf7aV/EMcdpSpjIDTZV49+ctC277oDq03CPAT8bAwGNkmjHww8l0Przk3KZwdF 16G9u3a3lCfP/HqGo8voG3VNzinpAli2xyhPlOsg5pnl+sUxlp+hWBa574Z7v6u1+z /EIzBA7w/Ktgvyz3NxKGD5/75ppX7gzXNAS9YxxihxCDX89jA7rrxH0IdA38qNSE7g YQ6JtO25i3YNg== Message-ID: <3bd8ceda-eb83-f56c-4582-fbae72a003a9@gotplt.org> Date: Mon, 2 Oct 2023 14:20:03 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH] Fix FORTIFY_SOURCE false positive Content-Language: en-US To: Adhemerval Zanella Netto , libc-alpha@sourceware.org, =?UTF-8?Q?Volker_Wei=c3=9fmann?= References: <20231002155339.2571514-1-volker.weissmann@gmx.de> <74305391-aad9-de52-9ad7-07df57e727f6@gotplt.org> <166e1d36-7aea-40a6-bd94-502fec434175@linaro.org> From: Siddhesh Poyarekar In-Reply-To: <166e1d36-7aea-40a6-bd94-502fec434175@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3036.1 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2023-10-02 14:12, Adhemerval Zanella Netto wrote: > > > On 02/10/23 15:00, Siddhesh Poyarekar wrote: >> On 2023-10-02 11:53, Volker Weißmann wrote: >>> When -D_FORTIFY_SOURCE=2 was given during compilation, >>> sprintf and similar functions will check if their >>> first argument is in read-only memory and exit with >>> *** %n in writable segment detected *** >>> otherwise. To check if the memory is read-only, glibc >>> reads form the file "/proc/self/maps". If opening this >>> file fails due to too many open files (EMFILE), glibc >>> will now ignore this error. >>> --- >> >> Ugh, that looks like an easy way to defeat format string fortification :/ > > The sprintf '%n' fortify is already brittle to rely on procfs access to check > if the format string is on a rw segment. > >> >> The fix is fine I think, just a little nit below. >> >>>   sysdeps/unix/sysv/linux/readonly-area.c | 10 +++++++++- >>>   1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c >>> index edc68873f6..629163461a 100644 >>> --- a/sysdeps/unix/sysv/linux/readonly-area.c >>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c >>> @@ -42,7 +42,15 @@ __readonly_area (const char *ptr, size_t size) >>>            to the /proc filesystem if it is set[ug]id.  There has >>>            been no willingness to change this in the kernel so >>>            far.  */ >>> -      || errno == EACCES) >>> +      || errno == EACCES >>> +          /* Example code to trigger EMFILE: >>> +          while(1) { >>> +            FILE *file = fopen("/dev/zero", "r"); >>> +            assert(file != NULL); >>> +          } >>> +          If your libc was compiled with -D_FORTIFY_SOURCE=2, we run >> >> Shouldn't this be "If the program was compiled with..." and not libc? Also, maybe the example code is unnecessary and you could just mention that the if the open file threshold is reached, this could become a spurious failure. >> > > I will just add a more simpler comment: > > /* Process has reached the maximum number of open files. */ > >>> +          into this if clause here. */ >>> +          || errno == EMFILE) >>>       return 1; >>>         return -1; >>>       } >>> -- >>> 2.42.0 >>> >> >> Also, if you don't have a copyright assignment on file with the FSF, could you add a Signed-off-by to certify your contribution? > > I think we should add a regression test for this, with EMFILE is easy to create > one than the one that check for procfs (which would require a test-container > that does not mount procfs): Sounds good. Once Volker sends a v2 with their S-o-b, could you include that and post a patchset with your test? That way both get credit for their work and pre-commit CI also won't complain. Thanks, Sid > > diff --git a/debug/Makefile b/debug/Makefile > index 434e52f780..d7e021a1c8 100644 > --- a/debug/Makefile > +++ b/debug/Makefile > @@ -178,6 +178,7 @@ CFLAGS-tst-longjmp_chk3.c += -fexceptions -fasynchronous-unwind-tables > CPPFLAGS-tst-longjmp_chk3.c += $(no-fortify-source),-D_FORTIFY_SOURCE=1 > CPPFLAGS-tst-realpath-chk.c += $(no-fortify-source),-D_FORTIFY_SOURCE=2 > CPPFLAGS-tst-chk-cancel.c += $(no-fortify-source),-D_FORTIFY_SOURCE=2 > +CFLAGS-tst-sprintf-fortify-rdonly.c += $(no-fortify-source),-D_FORTIFY_SOURCE=2 > > # _FORTIFY_SOURCE tests. > # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and > @@ -284,6 +285,7 @@ tests = \ > tst-longjmp_chk \ > tst-longjmp_chk2 \ > tst-realpath-chk \ > + tst-sprintf-fortify-rdonly \ > tst-sprintf-fortify-unchecked \ > # tests > > @@ -291,6 +293,9 @@ tests-time64 += \ > $(tests-all-time64-chk) \ > # tests-time64 > > +tests-container += \ > + # tests-container > + > ifeq ($(have-ssp),yes) > tests += tst-ssp-1 > endif > diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c > new file mode 100644 > index 0000000000..f07e4c7e4a > --- /dev/null > +++ b/debug/tst-sprintf-fortify-rdonly.c > @@ -0,0 +1,81 @@ > +/* Copyright (C) 2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +jmp_buf chk_fail_buf; > +bool chk_fail_ok; > + > +const char *str2 = "F"; > +char buf2[10] = "%s"; > + > +static int > +do_test (void) > +{ > + struct rlimit rl; > + int max_fd = 24; > + > + if (getrlimit (RLIMIT_NOFILE, &rl) == -1) > + FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m"); > + > + max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd); > + rl.rlim_cur = max_fd; > + > + if (setrlimit (RLIMIT_NOFILE, &rl) == 1) > + FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m"); > + > + /* Exhaust the file descriptor limit with temporary files. */ > + int nfiles = 0; > + for (; nfiles < max_fd; nfiles++) > + { > + int fd = create_temp_file ("tst-spawn3.", NULL); > + if (fd == -1) > + { > + if (errno != EMFILE) > + FAIL_EXIT1 ("create_temp_file: %m"); > + break; > + } > + } > + TEST_VERIFY_EXIT (nfiles != 0); > + > + /* When the format string is writable and contains %n, > + with -D_FORTIFY_SOURCE=2 it causes __chk_fail. However, if libc can not > + open procfs to check if the input format string in within a writable > + memory segment, the fortify version can not perform the check. */ > + char buf[128]; > + int n1; > + int n2; > + > + strcpy (buf2 + 2, "%n%s%n"); > + if (sprintf (buf, buf2, str2, &n1, str2, &n2) != 2 > + || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); > + > + return 0; > +} > + > +#include