From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102b.google.com (mail-pj1-x102b.google.com [IPv6:2607:f8b0:4864:20::102b]) by sourceware.org (Postfix) with ESMTPS id 96FBE3858D32 for ; Mon, 2 Oct 2023 18:12:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 96FBE3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-pj1-x102b.google.com with SMTP id 98e67ed59e1d1-27747002244so40421a91.2 for ; Mon, 02 Oct 2023 11:12:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1696270354; x=1696875154; darn=sourceware.org; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=GWayyDRO/Vtth+6BrP9AF6aiGjSgwiXDh3FTRHetwsc=; b=TlN0hOmcsiDltBWZoixgp0Hl+c0O4Cq49+9zw80LSozRLOES8rVVNKLSit9fIs9XjU HtYabP13XXYrDLVlflNCtmIQ7+tLWjYE4+u4+kqJjp6LWdwjhB1aGikMd4BPiKvqftf2 xuwBEpXN9XFFu+nXEFlbhY9tv1pStbRixJzzfPV4QUGAbTp1BKLRDSUT+ebzfTb5oef8 C4+yFnzp0RJVbOU9U7v2cMgLzg/OCOx5VugjICStVFA22okkI6J8CP9hPV/lX0WE6FTe GxrUqI1K+Y62VutzU+EPG0sc4a6DP+FINw7WvTLLhN5h7t0DA9mNrG+LiQ8SF8u5VxD+ CJwQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696270354; x=1696875154; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=GWayyDRO/Vtth+6BrP9AF6aiGjSgwiXDh3FTRHetwsc=; b=QKfCQs9GSeEG4zzjNDa/ojWoHsiH+kKYtb0kkZEiB23I/yOh6R8wx/yGfgf5Py1oMP rVJD78MGQjhB+VZLTFV7oCI3AvrzcG9OkElaWVaPvtXo8tMV/WM6nxEhxlJ+BKRMIXwu y0f8FzhZygGEjbuRkA0k2i0g52hTB6vOfYx/75U1AZm5at9g18BZs+YSc/P7Dz2MiRML PmLWkJ9Xtj5R0sBMT4U+3tugOPeyl/AFcVDNAEGV8vPAgjm0y8B7rHoeSJxbz9I2Ol3Y Dy7psCTY3FevV/YQ2DCpiX0P+iAe4eqxn3UmscVwdhAlHOcTuZeRsR9V4jXOtLbUPV2+ kXzA== X-Gm-Message-State: AOJu0YzaT0i4VU6HkPrm1z/7mh85OazknovSrYxwJqHinZO5JFhfaxmI HqzShO2C9urPn/lPm9BCYW6ZlXmc2X1N26OtbwTc+w== X-Google-Smtp-Source: AGHT+IFEma462nUv47Yy+0zuBZxTJdcW73Gu2/GtT4APpsVnOko9paMgWSDiXQpFhoxj8LHdpjxq9A== X-Received: by 2002:a17:90a:7486:b0:268:b682:23de with SMTP id p6-20020a17090a748600b00268b68223demr12089650pjk.28.1696270353963; Mon, 02 Oct 2023 11:12:33 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c1:feaf:9c9c:46fb:1bba:fa15? ([2804:1b3:a7c1:feaf:9c9c:46fb:1bba:fa15]) by smtp.gmail.com with ESMTPSA id o28-20020a637e5c000000b0057cb5a780ebsm17814802pgn.76.2023.10.02.11.12.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 02 Oct 2023 11:12:33 -0700 (PDT) Message-ID: <166e1d36-7aea-40a6-bd94-502fec434175@linaro.org> Date: Mon, 2 Oct 2023 15:12:31 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Fix FORTIFY_SOURCE false positive Content-Language: en-US To: libc-alpha@sourceware.org, Siddhesh Poyarekar , =?UTF-8?Q?Volker_Wei=C3=9Fmann?= References: <20231002155339.2571514-1-volker.weissmann@gmx.de> <74305391-aad9-de52-9ad7-07df57e727f6@gotplt.org> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <74305391-aad9-de52-9ad7-07df57e727f6@gotplt.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,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 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): 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 -- 2.34.1