Contrib.social

Multi-byte characters break node access controls

Open on Drupal.org →
Created on 7 May 2023, over 1 year ago

Originally reported to the Drupal security team by srees → on 14 October 2020. Disclosing the information on the vulnerability as per psa-2022-03-09 → .

---

Drupal 6 has a security/information disclosure vulnerability for sites that use a node access module and which have not enabled multibyte support in the database. This very likely also impacts D7 based on code review (untested). I have not reviewed D8 for this.

If a user enters a mb character into a node field and saves it, permission may be granted for anonymous users to view all private nodes. Note that this may need to be done twice to demonstrate the problem, depending on the setup.

When this happens:
1) The node_revisions table fails to insert because of the unsupported character.
2) The node table then fails to insert because of duplicate vid=0 (assuming it has happened at least once before), or because of the unsupported character itself.
3) This is then propagated through every installed module that hooks into node insert, doing their operations with nid=0 and vid=0.
4) Finally, these are followed by node_access_acquire_grants($node). Best case, the node access module in use produces broken entries for limited users (ie. og_access in context, private node). The node access module may also produce global access entries (ie. og_access in context, public node). But if the node access module does nothing with the grants (og_access, global node, so out of context), Drupal defaults to insert into the node_access table a row with nid=0, thereby granting anonymous permission to view all nodes (whether they were meant to be protected by the node access module or not).

From a broader point of view:
- A failure to insert into the node table should be trapped at the point of the insertion as there are multiple different situations that could result in a failed insert.
- The error should not be propagated into the node insert hooks as the majority of modules that use those hooks expect a valid nid.
- The fix belongs to core as core defaults to write the grants improperly violating node access module expectations.

Details from our implementation: Ubuntu, PHP, MySQL, Apache. Using D6 with OG (og_access). In our case, this became an issue from research-oriented material that contained specialized mathematical symbols, like a lower-case omega, for example.

Problem/Motivation

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Active

Component

Code

Created by

🇳🇱Netherlands dokumori Utrecht

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the “Report a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

  • Issue created by @dokumori
  • Comment over 1 year ago →
    🇳🇱Netherlands dokumori Utrecht

    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.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024