Repository: plone/plone.restapi
Issue category: Security Vulnerability

Issue:
Privilege escalation in @sharing endpoint (PloneHotfix20200121)
- https://github.com/plone/Products.CMFPlone/issues/3021
- https://plone.org/security/hotfix/20200121/privilege-escalation-when-plone-restapi-is-installed

`plone.restapi.deserializer.local_roles.DeserializeFromJson` has a weakness.
This endpoint was introduced in plone.restapi 1.0a10 (2017-03-22).

Please apply the patch from `restapi_local_roles.py` from the hotfix. Should be this, but please check:

```
--- a/PloneHotfixNext/Products/PloneHotfixNext/restapi_local_roles.py
+++ b/PloneHotfixNext/Products/PloneHotfixNext/restapi_local_roles.py
@@ -36,11 +36,17 @@ if DeserializeFromJson is not None:
         # roles
         roles_reindex = False
         new_roles = data.get("entries", None)
+        managed_roles = frozenset([r['id'] for r in sharing_view.roles()])
+
         if new_roles is not None:
             # the roles are converted into a FrozenSet so we have to filter
             # the data structure we get.
             for user in new_roles:
                 roles_list = [key for key in user["roles"] if user["roles"][key]]
+
+                # Limit roles to ones the user is allowed to delegate
+                roles_list = set(roles_list).intersection(managed_roles)
+
                 user["roles"] = roles_list
             roles_reindex = sharing_view.update_role_settings(new_roles, reindex=False)
```

@lukasgraf since you and Niklaus discovered this, could you take care of this?


Hints:

Patch:
diff --git a/news/857.bugfix b/news/857.bugfix
new file mode 100644
index 0000000000..acfdabf048
--- /dev/null
+++ b/news/857.bugfix
@@ -0,0 +1,2 @@
+Sharing POST: Limit roles to ones the user is allowed to delegate.
+[lgraf]
\ No newline at end of file
diff --git a/src/plone/restapi/deserializer/local_roles.py b/src/plone/restapi/deserializer/local_roles.py
index 73540a6831..68a2ec804a 100644
--- a/src/plone/restapi/deserializer/local_roles.py
+++ b/src/plone/restapi/deserializer/local_roles.py
@@ -47,11 +47,17 @@ def __call__(self):
         # roles
         roles_reindex = False
         new_roles = data.get("entries", None)
+        managed_roles = frozenset([r['id'] for r in sharing_view.roles()])
+
         if new_roles is not None:
             # the roles are converted into a FrozenSet so we have to filter
             # the data structure we get.
             for user in new_roles:
                 roles_list = [key for key in user["roles"] if user["roles"][key]]
+
+                # Limit roles to ones the user is allowed to delegate
+                roles_list = set(roles_list).intersection(managed_roles)
+
                 user["roles"] = roles_list
             roles_reindex = sharing_view.update_role_settings(new_roles, reindex=False)
 


Test patch:
diff --git a/src/plone/restapi/tests/test_content_local_roles.py b/src/plone/restapi/tests/test_content_local_roles.py
index a26e6f3877..bd27f9f48a 100644
--- a/src/plone/restapi/tests/test_content_local_roles.py
+++ b/src/plone/restapi/tests/test_content_local_roles.py
@@ -6,6 +6,8 @@
 from plone.app.testing import SITE_OWNER_NAME
 from plone.app.testing import SITE_OWNER_PASSWORD
 from plone.app.testing import TEST_USER_ID
+from plone.app.testing import TEST_USER_NAME
+from plone.app.testing import TEST_USER_PASSWORD
 from plone.restapi.serializer.local_roles import SerializeLocalRolesToJson
 from plone.restapi.testing import PLONE_RESTAPI_DX_FUNCTIONAL_TESTING
 from Products.CMFCore.utils import getToolByName
@@ -265,6 +267,129 @@ def test_set_local_roles_for_user(self):
             ],
         )
 
