Skip to content

Commit b4259c3

Browse files
shailend-ggvisor-bot
authored andcommitted
Implement S_ISGID clearing behavior in chmod.
When a non-privileged process (lacking `CAP_FSETID`) attempts to set the `S_ISGID` bit on a file, and the file's group does not match the process's effective or supplementary group IDs, the `S_ISGID` bit is silently cleared. PiperOrigin-RevId: 797579605
1 parent a7d1358 commit b4259c3

File tree

2 files changed

+62
-7
lines changed

2 files changed

+62
-7
lines changed

pkg/sentry/vfs/permissions.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ func MayWriteFileWithOpenFlags(flags uint32) bool {
185185

186186
// CheckSetStat checks that creds has permission to change the metadata of a
187187
// file with the given permissions, UID, and GID as specified by stat, subject
188-
// to the rules of Linux's fs/attr.c:setattr_prepare().
188+
// to the rules of Linux's fs/attr.c:setattr_prepare(). Might mutate `opts`.
189189
func CheckSetStat(ctx context.Context, creds *auth.Credentials, opts *SetStatOptions, mode linux.FileMode, kuid auth.KUID, kgid auth.KGID) error {
190190
stat := &opts.Stat
191191
if stat.Mask&linux.STATX_SIZE != 0 {
@@ -201,11 +201,13 @@ func CheckSetStat(ctx context.Context, creds *auth.Credentials, opts *SetStatOpt
201201
if !CanActAsOwner(creds, kuid) {
202202
return linuxerr.EPERM
203203
}
204-
// TODO(b/30815691): "If the calling process is not privileged (Linux:
205-
// does not have the CAP_FSETID capability), and the group of the file
206-
// does not match the effective group ID of the process or one of its
207-
// supplementary group IDs, the S_ISGID bit will be turned off, but
208-
// this will not cause an error to be returned." - chmod(2)
204+
// "If the calling process is not privileged (Linux: does not have the CAP_FSETID capability),
205+
// and the group of the file does not match the effective group ID of the process or one of its
206+
// supplementary group IDs, the S_ISGID bit will be turned off, but this will not cause an error
207+
// to be returned." - chmod(2)
208+
if stat.Mode&linux.S_ISGID != 0 && !creds.HasCapabilityOnFile(linux.CAP_FSETID, kuid, kgid) && !creds.InGroup(kgid) {
209+
stat.Mode &^= linux.S_ISGID
210+
}
209211
}
210212
if stat.Mask&linux.STATX_UID != 0 {
211213
if !((creds.EffectiveKUID == kuid && auth.KUID(stat.UID) == kuid) ||

test/syscalls/linux/chown.cc

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@
2323
#include "gtest/gtest.h"
2424
#include "absl/flags/flag.h"
2525
#include "absl/synchronization/notification.h"
26-
#include "test/util/capability_util.h"
2726
#include "test/util/file_descriptor.h"
2827
#include "test/util/fs_util.h"
28+
#include "test/util/linux_capability_util.h"
2929
#include "test/util/posix_error.h"
3030
#include "test/util/temp_path.h"
3131
#include "test/util/test_util.h"
@@ -127,6 +127,59 @@ TEST(ChownTest, FchownatEmptyPath) {
127127
ASSERT_THAT(fchownat(fd.get(), "", 0, 0, 0), SyscallFailsWithErrno(ENOENT));
128128
}
129129

130+
TEST(ChownTest, SetGroupIdBitDeniedWithoutCapFsetIdOrRelevantGroup) {
131+
SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SETGID)));
132+
const auto file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFile());
133+
AutoCapability cap(CAP_FSETID, false);
134+
135+
// Changing task creds in the main thread can mess with other tests.
136+
ScopedThread([&] {
137+
const mode_t chown_mode = 0755 | S_ISGID;
138+
const gid_t oddGid = 12345;
139+
struct stat stats;
140+
mode_t stat_mode;
141+
142+
// 1) Without CAP_FSETID, and no relevant group, S_ISGID is not applied.
143+
ASSERT_THAT(syscall(SYS_setresgid, oddGid, oddGid, oddGid),
144+
SyscallSucceeds());
145+
std::vector<gid_t> no_extra_groups;
146+
ASSERT_THAT(
147+
syscall(SYS_setgroups, no_extra_groups.size(), no_extra_groups.data()),
148+
SyscallSucceeds());
149+
150+
ASSERT_THAT(chmod(file.path().c_str(), chown_mode), SyscallSucceeds());
151+
ASSERT_THAT(stat(file.path().c_str(), &stats), SyscallSucceeds());
152+
stat_mode = stats.st_mode & ~S_IFMT;
153+
154+
EXPECT_EQ(stat_mode & S_ISGID, 0); // S_ISGID is not applied.
155+
EXPECT_EQ(stat_mode, chown_mode & ~S_ISGID); // but the rest is.
156+
157+
// 2) Without CAP_FSETID, but with a relevant group, S_ISGID is applied.
158+
gid_t file_gid = stats.st_gid;
159+
ASSERT_THAT(syscall(SYS_setresgid, file_gid, file_gid, file_gid),
160+
SyscallSucceeds());
161+
162+
ASSERT_THAT(chmod(file.path().c_str(), chown_mode), SyscallSucceeds());
163+
ASSERT_THAT(stat(file.path().c_str(), &stats), SyscallSucceeds());
164+
stat_mode = stats.st_mode & ~S_IFMT;
165+
166+
EXPECT_EQ(stat_mode & S_ISGID, S_ISGID); // S_ISGID is applied.
167+
EXPECT_EQ(stat_mode, chown_mode); // as is the rest.
168+
169+
// 3) With CAP_FSETID, but without a relevant group, S_ISGID is applied.
170+
AutoCapability cap_inner(CAP_FSETID, true);
171+
ASSERT_THAT(syscall(SYS_setresgid, oddGid, oddGid, oddGid),
172+
SyscallSucceeds());
173+
174+
ASSERT_THAT(chmod(file.path().c_str(), chown_mode), SyscallSucceeds());
175+
ASSERT_THAT(stat(file.path().c_str(), &stats), SyscallSucceeds());
176+
stat_mode = stats.st_mode & ~S_IFMT;
177+
178+
EXPECT_EQ(stat_mode & S_ISGID, S_ISGID); // S_ISGID is applied.
179+
EXPECT_EQ(stat_mode, chown_mode); // as is the rest.
180+
});
181+
}
182+
130183
using Chown =
131184
std::function<PosixError(const std::string&, uid_t owner, gid_t group)>;
132185

0 commit comments

Comments
 (0)