From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from crocodile.elm.relay.mailchannels.net (crocodile.elm.relay.mailchannels.net [23.83.212.45]) by sourceware.org (Postfix) with ESMTPS id 4C11A3858C60 for ; Fri, 21 Jan 2022 17:26:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4C11A3858C60 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 93F326C14F0; Fri, 21 Jan 2022 17:26:29 +0000 (UTC) Received: from pdx1-sub0-mail-a306.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id B36476C149A; Fri, 21 Jan 2022 17:26:27 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a306.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.124.238.99 (trex/6.4.3); Fri, 21 Jan 2022 17:26:29 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Trail-Suffer: 38705b01348a4d42_1642785989440_4113691762 X-MC-Loop-Signature: 1642785989440:3310233348 X-MC-Ingress-Time: 1642785989440 Received: from [192.168.1.174] (unknown [1.186.224.214]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a306.dreamhost.com (Postfix) with ESMTPSA id 4JgRB901h9zx; Fri, 21 Jan 2022 09:26:24 -0800 (PST) Message-ID: Date: Fri, 21 Jan 2022 22:56:19 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [PATCH v3 3/3] getcwd: Set errno to ERANGE for size == 1 (CVE-2021-3999) Content-Language: en-US To: Adhemerval Zanella , libc-alpha@sourceware.org Cc: eggert@cs.ucla.edu, fweimer@redhat.com, Qualys Security Advisory References: <20220119082147.3352868-1-siddhesh@sourceware.org> <20220120093252.1911498-1-siddhesh@sourceware.org> <20220120093252.1911498-4-siddhesh@sourceware.org> From: Siddhesh Poyarekar In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3491.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_BL_SPAMCOP_NET, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NEUTRAL, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Fri, 21 Jan 2022 17:26:34 -0000 On 21/01/2022 22:11, Adhemerval Zanella wrote: > > > On 20/01/2022 06:32, Siddhesh Poyarekar wrote: >> No valid path returned by getcwd would fit into 1 byte, so reject the >> size early and return NULL with errno set to ERANGE. This change is >> prompted by CVE-2021-3999, which describes a single byte buffer >> underflow and overflow when all of the following conditions are met: >> >> - The buffer size (i.e. the second argument of getcwd) is 1 byte >> - The current working directory is too long >> - '/' is also mounted on the current working directory >> >> Sequence of events: >> >> - In sysdeps/unix/sysv/linux/getcwd.c, the syscall returns ENAMETOOLONG >> because the linux kernel checks for name length before it checks >> buffer size >> >> - The code falls back to the generic getcwd in sysdeps/posix >> >> - In the generic func, the buf[0] is set to '\0' on line 250 >> >> - this while loop on line 262 is bypassed: >> >> while (!(thisdev == rootdev && thisino == rootino)) >> >> since the rootfs (/) is bind mounted onto the directory and the flow >> goes on to line 449, where it puts a '/' in the byte before the >> buffer. >> >> - Finally on line 458, it moves 2 bytes (the underflowed byte and the >> '\0') to the buf[0] and buf[1], resulting in a 1 byte buffer overflow. >> >> - buf is returned on line 469 and errno is not set. >> >> This resolves BZ #28769. >> >> Signed-off-by: Qualys Security Advisory >> Signed-off-by: Siddhesh Poyarekar > > Look good with just two fixed below for CMSG_DATA and a couple of comments. > Ok with the fixes, the comments would be good but it is a blocker. > > Reviewed-by: Adhemerval Zanella > >> --- >> NEWS | 6 + >> sysdeps/posix/getcwd.c | 7 + >> sysdeps/unix/sysv/linux/Makefile | 7 +- >> .../unix/sysv/linux/tst-getcwd-smallbuff.c | 245 ++++++++++++++++++ >> 4 files changed, 264 insertions(+), 1 deletion(-) >> create mode 100644 sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c >> >> diff --git a/NEWS b/NEWS >> index 4c392a445e..07e9eac52d 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -170,6 +170,12 @@ Security related changes: >> function could result in a memory leak and potential access of >> uninitialized memory. Reported by Qualys. >> >> + CVE-2021-3999: Passing a buffer of size exactly 1 byte to the getcwd >> + function may result in an off-by-one buffer underflow and overflow >> + when the current working directory is longer than PATH_MAX and also >> + corresponds to the / directory through an unprivileged mount >> + namespace. Reported by Qualys. >> + >> The following bugs are resolved with this release: >> >> [The release manager will add the list generated by >> diff --git a/sysdeps/posix/getcwd.c b/sysdeps/posix/getcwd.c >> index e147a31a81..9d5787b6f4 100644 >> --- a/sysdeps/posix/getcwd.c >> +++ b/sysdeps/posix/getcwd.c >> @@ -187,6 +187,13 @@ __getcwd_generic (char *buf, size_t size) >> size_t allocated = size; >> size_t used; >> >> + /* A size of 1 byte is never useful. */ >> + if (allocated == 1) >> + { >> + __set_errno (ERANGE); >> + return NULL; >> + } >> + >> #if HAVE_MINIMALLY_WORKING_GETCWD >> /* If AT_FDCWD is not defined, the algorithm below is O(N**2) and >> this is much slower than the system getcwd (at least on >> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile >> index 85fc8cbf75..7ca9350c99 100644 >> --- a/sysdeps/unix/sysv/linux/Makefile >> +++ b/sysdeps/unix/sysv/linux/Makefile >> @@ -346,7 +346,12 @@ sysdep_routines += xstatconv internal_statvfs \ >> >> sysdep_headers += bits/fcntl-linux.h >> >> -tests += tst-fallocate tst-fallocate64 tst-o_path-locks >> +tests += \ >> + tst-fallocate \ >> + tst-fallocate64 \ >> + tst-getcwd-smallbuff \ >> + tst-o_path-locks \ >> +# tests >> endif >> >> ifeq ($(subdir),elf) >> diff --git a/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c b/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c >> new file mode 100644 >> index 0000000000..791dfe4d02 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/tst-getcwd-smallbuff.c >> @@ -0,0 +1,245 @@ >> +/* Verify that getcwd returns ERANGE for size 1 byte and does not underflow >> + buffer when the CWD is too long and is also a mount target of /. See bug >> + #28769 or CVE-2021-3999 for more context. >> + Copyright The GNU Toolchain Authors. >> + 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 >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#ifndef PATH_MAX >> +# define PATH_MAX 1024 >> +#endif > > No need since it is a Linux only test and PATH_MAX is always defined. Ahh, I didn't actually need it anyway, leftover from a previous iteration. I'll remove it. > >> + >> +static char *base; >> +#define BASENAME "tst-getcwd-smallbuff" >> +#define MOUNT_NAME "mpoint" >> +static int sockfd[2]; >> + >> +static void >> +do_cleanup (void) >> +{ >> + support_chdir_toolong_temp_directory (base); >> + TEST_VERIFY_EXIT (rmdir (MOUNT_NAME) == 0); >> + free (base); >> +} >> + >> +static void >> +send_fd (const int sock, const int fd) >> +{ >> + struct msghdr msg; >> + union >> + { >> + struct cmsghdr hdr; >> + char buf[CMSG_SPACE (sizeof (int))]; >> + } cmsgbuf; > > Maybe zero-initialize both first to avoid the memset below? OK. > >> + struct cmsghdr *cmsg; >> + struct iovec vec; >> + char ch = 'A'; >> + ssize_t n; >> + >> + memset (&msg, 0, sizeof (msg)); >> + memset (&cmsgbuf, 0, sizeof (cmsgbuf)); >> + msg.msg_control = &cmsgbuf.buf; >> + msg.msg_controllen = sizeof (cmsgbuf.buf); >> + >> + cmsg = CMSG_FIRSTHDR (&msg); >> + cmsg->cmsg_len = CMSG_LEN (sizeof (int)); >> + cmsg->cmsg_level = SOL_SOCKET; >> + cmsg->cmsg_type = SCM_RIGHTS; >> + *(int *) CMSG_DATA (cmsg) = fd; > > I think CMSG_DATA does not guarantee the alignment, so I think it would be > safe to use memcpy here: > > memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd)); OK. > >> + >> + vec.iov_base = &ch; >> + vec.iov_len = 1; >> + msg.msg_iov = &vec; >> + msg.msg_iovlen = 1; >> + >> + while ((n = sendmsg (sock, &msg, 0)) == -1 && errno == EINTR); >> + >> + TEST_VERIFY_EXIT (n == 1); >> +} >> + > > Ok. > >> +static int >> +recv_fd (const int sock) >> +{ >> + struct msghdr msg; > > Maybe also zero-initialize here. > >> + union >> + { >> + struct cmsghdr hdr; >> + char buf[CMSG_SPACE(sizeof(int))]; >> + } cmsgbuf; >> + struct cmsghdr *cmsg; >> + struct iovec vec; >> + ssize_t n; >> + char ch = '\0'; >> + int fd = -1; >> + >> + memset (&msg, 0, sizeof (msg)); >> + vec.iov_base = &ch; >> + vec.iov_len = 1; >> + msg.msg_iov = &vec; >> + msg.msg_iovlen = 1; >> + >> + memset (&cmsgbuf, 0, sizeof (cmsgbuf)); >> + msg.msg_control = &cmsgbuf.buf; >> + msg.msg_controllen = sizeof (cmsgbuf.buf); >> + >> + while ((n = recvmsg (sock, &msg, 0)) == -1 && errno == EINTR); >> + if (n != 1 || ch != 'A') >> + return -1; >> + >> + cmsg = CMSG_FIRSTHDR (&msg); >> + if (cmsg == NULL) >> + return -1; >> + if (cmsg->cmsg_type != SCM_RIGHTS) >> + return -1; >> + fd = *(const int *) CMSG_DATA (cmsg); > > Same as before, I think you will need to copy to a temporary using memcpy. Why not just: memcpy (&fd, CMSG_DATA (cmsg), sizeof (fd)); i.e., is a temporary necessary? > >> + if (fd < 0) >> + return -1; >> + return fd; >> +} >> + >> +static int >> +child_func (void * const arg) >> +{ >> + xclose (sockfd[0]); >> + const int sock = sockfd[1]; >> + char ch; >> + >> + TEST_VERIFY_EXIT (read (sock, &ch, 1) == 1); >> + TEST_VERIFY_EXIT (ch == '1'); >> + >> + if (mount ("/", MOUNT_NAME, NULL, MS_BIND | MS_REC, NULL)) >> + FAIL_EXIT1 ("mount failed: %m\n"); >> + const int fd = xopen ("mpoint", >> + O_RDONLY | O_PATH | O_DIRECTORY | O_NOFOLLOW, 0); >> + >> + send_fd (sock, fd); >> + xclose (fd); >> + >> + TEST_VERIFY_EXIT (read (sock, &ch, 1) == 1); >> + TEST_VERIFY_EXIT (ch == 'a'); >> + >> + xclose (sock); >> + return 0; >> +} >> + >> +static void >> +update_map (char * const mapping, const char * const map_file) >> +{ >> + const size_t map_len = strlen (mapping); >> + >> + const int fd = xopen (map_file, O_WRONLY, 0); >> + xwrite (fd, mapping, map_len); >> + xclose (fd); >> +} >> + >> +static void >> +proc_setgroups_write (const long child_pid, const char * const str) >> +{ >> + const size_t str_len = strlen(str); >> + >> + char setgroups_path[64]; > > > Maybe define the size as: > > /* The path is the form /proc/%ld/setgroups. */ > char map_path[sizeof("/proc/setgroups") + INT_STRLEN_BOUND (long int)]; > OK. >> + snprintf (setgroups_path, sizeof (setgroups_path), >> + "/proc/%ld/setgroups", child_pid); >> + >> + const int fd = open (setgroups_path, O_WRONLY); >> + >> + if (fd < 0) >> + { >> + TEST_VERIFY_EXIT (errno == ENOENT); >> + FAIL_UNSUPPORTED ("/proc/%ld/setgroups not found\n", child_pid); >> + } >> + >> + xwrite (fd, str, str_len); >> + xclose(fd); >> +} >> + >> +static char child_stack[1024 * 1024]; >> + >> +int >> +do_test (void) >> +{ >> + base = support_create_and_chdir_toolong_temp_directory (BASENAME); >> + >> + xmkdir (MOUNT_NAME, S_IRWXU); >> + atexit (do_cleanup); >> + >> + TEST_VERIFY_EXIT (socketpair (AF_UNIX, SOCK_STREAM, 0, sockfd) == 0); >> + pid_t child_pid = xclone (child_func, NULL, child_stack, >> + sizeof (child_stack), >> + CLONE_NEWUSER | CLONE_NEWNS | SIGCHLD); >> + >> + xclose (sockfd[1]); >> + const int sock = sockfd[0]; >> + >> + char map_path[64], map_buf[64]; > > Same comment as for setgroups_path. > >> + snprintf (map_path, sizeof (map_path), , >> + (long) child_pid); >> + snprintf (map_buf, sizeof (map_buf), "0 %ld 1", (long) getuid()); >> + update_map (map_buf, map_path); >> + >> + proc_setgroups_write ((long) child_pid, "deny"); >> + snprintf (map_path, sizeof (map_path), "/proc/%ld/gid_map", >> + (long) child_pid); >> + snprintf (map_buf, sizeof (map_buf), "0 %ld 1", (long) getgid()); >> + update_map (map_buf, map_path); >> + >> + TEST_VERIFY_EXIT (send (sock, "1", 1, MSG_NOSIGNAL) == 1); >> + const int fd = recv_fd (sock); >> + TEST_VERIFY_EXIT (fd >= 0); >> + TEST_VERIFY_EXIT (fchdir (fd) == 0); >> + >> + static char buf[2 * 10 + 1]; >> + memset (buf, 'A', sizeof(buf)); > > > Space before (. > >> + >> + /* Finally, call getcwd and check if it resulted in a buffer underflow. */ >> + char * cwd = getcwd (buf + sizeof(buf) / 2, 1); >> + TEST_VERIFY (cwd == NULL); >> + TEST_VERIFY (errno == ERANGE); >> + >> + for (int i = 0; i < sizeof (buf); i++) >> + if (buf[i] != 'A') >> + { >> + printf ("buf[%d] = %02x\n", i, (unsigned int) buf[i]); >> + support_record_failure (); >> + } >> + >> + TEST_VERIFY_EXIT (send (sock, "a", 1, MSG_NOSIGNAL) == 1); >> + xclose (sock); >> + TEST_VERIFY_EXIT (xwaitpid (child_pid, NULL, 0) == child_pid); >> + >> + return 0; >> +} >> + >> +#define CLEANUP_HANDLER do_cleanup >> +#include >