This module has a minor elevated access vulnerability which has been cleared by the security team for the public issue queue.

You can see this vulnerability by:
1. Creating a role with "Administer content types" permission but NOT create [content type] content or "Administer content".
2. Access /node/add/[content type]

The bug is in core/modules/node/src/Access/NodeAddAccessCheck.php:access


// If checking whether a node of a particular type may be created.
if ($account->hasPermission('administer content types')) {
return AccessResult::allowed()->cachePerPermissions();
}
if ($node_type) {
return $access_control_handler->createAccess($node_type->id(), $account, [], TRUE);
}

The permission for 'administer content types' is checked rather than the permission for 'administer nodes'. 'administer content types' allows access to change content types and NOT create content for any type of node.

Permissions reference source: https://www.drupal.org/node/132202

Comments

lahoosascoots created an issue. See original summary.

lahoosascoots’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 2744381-node-add-access-check.patch, failed testing.

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
4.69 KB
3.83 KB

Changing permissions in failing test cases

Status: Needs review » Needs work

The last submitted patch, 4: 2744381-node-add-access-check-4.patch, failed testing.

mohit_aghera’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
1.61 KB

Fixing few more permissions

Status: Needs review » Needs work

The last submitted patch, 6: 2744381-node-add-access-check-6.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

The reason this exists is that you can create content types with that permission, otherwise we would not allow access to node/add if there are no node types.

Having that permission but not being able to create any content and there being node types on the page seems like a pretty weird special case. You could create node types that you then can't create content for?

Also, administer nodes is definitely not better. If anything, it would need to be bypass, but then you never get to that point if there are node types.

\Drupal\Core\Entity\EntityCreateAnyAccessCheck::access() was added in a consistent way, on purpose. and it was discussed in #2696669: Incomplete/broken access checking in EntityController::addPage().

Note that you can *not* create any content. You simply see the page with a link to create a new type, which is exactly what this page allows.

Works as designed IMHO

effulgentsia’s picture

Note that you can *not* create any content. You simply see the page with a link to create a new type, which is exactly what this page allows.

Huh? According to the IS, you *can* visit and submit the /node/add/[content type] page with merely the "Administer content types" permission and no other permission. I tried this just now and found it to be true as well. This does mean that someone with just "Administer content types" permission is able to create a node of any type. I don't know if that's by design or not, but there's nothing in the name or description of that permission that communicates that.

Berdir’s picture

Status: Needs work » Needs review
FileSize
743 bytes

Oh wow, you are right. node/add works correctly and shows that you are not allowed to create any types, but if you know how they are named, then you can acess them.

This might be a regression from #2431281: Drop support for _access_mode routes and always assume ALL which added the content types check to \Drupal\node\Access\NodeAddAccessCheck::access() but doesn't limit it to the non-content type specific case. I think it should be below that if ($node_type) if.

Lets see if using the generic implementation works or results in test fails, we might also want to fix the access checks, but IMHO we probably want to deprecate them in favor of the generic ones anyway.

dawehner’s picture

Issue tags: +Needs tests

Interesting find! I totally agree with @berdir and @effulgentsia that this is NOT the expected behaviour.

This might be a regression from #2431281: Drop support for _access_mode routes and always assume ALL which added the content types check to \Drupal\node\Access\NodeAddAccessCheck::access() but doesn't limit it to the non-content type specific case. I think it should be below that if ($node_type) if.

I think this analysis is totally right. We simply didn't thought about the fact that this class is used in multiple places.

To get this issue done we IMHO though need tests :)

Status: Needs review » Needs work

The last submitted patch, 11: node-create-access-2744381-11.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
744 bytes

Oops, was missing an _.

Status: Needs review » Needs work

The last submitted patch, 14: node-create-access-2744381-14.patch, failed testing.

Berdir’s picture

Ok, so we have a bunch of tests failing, fixed those, often happens because they create new node types and don't add permission for them. I just added the bypass permission.

Also added a new test that should fail on HEAD.

Still need to deprecate the node specific access check. Won't be very visible and I guess needs a change record, so maybe a separate issue?

The last submitted patch, 16: node-create-access-2744381-16-test-only.patch, failed testing.

Berdir’s picture

cilefen’s picture

Title: Improper access check for creating nodes » NodeAddAccessCheck allows roles holding the "Administer content types" permission to create nodes
Priority: Major » Normal

The core committers and the entity team discussed this issue at a recent triage meeting. It was decided to downgrade this issue to normal priority. The reasoning behind the decision is that Administer content types is a restricted access permission that provides many ways to change the configuration of a site. The fact that a role holding Administer content types could add content is not significant enough to raise the issue priority.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

I ran into this too today. The "Add content" action at /admin/content does NOT show up when the user has the administer nodes permission. Fixing NodeAddAccessCheck fixes it.

Rebased this patch… only to find out that this still does not solve that. :/

Wim Leers’s picture

This change fixes my use case. I think it ought to be integrated into this patch/issue.

(And wow, I never knew we had so many similar things: bypass node access, administer nodes, et cetera!)

The last submitted patch, 21: node-create-access-2744381-21.patch, failed testing. View results

Berdir’s picture

I don't think that is correct. Administer nodes does not allow create access. It only gives access to a bunch of node fields like author and status.

Yes, It is a very weird permission, basically all the left-overs after splitting bypass and access overview away.

Wim Leers’s picture

:O That was not clear to me at all.

So how does one allow a user to CRUD all nodes, and allow to create nodes of any bundle, without granting administer content types, which also allows changing field configuration? It sounds like there's no single permission for that at all?

Berdir’s picture

That's bypass node access

Wim Leers’s picture

Oh wow. This is … very confusing :( Thanks for clarifying, @Berdir!

So, my patch in #22 should be ignored.

We should continue with #18, which I rerolled in #21, but I failed to resolve one of the conflicts. Resolved that last conflict. (That's the least I could do in exchange for your guidance, @Berdir!)

Status: Needs review » Needs work

The last submitted patch, 27: node-create-access-2744381-27.patch, failed testing. View results

Berdir’s picture

No problem, I'll take a look at this next week, currently enjoying the amazing weather on Jersey/Guernsey for another week :)

Wim Leers’s picture

WOOOT, nice! Enjoy :) 🏝