+    def test_may_only_manage_roles_already_held(self):
+        # Grant Editor role to our test user (which gives them the required
+        # "plone.DelegateRoles" permission to manage local roles at all)
+        api.user.grant_roles(username=TEST_USER_ID, obj=self.portal.folder1,
+                             roles=['Editor'])
+        transaction.commit()
+
+        # Guard assertion - our test user starts with a limited set of roles
+        existing_roles = api.user.get_roles(username=TEST_USER_ID,
+                                            obj=self.portal.folder1)
+        self.assertEqual(
+            sorted(['Member', 'Authenticated', 'Editor']),
+            sorted(existing_roles))
+
+        # Attempt to gain additional roles not already held
+        response = requests.post(
+            self.portal.folder1.absolute_url() + "/@sharing",
+            headers={"Accept": "application/json"},
+            auth=(TEST_USER_NAME, TEST_USER_PASSWORD),
+            json={
+                "entries": [
+                    {
+                        u"id": TEST_USER_ID,
+                        u"roles": {
+                            u"Contributor": True,
+                            u"Editor": True,
+                            u"Reader": True,
+                            u"Publisher": True,
+                            u"Reviewer": True,
+                            u"Manager": True,
+                        },
+                        u"type": u"user",
+                    }
+                ]
+            },
+        )
+
+        transaction.commit()
+
+        self.assertEqual(response.status_code, 204)
+        new_roles = api.user.get_roles(username=TEST_USER_ID,
+                                       obj=self.portal.folder1)
+
+        # New roles should not contain any new roles that the user didn't
+        # have permission to delegate.
+        self.assertNotIn(u'Manager', new_roles)
+        self.assertNotIn(u'Publisher', new_roles)
+        self.assertNotIn(u'Reviewer', new_roles)
+        self.assertNotIn(u'Contributor', new_roles)
+
+        # 'Reader' gets added because the permission to delegate it is
+        # assigned to 'Editor' by default (see p.a.workflow.permissions)
+        self.assertEqual(
+            sorted(['Member', 'Authenticated', 'Editor', 'Reader']),
+            sorted(new_roles))
+
+    def test_unmanaged_existing_roles_are_retained_on_update(self):
+        """Make sure that existing roles don't get dropped when a user that
+        doesn't manage that roles updates local roles for another user that
+        already holds that role.
+        """
+        # Create another user that holds the Reviewer role, which is not
+        # managed by our test user
+        api.user.create(username='peter', email='peter@example.org',
+                        password='secret', roles=('Member', ))
+
+        api.user.grant_roles(username='peter', obj=self.portal.folder1,
+                             roles=['Reviewer'])
+        transaction.commit()
+
+        peters_existing_roles = api.user.get_roles(username='peter',
+                                                   obj=self.portal.folder1)
+        self.assertEqual(sorted(['Member', 'Reviewer', 'Authenticated']),
+                         sorted(peters_existing_roles))
+
+        # Grant Editor role to our test user (which gives them the required
+        # "plone.DelegateRoles" permission to manage local roles at all)
+        api.user.grant_roles(username=TEST_USER_ID, obj=self.portal.folder1,
+                             roles=['Editor'])
+        transaction.commit()
+
+        # Guard assertion - our test user doesn't have/manage Reviewer
+        existing_roles = api.user.get_roles(username=TEST_USER_ID,
+                                            obj=self.portal.folder1)
+        self.assertEqual(
+            sorted(['Member', 'Authenticated', 'Editor']),
+            sorted(existing_roles))
+
+        # Test user now gives Editor to peter. This should not lead to
+        # peter losing the Reviewer role.
+        response = requests.post(
+            self.portal.folder1.absolute_url() + "/@sharing",
+            headers={"Accept": "application/json"},
+            auth=(TEST_USER_NAME, TEST_USER_PASSWORD),
+            json={
+                "entries": [
+                    {
+                        u"id": "peter",
+                        u"roles": {
+                            u"Contributor": False,
+                            u"Editor": True,
+                            u"Reader": True,
+                            u"Publisher": False,
+                            u"Reviewer": True,
+                            u"Manager": False,
+                        },
+                        u"type": u"user",
+                    }
+                ]
+            },
+        )
+
+        transaction.commit()
+
+        self.assertEqual(response.status_code, 204)
+        new_roles = api.user.get_roles(username='peter',
+                                       obj=self.portal.folder1)
+
+        self.assertIn(u'Reviewer', new_roles)
+        self.assertEqual(
+            sorted(['Member', 'Authenticated', 'Editor', 'Reader', 'Reviewer']),
+            sorted(new_roles))
+
     def test_unset_local_roles_for_user(self):
         api.user.grant_roles(
             username=TEST_USER_ID, obj=self.portal.folder1, roles=["Reviewer", "Reader"]