From patchwork Wed Feb 12 20:24:54 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 1237068 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-109628-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha1 header.s=default header.b=FEKKAPvD; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=StRSREsV; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48Hrjy6JXGz9s1x for ; Thu, 13 Feb 2020 07:25:26 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type:content-transfer-encoding; q=dns; s=default; b=yis dki5Mgm4znvBfujTJ2R2vpcymJ39WxRKm7xSxDlUCAiBZQiT2KMXP0WfUZ1fcQr9 yu3XXnHjQCW3SzqZ3XaO1gS3evPgIB6GDMlLoPyUQTl4jp/TzXq5qxQ0O8J9YE6R FJ0HvZxj7lUNhB6v5p+380NASSx3qBbrz5wmXzdg= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id:mime-version :content-type:content-transfer-encoding; s=default; bh=CdpZj5Vk9 FcoW1lNNZb7C+sD1iY=; b=FEKKAPvDdimL0l0BpRUFDl53gJDstAKPJGcsj8NGy 7hAQR+2Tp+GD8HrlRQDEpgPPH+MH7lHpye3Kh/2bDDDZtW+l7V89S5qbV3CyAavn vG4pA74fYG24yGAi/WPvYJei8Dc0FoIBw0AwRmaG3SuKEjP3tcxF5O5T+oJs390Y EM= Received: (qmail 60183 invoked by alias); 12 Feb 2020 20:25:20 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 60079 invoked by uid 89); 12 Feb 2020 20:25:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581539102; 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; bh=XJDlYO/NmL/zuSnXQ46eRWFxkfL132PjJL5spGHqUCU=; b=StRSREsV5lMPL1MEi2sAojPtDwphhlxVdE6LYQGB9p5ZwamoCZsqVCRJ4JBF8tcNsDgMgH duHbiHbFzyJPpG4H/hbPIFFP+q4U2AJpGJyrMjBZA+TZ1llYWUE0pjacx9tpgL7qBfsMgX 4GXeWnhn1XV6UWGFWIngPDY1SH+ukTs= From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH] Linux: Work around kernel bugs in chmod on /proc/self/fd paths Date: Wed, 12 Feb 2020 21:24:54 +0100 Message-ID: <87sgjfd04p.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com It appears that the ability to change symbolic link modes through such paths is unintended. On several file systems, the operation fails with EOPNOTSUPP, even though the symbolic link permissions are updated. The expected behavior is a failure to update the permissions, without file system changes. ----- io/tst-lchmod.c | 83 ++++++++++++-------------------------- sysdeps/unix/sysv/linux/fchmodat.c | 28 +++++++++++-- 2 files changed, 49 insertions(+), 62 deletions(-) Reviewed-by: Matheus Castanho diff --git a/io/tst-lchmod.c b/io/tst-lchmod.c index 73e45549af..59873f130d 100644 --- a/io/tst-lchmod.c +++ b/io/tst-lchmod.c @@ -102,68 +102,44 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t, TEST_VERIFY ((st.st_mode & 0777) != 2); mode_t original_symlink_mode = st.st_mode; - /* Set to true if AT_SYMLINK_NOFOLLOW is supported. */ - bool nofollow; - /* We should be able to change the mode of a file, including through the symbolic link to-file. */ const char *arg = select_path (do_relative_path, path_file, "file"); TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0); xstat (path_file, &st); TEST_COMPARE (st.st_mode & 0777, 1); - int ret = chmod_func (fd, path_file, 2, AT_SYMLINK_NOFOLLOW); - if (ret == 0) - { - printf ("info: AT_SYMLINK_NOFOLLOW support in %s\n", tempdir); - nofollow = true; - } - else - { - printf ("info: no AT_SYMLINK_NOFOLLOW support in %s\n", tempdir); - nofollow = false; - - /* Set up things for the code below. */ - TEST_COMPARE (chmod_func (fd, path_file, 2, 0), 0); - } + arg = select_path (do_relative_path, path_to_file, "to-file"); + TEST_COMPARE (chmod_func (fd, path_to_file, 2, 0), 0); xstat (path_file, &st); TEST_COMPARE (st.st_mode & 0777, 2); + xlstat (path_to_file, &st); + TEST_COMPARE (original_symlink_mode, st.st_mode); + arg = select_path (do_relative_path, path_file, "file"); + TEST_COMPARE (chmod_func (fd, arg, 1, 0), 0); + xstat (path_file, &st); + TEST_COMPARE (st.st_mode & 0777, 1); + xlstat (path_to_file, &st); + TEST_COMPARE (original_symlink_mode, st.st_mode); + + /* Changing the mode of a symbolic link should fail fail. */ arg = select_path (do_relative_path, path_to_file, "to-file"); - TEST_COMPARE (chmod_func (fd, path_to_file, 1, 0), 0); + int ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW); + TEST_COMPARE (ret, -1); + TEST_COMPARE (errno, EOPNOTSUPP); + + /* The modes should remain unchanged. */ xstat (path_file, &st); TEST_COMPARE (st.st_mode & 0777, 1); xlstat (path_to_file, &st); TEST_COMPARE (original_symlink_mode, st.st_mode); - /* Changing the mode of a symbolic link may fail. */ arg = select_path (do_relative_path, path_to_file, "to-file"); ret = chmod_func (fd, arg, 2, AT_SYMLINK_NOFOLLOW); - if (nofollow) - { - TEST_COMPARE (ret, 0); + TEST_COMPARE (ret, -1); + TEST_COMPARE (errno, EOPNOTSUPP); - /* The mode of the link changed. */ - xlstat (path_to_file, &st); - TEST_COMPARE (st.st_mode & 0777, 2); - - /* But the mode of the file is unchanged. */ - xstat (path_file, &st); - TEST_COMPARE (st.st_mode & 0777, 1); - - } - else - { - TEST_COMPARE (ret, -1); - TEST_COMPARE (errno, EOPNOTSUPP); - - /* The modes should remain unchanged. */ - xstat (path_file, &st); - TEST_COMPARE (st.st_mode & 0777, 1); - xlstat (path_to_file, &st); - TEST_COMPARE (original_symlink_mode, st.st_mode); - } - - /* If we have NOFOLLOW support, we should be able to change the mode - of a dangling symbolic link or a symbolic link loop. */ + /* Likewise, changing dangling and looping symbolic links must + fail. */ const char *paths[] = { path_dangling, path_loop }; for (size_t i = 0; i < array_length (paths); ++i) { @@ -178,19 +154,10 @@ test_1 (bool do_relative_path, int (*chmod_func) (int fd, const char *, mode_t, original_symlink_mode = st.st_mode; arg = select_path (do_relative_path, path, filename); ret = chmod_func (fd, arg, new_mode, AT_SYMLINK_NOFOLLOW); - if (nofollow) - { - TEST_COMPARE (ret, 0); - xlstat (path, &st); - TEST_COMPARE (st.st_mode & 0777, new_mode); - } - else /* !nofollow. */ - { - TEST_COMPARE (ret, -1); - TEST_COMPARE (errno, EOPNOTSUPP); - xlstat (path, &st); - TEST_COMPARE (st.st_mode, original_symlink_mode); - } + TEST_COMPARE (ret, -1); + TEST_COMPARE (errno, EOPNOTSUPP); + xlstat (path, &st); + TEST_COMPARE (st.st_mode, original_symlink_mode); } /* A missing file should always result in ENOENT. The presence of diff --git a/sysdeps/unix/sysv/linux/fchmodat.c b/sysdeps/unix/sysv/linux/fchmodat.c index 719053b333..17eca54051 100644 --- a/sysdeps/unix/sysv/linux/fchmodat.c +++ b/sysdeps/unix/sysv/linux/fchmodat.c @@ -45,6 +45,30 @@ fchmodat (int fd, const char *file, mode_t mode, int flag) caller can treat them as temporary if necessary. */ return pathfd; + /* Use fstatat because fstat does not work on O_PATH descriptors + before Linux 3.6. */ + struct stat64 st; + if (fstatat64 (pathfd, "", &st, AT_EMPTY_PATH) != 0) + { + __close_nocancel (pathfd); + return -1; + } + + /* Some Linux versions with some file systems can actually + change symbolic link permissions via /proc, but this is not + intentional, and it gives inconsistent results (e.g., error + return despite mode change). The expected behavior is that + symbolic link modes cannot be changed at all, and this check + enforces that. */ + if (S_ISLNK (st.st_mode)) + { + __close_nocancel (pathfd); + __set_errno (EOPNOTSUPP); + return -1; + } + + /* For most file systems, fchmod does not operate on O_PATH + descriptors, so go through /proc. */ char buf[32]; if (__snprintf (buf, sizeof (buf), "/proc/self/fd/%d", pathfd) < 0) { @@ -54,10 +78,6 @@ fchmodat (int fd, const char *file, mode_t mode, int flag) return -1; } - /* This operates directly on the symbolic link if it is one. - /proc/self/fd files look like symbolic links, but they are - not. (fchmod and fchmodat do not work on O_PATH descriptors, - similar to fstat before Linux 3.6.) */ int ret = __chmod (buf, mode); if (ret != 0) {