From 614e4cf5a485d51861e49f17ecd194944b281d42 Mon Sep 17 00:00:00 2001 From: Sage Weil Date: Thu, 3 Sep 2015 12:21:47 -0400 Subject: [PATCH] mds: fix chown/chgrp check and tests Signed-off-by: Sage Weil --- src/mds/MDSAuthCaps.cc | 19 ++++++++++++++++- src/mds/MDSAuthCaps.h | 3 ++- src/mds/SessionMap.cc | 14 ++++-------- src/mds/SessionMap.h | 3 ++- src/test/libcephfs/access.cc | 24 +++++++++++++++++++++ src/test/mds/TestMDSAuthCaps.cc | 38 ++++++++++++++++----------------- 6 files changed, 69 insertions(+), 32 deletions(-) diff --git a/src/mds/MDSAuthCaps.cc b/src/mds/MDSAuthCaps.cc index c94755601621..483e0a18774d 100644 --- a/src/mds/MDSAuthCaps.cc +++ b/src/mds/MDSAuthCaps.cc @@ -137,7 +137,8 @@ bool MDSAuthCaps::is_capable(const std::string &inode_path, uid_t inode_uid, gid_t inode_gid, unsigned inode_mode, uid_t caller_uid, gid_t caller_gid, - unsigned mask) const + unsigned mask, + uid_t new_uid, gid_t new_gid) const { if (cct) ldout(cct, 10) << __func__ << " inode(path /" << inode_path @@ -145,6 +146,7 @@ bool MDSAuthCaps::is_capable(const std::string &inode_path, << " mode 0" << std::oct << inode_mode << std::dec << ") by caller " << caller_uid << ":" << caller_gid << " mask " << mask + << " new " << new_uid << ":" << new_gid << " cap: " << *this << dendl; for (std::vector::const_iterator i = grants.begin(); @@ -165,6 +167,21 @@ bool MDSAuthCaps::is_capable(const std::string &inode_path, continue; } + // chown/chgrp + if (mask & MAY_CHOWN) { + if (new_uid != caller_uid || // you can't chown to someone else + inode_uid != caller_uid) { // you can't chown from someone else + continue; + } + } + if (mask & MAY_CHGRP) { + // you can only chgrp *to* one of your groups... if you own the file. + if (inode_uid != caller_uid || + std::find(i->match.gids.begin(), i->match.gids.end(), new_gid) == + i->match.gids.end()) { + continue; + } + } if (inode_uid == caller_uid) { if ((!(mask & MAY_READ) || (inode_mode & S_IRUSR)) && diff --git a/src/mds/MDSAuthCaps.h b/src/mds/MDSAuthCaps.h index 88beeff1e6fd..7f361ecaefdd 100644 --- a/src/mds/MDSAuthCaps.h +++ b/src/mds/MDSAuthCaps.h @@ -111,7 +111,8 @@ public: bool allow_all() const; bool is_capable(const std::string &inode_path, uid_t inode_uid, gid_t inode_gid, unsigned inode_mode, - uid_t uid, gid_t gid, unsigned mask) const; + uid_t uid, gid_t gid, unsigned mask, + uid_t new_uid, gid_t new_gid) const; friend std::ostream &operator<<(std::ostream &out, const MDSAuthCaps &cap); }; diff --git a/src/mds/SessionMap.cc b/src/mds/SessionMap.cc index 7f6b016ea721..e2034f89f2ca 100644 --- a/src/mds/SessionMap.cc +++ b/src/mds/SessionMap.cc @@ -837,7 +837,7 @@ void Session::decode(bufferlist::iterator &p) bool Session::check_access(CInode *in, unsigned mask, int caller_uid, int caller_gid, - int setattr_uid, int setattr_gid) + int new_uid, int new_gid) { string path; CInode *diri = in->get_parent_inode(); @@ -857,21 +857,15 @@ bool Session::check_access(CInode *in, unsigned mask, if (!(auth_caps.is_capable(path, caller_uid, caller_gid, 0 /* irrelevant */, caller_uid, caller_gid, - MAY_CREATE))) { + MAY_CREATE, 0, 0))) { return false; } mask &= ~MAY_CREATE; } - if ((mask & (MAY_CHOWN|MAY_CHGRP)) && - !(auth_caps.is_capable(path, in->inode.uid, in->inode.gid, in->inode.mode, - caller_uid, caller_gid, mask))) { - return false; - } - - // normal unix MAY_{READ,WRITE} checks if (auth_caps.is_capable(path, in->inode.uid, in->inode.gid, in->inode.mode, - caller_uid, caller_gid, mask)) { + caller_uid, caller_gid, mask, + new_uid, new_gid)) { return true; } return false; diff --git a/src/mds/SessionMap.h b/src/mds/SessionMap.h index 6a2a840b635b..5dc459a9d6b5 100644 --- a/src/mds/SessionMap.h +++ b/src/mds/SessionMap.h @@ -301,7 +301,8 @@ public: completed_requests_dirty = false; } - bool check_access(CInode *in, unsigned mask, int caller_uid, int caller_gid, int setattr_uid, int setattr_gid); + bool check_access(CInode *in, unsigned mask, int caller_uid, int caller_gid, + int new_uid, int new_gid); Session() : diff --git a/src/test/libcephfs/access.cc b/src/test/libcephfs/access.cc index 7dc8e4bc49cc..5dfacea767db 100644 --- a/src/test/libcephfs/access.cc +++ b/src/test/libcephfs/access.cc @@ -303,6 +303,30 @@ TEST(AccessTest, User) { ASSERT_EQ(0, ceph_chown(admin, dir.c_str(), 123, 456)); ASSERT_EQ(-EACCES, ceph_mkdir(cmount, string(dir + "/no").c_str(), 0755)); + // chown and chgrp + ASSERT_EQ(0, ceph_chmod(admin, dir.c_str(), 0700)); + ASSERT_EQ(0, ceph_chown(admin, dir.c_str(), 123, 456)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), 123, 789)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), 123, 456)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), -1, 789)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), -1, 456)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 123, 1)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 1, 456)); + + ASSERT_EQ(0, ceph_chown(admin, dir.c_str(), 1, 1)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 123, 456)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 123, -1)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), -1, 456)); + + ASSERT_EQ(0, ceph_chown(admin, dir.c_str(), 1, 456)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 123, 456)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), 123, -1)); + ASSERT_EQ(-EACCES, ceph_chown(cmount, dir.c_str(), -1, 456)); + + ASSERT_EQ(0, ceph_chown(admin, dir.c_str(), 123, 1)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), -1, 456)); + ASSERT_EQ(0, ceph_chown(cmount, dir.c_str(), 123, 789)); + ceph_shutdown(cmount); // clean up diff --git a/src/test/mds/TestMDSAuthCaps.cc b/src/test/mds/TestMDSAuthCaps.cc index 0cd0e8ef0936..49d91f21c03f 100644 --- a/src/test/mds/TestMDSAuthCaps.cc +++ b/src/test/mds/TestMDSAuthCaps.cc @@ -110,37 +110,37 @@ TEST(MDSAuthCaps, AllowAll) { ASSERT_TRUE(cap.parse(g_ceph_context, "allow *", NULL)); ASSERT_TRUE(cap.allow_all()); - ASSERT_TRUE(cap.is_capable("foo/bar", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); + ASSERT_TRUE(cap.is_capable("foo/bar", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); } TEST(MDSAuthCaps, AllowUid) { MDSAuthCaps cap; ASSERT_TRUE(cap.parse(g_ceph_context, "allow * uid=10", NULL)); ASSERT_FALSE(cap.allow_all()); - ASSERT_TRUE(cap.is_capable("foo", 0, 0, 0777, 10, 0, MAY_READ | MAY_WRITE)); - ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, -1, 0, MAY_READ | MAY_WRITE)); - ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); - ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0775, 10, 10, MAY_READ)); - ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_WRITE)); - ASSERT_TRUE(cap.is_capable("foo", 10, 10, 0755, 10, 10, MAY_WRITE)); - ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 10, MAY_READ|MAY_CREATE)); - ASSERT_FALSE(cap.is_capable("foo", 10, 10, 0755, 0, 0, MAY_READ)); - ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_READ)); - ASSERT_FALSE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_READ|MAY_CREATE)); - ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0557, 10, 10, MAY_READ)); - ASSERT_TRUE(cap.is_capable("foo", 0, 0, 0557, 10, 10, MAY_READ)); - ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0557, 10, 10, MAY_CREATE)); - ASSERT_FALSE(cap.is_capable("foo", 10, 10, 0577, 10, 10, MAY_WRITE)); + ASSERT_TRUE(cap.is_capable("foo", 0, 0, 0777, 10, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, -1, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0775, 10, 10, MAY_READ, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_WRITE, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 10, 10, 0755, 10, 10, MAY_WRITE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 10, MAY_READ|MAY_CREATE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 10, 10, 0755, 0, 0, MAY_READ, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_READ, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 10, 0777, 10, 10, MAY_READ|MAY_CREATE, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 0, 10, 0557, 10, 10, MAY_READ, 0, 0)); + ASSERT_TRUE(cap.is_capable("foo", 0, 0, 0557, 10, 10, MAY_READ, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0557, 10, 10, MAY_CREATE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 10, 10, 0577, 10, 10, MAY_WRITE, 0, 0)); } TEST(MDSAuthCaps, AllowPath) { MDSAuthCaps cap; ASSERT_TRUE(cap.parse(g_ceph_context, "allow * path=/sandbox", NULL)); ASSERT_FALSE(cap.allow_all()); - ASSERT_TRUE(cap.is_capable("sandbox/foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); - ASSERT_TRUE(cap.is_capable("sandbox", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); - ASSERT_FALSE(cap.is_capable("sandboxed", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); - ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE)); + ASSERT_TRUE(cap.is_capable("sandbox/foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_TRUE(cap.is_capable("sandbox", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_FALSE(cap.is_capable("sandboxed", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); + ASSERT_FALSE(cap.is_capable("foo", 0, 0, 0777, 0, 0, MAY_READ | MAY_WRITE, 0, 0)); } TEST(MDSAuthCaps, OutputParsed) { -- 2.47.3