silverstripe-terraformers / silverstripe-embargo-expiry

BSD 3-Clause "New" or "Revised" License
7 stars 7 forks source link

Fallback to SiteTree permissions to prevent privilege escalation #34

Closed caffeineinc closed 6 years ago

caffeineinc commented 6 years ago

If your site has a SiteTree extension extending canEdit and returning true then the default SiteTree Permissions are never called, and the user is always granted permission.

Background: SiteTree::canEdit() currently doesn't check the normal SiteTree edit permissions if any extension decides on whether the user can edit the page or not. https://github.com/silverstripe/silverstripe-cms/blob/4/code/Model/SiteTree.php#L1279

It supports three values for extensions implementing canEdit: true, false and null. In the current format, true basically means "ignore the normal SiteTree permission scheme”, which I don’t think is the intention, general permission practice is block until allowed.

DataObject::extendedCan() processes the results from all extensions of which null results are filtered out and the lowest of true and false values (always false unless all are true) is returned. This means if an extension returns true, and no other extension returns false, then it automatically overrides the default permissions. https://github.com/silverstripe/silverstripe-framework/blob/4/src/ORM/DataObject.php#L2782

Impact:

Users could have edit permissions because default and inherited permissions are only checked after extensions are processed, and extensions might return true (give permissions regardless) instead of null (don’t affect the outcome). If a user was able to login to the system, they could edit any page they had if all extensions applied returned true.