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

CommentFileSizeAuthor
#45 node-create-access-2744381-45-interdiff.txt1.47 KBBerdir
#45 node-create-access-2744381-45.patch5.91 KBBerdir
#31 node-create-access-2744381-31-interdiff.txt579 bytesBerdir
#31 node-create-access-2744381-31.patch5.82 KBBerdir
#27 interdiff-rebase.txt1.13 KBWim Leers
#27 node-create-access-2744381-27.patch5.2 KBWim Leers
#22 node-create-access-also-administer-nodes-2744381-22.patch978 bytesWim Leers
#21 node-create-access-2744381-21.patch5.42 KBWim Leers
#18 node-create-access-2744381-18-interdiff.txt1.13 KBBerdir
#18 node-create-access-2744381-18.patch5.06 KBBerdir
#16 node-create-access-2744381-16-interdiff.txt3.21 KBBerdir
#16 node-create-access-2744381-16.patch3.94 KBBerdir
#16 node-create-access-2744381-16-test-only.patch809 bytesBerdir
#14 node-create-access-2744381-14.patch744 bytesBerdir
#11 node-create-access-2744381-11.patch743 bytesBerdir
#6 interdiff-2744381-4-6.txt1.61 KBmohit_aghera
#6 2744381-node-add-access-check-6.patch4.93 KBmohit_aghera
#4 interdiff-2744381-2-4.txt3.83 KBmohit_aghera
#4 2744381-node-add-access-check-4.patch4.69 KBmohit_aghera
#2 2744381-node-add-access-check.patch880 byteslahoosascoots
Members fund testing for the Drupal project. Drupal Association Learn more

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 :) 🏝

Berdir’s picture

So much for "next week".

The patch looks fine to me, I think it was just confused about changed context in that test.

This should fix the date test fails.

dawehner’s picture

It feels like it is a tiny bit safer to use create date_content content instead of bypass node access. Do we care about that level of test coverage?

Berdir’s picture

THis would require quite some chances to those tests. Most tests actually use test entities, it's just some methods that even create that node type, and when the type doesn't exist yet, we can't create a user with that permision, so we'd have to get his role and update the role.

The test is about testing date widget, I don't think it's relevant what kind of access we grant there. Pretty sure we also wouldn't actually need administer content types permission, as we create the node type through the API.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Fair point, I was just afraid of accidentally loosing some sort of access test coverage, but I agree with your argument. This is not about node access test coverage, but rather purely about date module functionality.

Status: Reviewed & tested by the community » Needs work

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

Berdir’s picture

Status: Needs work » Reviewed & tested by the community

Random fail?

Status: Reviewed & tested by the community » Needs work

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

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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

Status: Needs work » Reviewed & tested by the community

Green again, back to RTBC.

Berdir’s picture

Also, leaving that for the maintainers to decide, but I think this is still fine for 8.4?

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @Berdir.

+++ b/core/modules/node/node.services.yml
@@ -16,6 +16,7 @@ services:
+    deprecated: The "%service_id%" service is deprecated. Use _entity_create_access or _entity_create_any_access access checks instead.

+++ b/core/modules/node/src/Access/NodeAddAccessCheck.php
@@ -12,6 +12,9 @@
+ * @deprecated in Drupal 8.3.0, will be removed before Drupal 9.0.0. Use
+ * _entity_create_access or _entity_create_any_access access checks instead.

We should reference the change record for this change here as well. (See examples at https://www.drupal.org/core/deprecation.)

Also I think we need to fix the "in Drupal 8.3.0"; it's at least 8.4.0 now. I would backport this up to the beta code freeze (so through Aug. 15) since this is a security hardening, but thereafter it's 8.5.x only since it's disruptive.

xjm’s picture

Oh, re: the datetime tests, I had the same question as @dawehner when reading the test, but when you look at the permissions list, it already grants everything and the kitchen sink permission-wise, so I think it's fine as it is in the patch.

xjm’s picture

Oh and also the @deprecated is missing its indentation for the second line.

xjm’s picture

Jersey/Guernsey

Readers might be interested to know that both Jersey and Guernsey are also breeds of dairy cow. I know this because I am from Wisconsin, which we have established is higher in per capita cow population than Switzerland, though a lower total. ;)

Berdir’s picture

Jersey/Guernsey

Readers might be interested to know that both Jersey and Guernsey are also breeds of dairy cow. I know this because I am from Wisconsin, which we have established is higher in per capita cow population than Switzerland, though a lower total. ;)

I would like to point out that I was *not*, at any time, *on* a cow, but only on the islands those cows are coming from. But yes, they're proud of their cows there ;)

Also, addressed the deprecetion messages, is it correct like that?