Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

client: Clear setuid bits when writing or truncating #12412

Merged
merged 5 commits into from
Dec 19, 2016
Merged

client: Clear setuid bits when writing or truncating #12412

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18e3f9c
test: add missing ceph_ll_close calls
jtlayton Dec 8, 2016
6b7e39e
client: drop correct caps when setting btime
jtlayton Dec 8, 2016
8338b94
test: flesh out the setuid/setgit clearing test
jtlayton Dec 8, 2016
2d1c91f
client/mds: have write codepath clear the setuid/setgid bits if they'…
jtlayton Dec 8, 2016
03076a6
client: set CEPH_SETATTR_KILL_SGUID on truncate
jtlayton Dec 8, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 25 additions & 8 deletions src/client/Client.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6494,7 +6494,9 @@ int Client::_do_setattr(Inode *in, struct ceph_statx *stx, int mask,
}

if (in->caps_issued_mask(CEPH_CAP_AUTH_EXCL)) {
bool kill_sguid = false;
bool kill_sguid = mask & CEPH_SETATTR_KILL_SGUID;

mask &= ~CEPH_SETATTR_KILL_SGUID;

if (mask & CEPH_SETATTR_UID) {
in->ctime = ceph_clock_now(cct);
Expand Down Expand Up @@ -6571,6 +6573,9 @@ int Client::_do_setattr(Inode *in, struct ceph_statx *stx, int mask,
req->set_filepath(path);
req->set_inode(in);

if (mask & CEPH_SETATTR_KILL_SGUID) {
req->inode_drop |= CEPH_CAP_AUTH_SHARED;
}
if (mask & CEPH_SETATTR_MODE) {
req->head.args.setattr.mode = stx->stx_mode;
req->inode_drop |= CEPH_CAP_AUTH_SHARED;
Expand All @@ -6586,6 +6591,10 @@ int Client::_do_setattr(Inode *in, struct ceph_statx *stx, int mask,
req->inode_drop |= CEPH_CAP_AUTH_SHARED;
ldout(cct,10) << "changing gid to " << stx->stx_gid << dendl;
}
if (mask & CEPH_SETATTR_BTIME) {
req->head.args.setattr.btime = utime_t(stx->stx_btime);
req->inode_drop |= CEPH_CAP_AUTH_SHARED;
}
if (mask & CEPH_SETATTR_MTIME) {
req->head.args.setattr.mtime = utime_t(stx->stx_mtime);
req->inode_drop |= CEPH_CAP_AUTH_SHARED | CEPH_CAP_FILE_RD |
Expand All @@ -6596,11 +6605,6 @@ int Client::_do_setattr(Inode *in, struct ceph_statx *stx, int mask,
req->inode_drop |= CEPH_CAP_FILE_CACHE | CEPH_CAP_FILE_RD |
CEPH_CAP_FILE_WR;
}
if (mask & CEPH_SETATTR_BTIME) {
req->head.args.setattr.btime = utime_t(stx->stx_btime);
req->inode_drop |= CEPH_CAP_FILE_CACHE | CEPH_CAP_FILE_RD |
CEPH_CAP_FILE_WR;
}
if (mask & CEPH_SETATTR_SIZE) {
if ((unsigned long)stx->stx_size < mdsmap->get_max_filesize()) {
req->head.args.setattr.size = stx->stx_size;
Expand Down Expand Up @@ -8829,9 +8833,22 @@ int Client::_write(Fh *f, int64_t offset, uint64_t size, const char *buf,
utime_t lat;
uint64_t totalwritten;
int have;
int r = get_caps(in, CEPH_CAP_FILE_WR, CEPH_CAP_FILE_BUFFER, &have, endoff);
if (r < 0) {
int r = get_caps(in, CEPH_CAP_FILE_WR|CEPH_CAP_AUTH_SHARED,
CEPH_CAP_FILE_BUFFER, &have, endoff);
if (r < 0)
return r;

/* clear the setuid/setgid bits, if any */
if (unlikely((in->mode & S_ISUID) ||
(in->mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))) {
struct ceph_statx stx = { 0 };

put_cap_ref(in, CEPH_CAP_AUTH_SHARED);
r = __setattrx(in, &stx, CEPH_SETATTR_KILL_SGUID, f->actor_perms);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is asynchronously sending off an MDS request and then doing the normal write. That does mean under nasty failure conditions we could send off the write, fail to send off the setattr, and then crash...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that's a synchronous call. do_setattr calls make_request, which appears to wait on the reply (like we would for any other setattr). Am I missing something there?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...wow, I'm not sure how I got that confused. You're right of course.

if (r < 0)
return r;
} else {
put_cap_ref(in, CEPH_CAP_AUTH_SHARED);
}

if (f->flags & O_DIRECT)
Expand Down
1 change: 1 addition & 0 deletions src/include/ceph_fs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ extern const char *ceph_mds_op_name(int op);
#endif
#define CEPH_SETATTR_MTIME_NOW (1 << 7)
#define CEPH_SETATTR_ATIME_NOW (1 << 8)
#define CEPH_SETATTR_KILL_SGUID (1 << 10)

/*
* Ceph setxattr request flags.
Expand Down
4 changes: 2 additions & 2 deletions src/mds/Server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3740,7 +3740,7 @@ void Server::handle_client_setattr(MDRequestRef& mdr)
__u32 access_mask = MAY_WRITE;

// xlock inode
if (mask & (CEPH_SETATTR_MODE|CEPH_SETATTR_UID|CEPH_SETATTR_GID|CEPH_SETATTR_BTIME))
if (mask & (CEPH_SETATTR_MODE|CEPH_SETATTR_UID|CEPH_SETATTR_GID|CEPH_SETATTR_BTIME|CEPH_SETATTR_KILL_SGUID))
xlocks.insert(&cur->authlock);
if (mask & (CEPH_SETATTR_MTIME|CEPH_SETATTR_ATIME|CEPH_SETATTR_SIZE))
xlocks.insert(&cur->filelock);
Expand Down Expand Up @@ -3800,7 +3800,7 @@ void Server::handle_client_setattr(MDRequestRef& mdr)

if (mask & CEPH_SETATTR_MODE)
pi->mode = (pi->mode & ~07777) | (req->head.args.setattr.mode & 07777);
else if ((mask & (CEPH_SETATTR_UID|CEPH_SETATTR_GID)) &&
else if ((mask & (CEPH_SETATTR_UID|CEPH_SETATTR_GID|CEPH_SETATTR_KILL_SGUID)) &&
S_ISREG(pi->mode)) {
pi->mode &= ~S_ISUID;
if ((pi->mode & (S_ISGID|S_IXGRP)) == (S_ISGID|S_IXGRP))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from the previous set of commits but I guess I didn't ask — why are we unconditionally removing S_ISUID and doing a check before removing S_ISGID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the S_ISGID bit set on a file that does not have the group execute bit is (oddly enough) used to indicate that the file is a candidate for mandatory locking. So you only want to clear that bit if the group execute bit is also set.

Expand Down
3 changes: 3 additions & 0 deletions src/test/libcephfs/recordlock.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,8 @@ static void thread_ConcurrentRecordLocking(str_ConcurrentRecordLocking& s) {
lock1.l_pid = getpid();
ASSERT_EQ(0, ceph_ll_setlk(cmount, fh, &lock1, pthread_self(), false));
PING_MAIN(7); // (7)

ASSERT_EQ(0, ceph_ll_close(cmount, fh));
}

// Used by ConcurrentRecordLocking test
Expand Down Expand Up @@ -746,6 +748,7 @@ static void process_ConcurrentRecordLocking(str_ConcurrentRecordLocking& s) {
ASSERT_EQ(0, ceph_ll_setlk(cmount, fh, &lock1, mypid, false));
PING_MAIN(7); // (7)

ASSERT_EQ(0, ceph_ll_close(cmount, fh));
CLEANUP_CEPH();

s.sem_destroy();
Expand Down
50 changes: 42 additions & 8 deletions src/test/libcephfs/test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1402,6 +1402,7 @@ TEST(LibCephFS, Nlink) {
ASSERT_EQ(ceph_ll_mkdir(cmount, root, dirname, 0755, &dir, &stx, 0, 0, perms), 0);
ASSERT_EQ(ceph_ll_create(cmount, dir, filename, 0666, O_RDWR|O_CREAT|O_EXCL,
&file, &fh, &stx, CEPH_STATX_NLINK, 0, perms), 0);
ASSERT_EQ(ceph_ll_close(cmount, fh), 0);
ASSERT_EQ(stx.stx_nlink, (nlink_t)1);

ASSERT_EQ(ceph_ll_link(cmount, file, dir, linkname, perms), 0);
Expand Down Expand Up @@ -1573,6 +1574,7 @@ TEST(LibCephFS, LazyStatx) {
ceph_ll_unlink(cmount1, root1, filename, perms1);
ASSERT_EQ(ceph_ll_create(cmount1, root1, filename, 0666, O_RDWR|O_CREAT|O_EXCL,
&file1, &fh, &stx, 0, 0, perms1), 0);
ASSERT_EQ(ceph_ll_close(cmount1, fh), 0);

ASSERT_EQ(ceph_ll_lookup_root(cmount2, &root2), 0);

Expand Down Expand Up @@ -1719,23 +1721,55 @@ TEST(LibCephFS, ClearSetuid) {
Fh *fh;
Inode *in;
struct ceph_statx stx;
const mode_t after_mode = S_IRWXU | S_IRWXG;
const mode_t before_mode = S_IRWXU | S_IRWXG | S_ISUID | S_ISGID;
const unsigned want = CEPH_STATX_UID|CEPH_STATX_GID|CEPH_STATX_MODE;
UserPerm *usercred = ceph_mount_perms(cmount);

ceph_ll_unlink(cmount, root, filename, ceph_mount_perms(cmount));
ASSERT_EQ(ceph_ll_create(cmount, root, filename, 06555,
O_RDWR|O_CREAT|O_EXCL, &in, &fh, &stx,
CEPH_STATX_UID|CEPH_STATX_GID|CEPH_STATX_MODE, 0,
ceph_mount_perms(cmount)), 0);
ceph_ll_unlink(cmount, root, filename, usercred);
ASSERT_EQ(ceph_ll_create(cmount, root, filename, before_mode,
O_RDWR|O_CREAT|O_EXCL, &in, &fh, &stx, want, 0,
usercred), 0);

ASSERT_EQ(stx.stx_mode & 07777, 06555);
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, before_mode);

// write
ASSERT_EQ(ceph_ll_write(cmount, fh, 0, 3, "foo"), 3);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, usercred), 0);
ASSERT_TRUE(stx.stx_mask & CEPH_STATX_MODE);
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, after_mode);

// reset mode
stx.stx_mode = before_mode;
ASSERT_EQ(ceph_ll_setattr(cmount, in, &stx, CEPH_STATX_MODE, usercred), 0);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, usercred), 0);
ASSERT_TRUE(stx.stx_mask & CEPH_STATX_MODE);
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, before_mode);

// truncate
stx.stx_size = 1;
ASSERT_EQ(ceph_ll_setattr(cmount, in, &stx, CEPH_SETATTR_SIZE, usercred), 0);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, usercred), 0);
ASSERT_TRUE(stx.stx_mask & CEPH_STATX_MODE);
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, after_mode);

// reset mode
stx.stx_mode = before_mode;
ASSERT_EQ(ceph_ll_setattr(cmount, in, &stx, CEPH_STATX_MODE, usercred), 0);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, usercred), 0);
ASSERT_TRUE(stx.stx_mask & CEPH_STATX_MODE);
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, before_mode);

// chown -- for this we need to be "root"
UserPerm *rootcred = ceph_userperm_new(0, 0, 0, NULL);
ASSERT_TRUE(rootcred);
stx.stx_uid++;
stx.stx_gid++;
ASSERT_EQ(ceph_ll_setattr(cmount, in, &stx, CEPH_SETATTR_UID|CEPH_SETATTR_GID, rootcred), 0);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, ceph_mount_perms(cmount)), 0);
ASSERT_EQ(ceph_ll_getattr(cmount, in, &stx, CEPH_STATX_MODE, 0, usercred), 0);
ASSERT_TRUE(stx.stx_mask & CEPH_STATX_MODE);
ASSERT_FALSE(stx.stx_mode & (S_ISUID|S_ISGID));
ASSERT_EQ(stx.stx_mode & (mode_t)ALLPERMS, after_mode);

ASSERT_EQ(ceph_ll_close(cmount, fh), 0);
ceph_shutdown(cmount);
}