SERVER-8623 Don't allow renameCollection to bypass auth checks on system namespaces

3 files changed, 99 insertions, 13 deletions

diff --git a/jstests/auth/renameSystemCollections.js b/jstests/auth/renameSystemCollections.js
new file mode 100644
index 00000000000..20296070bb3
--- /dev/null
+++ b/jstests/auth/renameSystemCollections.js
@@ -0,0 +1,87 @@
+// SERVER-8623: Test that renameCollection can't be used to bypass auth checks on system namespaces
+var conn = MongoRunner.runMongod({auth : ""});
+
+var adminDB = conn.getDB("admin");
+var testDB = conn.getDB("testdb");
+var testDB2 = conn.getDB("testdb2");
+
+testDB.addUser({user:'spencer',
+                pwd:'password',
+                roles:['readWrite']});
+
+adminDB.addUser({user:'userAdmin',
+                 pwd:'password',
+                 roles:['userAdminAnyDatabase']});
+
+var userAdminConn = new Mongo(conn.host);
+userAdminConn.getDB('admin').auth('userAdmin', 'password');
+userAdminConn.getDB('admin').addUser({user:'readWriteAdmin',
+                                      pwd:'password',
+                                      roles:['readWriteAnyDatabase']});
+
+
+// Test that a readWrite user can't rename system.profile to something they can read.
+testDB.auth('spencer', 'password');
+res = testDB.system.profile.renameCollection("profile");
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+
+
+// Test that a readWrite user can't rename system.users to something they can read.
+var res = testDB.system.users.renameCollection("users");
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+assert.eq(0, testDB.users.count());
+
+
+// Test that a readWrite user can't use renameCollection to override system.users
+testDB.users.insert({user:'backdoor',
+                     pwd:'hashedpassword',
+                     roles:'userAdmin'});
+res = testDB.users.renameCollection("system.users", true);
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+assert.eq(null, userAdminConn.getDB('testdb').system.users.findOne({user:'backdoor'}));
+
+
+// Test that a readWrite user can't create system.users using renameCollection
+adminDB.auth('readWriteAdmin', 'password');
+testDB2.users.insert({user:'backdoor',
+                      pwd:'hashedpassword',
+                      roles:'userAdmin'});
+res = testDB2.users.renameCollection("system.users");
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+assert.eq(0, userAdminConn.getDB('testdb2').system.users.count());
+
+
+// Test that you can't rename system.users across databases
+testDB2.users.drop();
+var res = adminDB.runCommand({renameCollection:'testdb.system.users', to:'testdb2.users'});
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+assert.eq(0, testDB2.users.count());
+
+
+// Test that a userAdmin can't rename system.users without readWrite
+testDB.users.drop();
+var res = userAdminConn.getDB('testdb').system.users.renameCollection("users");
+assert.eq(0, res.ok);
+assert.eq("unauthorized", res.errmsg);
+assert.eq(0, testDB.users.count());
+
+
+// Test that with userAdmin AND dbAdmin you CAN rename to/from system.users
+adminDB.auth('userAdmin', 'password');
+var res = testDB.system.users.renameCollection("users");
+assert.eq(1, res.ok);
+assert.eq(1, testDB.users.count());
+
+testDB.users.drop();
+testDB.users.insert({user:'newUser',
+                     pwd:'hashedPassword',
+                     roles:['readWrite']});
+var res = testDB.users.renameCollection("system.users");
+assert.eq(1, res.ok);
+assert.neq(null, testDB.system.users.findOne({user:'newUser'}));
+assert.eq(null, testDB.system.users.findOne({user:'spencer'}));
diff --git a/src/mongo/db/auth/authorization_manager.cpp b/src/mongo/db/auth/authorization_manager.cpp
index a163ad8b470..ab2f5cd26a4 100644
--- a/src/mongo/db/auth/authorization_manager.cpp
+++ b/src/mongo/db/auth/authorization_manager.cpp
@@ -763,7 +763,7 @@ namespace {
             newActions.removeAction(ActionType::update);
             newActions.removeAction(ActionType::remove);
             newActions.addAction(ActionType::userAdmin);
-        } else if (collectionName == "system.profile" && newActions.contains(ActionType::find)) {
+        } else if (collectionName == "system.profile") {
             newActions.removeAction(ActionType::find);
             newActions.addAction(ActionType::profileRead);
         } else if (collectionName == "system.indexes" && newActions.contains(ActionType::find)) {
diff --git a/src/mongo/db/commands/rename_collection_common.cpp b/src/mongo/db/commands/rename_collection_common.cpp
index 11f4cd5fc4b..51ca4bbe6fa 100644
--- a/src/mongo/db/commands/rename_collection_common.cpp
+++ b/src/mongo/db/commands/rename_collection_common.cpp
@@ -32,22 +32,21 @@ namespace rename_collection {
                                                   std::vector<Privilege>* out) {
         NamespaceString sourceNS = NamespaceString(cmdObj.getStringField("renameCollection"));
         NamespaceString targetNS = NamespaceString(cmdObj.getStringField("to"));
+        ActionSet sourceActions;
+        ActionSet targetActions;
+
         if (sourceNS.db == targetNS.db) {
-            ActionSet actions;
-            actions.addAction(ActionType::renameCollectionSameDB);
-            out->push_back(Privilege(sourceNS.db, actions));
-            return;
+            sourceActions.addAction(ActionType::renameCollectionSameDB);
+            targetActions.addAction(ActionType::renameCollectionSameDB);
+        } else {
+            sourceActions.addAction(ActionType::cloneCollectionLocalSource);
+            sourceActions.addAction(ActionType::dropCollection);
+            targetActions.addAction(ActionType::createCollection);
+            targetActions.addAction(ActionType::cloneCollectionTarget);
+            targetActions.addAction(ActionType::ensureIndex);
         }
 
-        ActionSet sourceActions;
-        sourceActions.addAction(ActionType::cloneCollectionLocalSource);
-        sourceActions.addAction(ActionType::dropCollection);
         out->push_back(Privilege(sourceNS.ns(), sourceActions));
-
-        ActionSet targetActions;
-        targetActions.addAction(ActionType::createCollection);
-        targetActions.addAction(ActionType::cloneCollectionTarget);
-        targetActions.addAction(ActionType::ensureIndex);
         out->push_back(Privilege(targetNS.ns(), targetActions));
     }