Missing permission check on default edit form

[thomasmassmann]

The default edit form for dexterity content is only protected by a view permission setting in ZCML with the Modify portal content permission. There is no additional check on the form itself before new data is applied to the content.

Individual field permissions applied to a schema are checked by the tagged value settings of plone.autoform.utils, but fields without a specific read or write permission are ignored.

There is aDXFieldPermissionChecker (using Modify portal content as default permission), but it is only called and used for the vocabulary view for AJAX requests.

Adding a permission check in the DefaultEditForm would help to secure the write operation:

class DefaultEditForm(DexterityExtensibleForm, form.EditForm):

    success_message = _(u"Changes saved")
    DEFAULT_PERMISSION = 'Modify portal content'

    @button.buttonAndHandler(_(u'Save'), name='save')
    def handleApply(self, action):
        data, errors = self.extractData()
        if errors:
            self.status = self.formErrorsMessage
            return

        # Additional check start
        checker = getSecurityManager().checkPermission
        if not checker(self.DEFAULT_PERMISSION, self.context):
            raise Unauthorized()
        # Additional check end

        self.applyChanges(data)
        IStatusMessage(self.request).addStatusMessage(
            self.success_message, "info"
        )
        self.request.response.redirect(self.nextURL())
        notify(EditFinishedEvent(self.context))

    def update(self):
        self.portal_type = self.context.portal_type
        super(DefaultEditForm, self).update()

        # Additional check start
        checker = getSecurityManager().checkPermission
        if not checker(self.DEFAULT_PERMISSION, self.context):
            raise Unauthorized()
        # Additional check end

        # fire the edit begun only if no action was executed
        if len(self.actions.executedActions) == 0:
            notify(EditBegunEvent(self.context))

If someone needs to customize the default edit form and requires a different permission, the DEFAULT_PERMISSION can be adjusted as well:

class MyCustomEditForm(DefaultEditForm):

    DEFAULT_PERMISSION = 'My custom permission'

[rafaelbco]

Why is the view permission not enough?

[thomasmassmann]

Imagine someone creates a custom edit form like so, maybe to adjust some form attributes:

  <browser:page
      class=".forms.MyEditForm"
      for="plone.dexterity.interfaces.IDexterityContent"
      layer="my.addon.interfaces.IMyAddonLayer"
      name="edit"
      permission="zope2.View"
      />

Do you notice the wrong permission? Now with that form anyone having zope2.View permission is able to edit the content (all dexterity content) and change it.

[thomasmassmann]

This also works with zope.Public. I tried and was able to edit private (not published) content as anonymous user, knowing the URL of course.

[rafaelbco]

The example code you provided is clear: you're overriding the edit view and changing it's permission. I don't see that as a problem.

I don't agree it's a bug.

[thomasmassmann]

So a badly chosen view permission is worth much more than any permissions set by roles, workflows etc? 🤦‍♂

[thomasmassmann]

So basically Plone is currently working like this:

Hey user, you don't have permission to access, view or modify the content based on your role and the current workflow status. But because you are on that page/form which only requires this very low permission, I give you full access to the content. Go ahead and change it.

🤡 😂 🤢 💣 💥

Rest easy, Plone has the best security track record of any Enterprise Content Management System

🚮

[datakurre]

@tmassman But it is an edit form. What is the use case of requiring a different permission to view the form than submit it?

[thomasmassmann]

There is no "usecase", but I see people over and over again copy&pasting code without checking and opening things like the edit form, because they wanted to customize some properties.

If someone really wants to make an edit form available for the general public (or some regular Plone users without further permissions) than that should be done explicitly by adjusting the permission in the view declaration (zcml), the view permission check (python code) and the corresponding workflow settings.

I've written ZTK applications with strict security policies, using security-proxied objects by default. Plone's security model currently feels like a big Swiss cheese. Here workflow settings and common agreements on a default edit permission (Modify portal content) are ignored and given in the hands of the permission setting on a single view.

[rafaelbco]

If someone really wants to make an edit form available for the general public (or some regular Plone users without further permissions) than that should be done explicitly by adjusting the permission in the view declaration (zcml), the view permission check (python code) and the corresponding workflow settings.

This is the main point of our disagreement. You're proposing that in order to change the permission of the edit form the developer must do 2 things: change the zcml and the python code. The rationale being the developer often makes mistakes when dealing with the zcml.

I think it's not a good idea. Permission settings should be defined in a single place. Besides that, this is not a pattern I've seen anywhere in Plone code.

[thomasmassmann]

So, things like:

• https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/actions.py#L149
• https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/contents/__init__.py#L117
• https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/contents/rename.py#L73
• https://github.com/plone/plone.app.content/blob/master/plone/app/content/browser/vocabulary.py#L214
• and many many more

are all irrelevant and the zcml permission settings are enough?

[mauritsvanrees]

So you say it is like this:

Hey user, you don't have permission to access, view or modify the content based on your role and the current workflow status. But because you are on that page/form which only requires this very low permission, I give you full access to the content. Go ahead and change it.

And you want to change it to:

Hey user, I have this nice form for you, you can submit it and then always get an error.

I am not saying that all checks that you mention are irrelevant. The first one checks a permission on the parent of a context, which you cannot express in ZCML. The other three go through a list of brains that the user can see, and filters out ones for which the user does not have a certain permission.
But checking the same permission twice, is superfluous.

[mauritsvanrees]

For the record, Thomas has responsibly and securely sent a message to the Plone security team before filing this bug. We discussed it, and think it is no security bug, and said he was free to discuss it in public, which he is doing now.

As a community, we can always decide that it is better to add more permission checks, even when it is not strictly necessary. For me, this is what this bug is about, and I welcome the discussion.

[rafaelbco]

But checking the same permission twice, is superfluous.

My opinion is the same.

The examples provided by @tmassman are of cases where the permission of the view is really not sufficient. One is checking for a permission in the parent object, others are checking in a collection of objects managed by the view. There's no way to express it in zcml, so the checks must be done in python code.

[jensens]

First, as @mauritsvanrees said, I agree this is not a security issue. But I tend to agree we can do better.

We have the global permission "Modify portal content". Only when this permission is given, attributes shall be modified. Except the field-level permission tells us something different. In fact, not only in the z3c.form add and edit, but elsewhere too.

Btw., this applies also to the field-level view permission. Goes to far?

I fear performance-implications are not on our side when adding this. All has its costs.