🇳🇱Netherlands @dokumori

I ran into the same issue. Although the wording is different, I opened an issue in their issue queue:: https://github.com/ckeditor/ckeditor5/issues/15408

I was able to reproduce the issue on their demo page so it has to do with CKEditor5, not Durpal. Maybe this ticket can be closed (won't fix), but I'll let the maintainers decide.

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

Comment by catch:

This is more of a data integrity issue than a security issue - the allowed providers list is for new content, not existing content.

dokumori → created an issue.

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issueAdding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the original issue

Adding a tag and a list of contributors in the private issue

Adding a tag and a list of contributors in the private issue

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

Setting this back to active, as it's been 14 days since a DM was sent to the current maintainer and no responses have been received.

Replacing the information on extended support with information on certified migration partners. This change has been reviewed and signed off within the Drupal Security Team.

larowlan → credited dokumori → .

dokumori → created an issue.

comment by @berdir

Considerable parts of this fix have been part of a public issues since 2018: https://www.drupal.org/node/2155787 → .

However, the public patch there does more things that I think are questionable and I think are not necessary, but I'm not sure how to move forward with that public issue. Focusing on this problem there would basically put even more focus on those parts and be the same as fixing this publicly. This also has tests that would be nice to have.

AFAIK, worst case scenario is that it allows someone to change or add translations of an entity in a language that they are not meant to. That's definitely a bug, but a security issue? I personally don't think so. We could just make it public?

dokumori → created an issue.

Comment and patch by @alexpott

I can confirm that this is an issue. There's a race in \Drupal\user\Controller\UserController::resetPassLogin() - in between loading the user and then updating the user in user_login_finalize(). In order to fix this we need to surround this with a lock.

I think we might decide that this issue can be a public security improvement because whilst the race should be avoided I'm not sure its existence is a security issue per se. In order to leverage it you'd need to obtain a one time login link and if you can obtain it, eg from a MITM attack, and enter the race then there's a chance you'll be first and therefore will win regardless of this fix. This fix means that you have to be first instead of a close second.

dokumori → created an issue.

comment by @catch:

https://www.drupal.org/project/drupal/issues/3304421 🌱 Make pre-save entity validation more commonplace (e.g., the default) Active and https://www.drupal.org/project/drupal/issues/2839195 📌 Add a method to access the original property Needs work are both related somewhat.

I think this is fine to discuss in the public queue, lack of/wrong entity validation is a bug but not a security issue. I'm not sure what specifically from here needs to be moved into the core queue though - is it an issue against the JSON:API component?

dokumori → created an issue.

Comment by @catch:

I think this should be a public hardening issue, it's a core API 'quirk' with the node module, which causes information disclosure issues in contrib or custom modules in some circumstances due to inconsistent behaviour.

https://www.drupal.org/project/drupal/issues/3309831 🐛 Add base_table to entity query metadata Needs work is an existing public issue dealing with node_query_access_alter() oddities.

We also have https://www.drupal.org/project/drupal/issues/777578 ✨ Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() Needs work open in the public queue for trying to replace the whole thing.

dokumori → created an issue.

@catch's comment:

The potential to replace deleted files seems very low impact and complex for an attacker to implement, for me it is a straight race condition/data integrity issue from that standpoint. Otherwise any race condition + data integrity issue would have to be handled in private.

This is likely to require a database update, so it is something I think should be done in public, and only in a minor release of core.

It's also mitigated by this 8.4.0 change that stops the automatic deletion of files with 0 usage (because file_usage was too unreliable, so files in use kept getting deleted at random).

https://www.drupal.org/node/2891902 →

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

dokumori → created an issue.

Comments by @srees:

---
We are converting as many of our tables as possible to support mb, and also adjusting the code in node.module's node_save function to prevent future issues that may arise:
~~~~~~~
if ($node->is_new) {
/*
* Database insertion errors with either revisions or the node tables cascade into every
* module that hooks into node_insert, and finally the node_access table
* via the call to node_access_acquire_grants($node) below, where we may end
* up with new rows where nid=0, which grants anonymous permission to view
* all nodes protected by a node_access module.
*/
$success = FALSE;//We want to fail safe
_node_save_revision($node, $user->uid);
//when creating a node, we should *always* have a vid, or there was a DB error
if($node->vid) {//do not attempt to write to node if the vid is missing
$success = drupal_write_record('node', $node);//if this works, $success = 1
}
if(!$success){
watchdog('node', 'Failed to insert node: ' . db_error(), [], WATCHDOG_ERROR);
drupal_set_message('Failed to insert node, please contact an administrator for assistance.');
if($node->vid) {//if the revision saved but the node didn't, we should remove the revision record
db_query('DELETE FROM {node_revisions} WHERE vid = %d', $node->vid);
}
drupal_goto('');
exit();
}
db_query('UPDATE {node_revisions} SET nid = %d WHERE vid = %d', $node->nid, $node->vid);
$op = 'insert';
}
~~~~~~~~~~~~
---

Further note: settings.php $db_collation is not set. Setting it to $db_collation = 'utf8mb4_unicode_520_ci'; prevents the issue as the database will accept that (regardless if the mysql collation is actually the same). Outside of my realm of expertise how that all works.

---

Comments by @dsnopek

I was able to partially reproduce this on D6.

I was testing with the 'nodeaccess' module, using a content type that disallowed anonymous users. On inserting a new node that had an emoji in the body, it would fail to fully create the node: a row is added to the node table with vid = 0, but no row is added to the node_revisions table. This meant the node was broken, and couldn't be viewed by anyone. However, the entry on the 'node_access' table looks correct, so there's actually no security implication.

Perhaps the security part of this comes from the way that 'og_access' works?

In any case, this is definitely a bug: the node shouldn't be considered successfully created if the row can't be added to the node_revisions table, and it should abort early.

---

Comments by @srees:

I think what may be different in the og_access case is that og_access does not create an entry in the node_access table for all cases. If the node being generated is not within its realm, it will not generate a node_access row, and then Drupal defaults to create a row on its own for node=0.

I believe og_access is correct in not defining an access row if the node is not under its control.

dokumori → created an issue.