Problem/Motivation

Similar to Validating String Encodings Before Insert, a malicious user tried to change an sql query by manipulating the url of an ajax call which resulted in the following error:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1267 Illegal mix of collations (ascii_general_ci,IMPLICIT) and (utf8mb4_general_ci,COERCIBLE) for operation '=': SELECT "name", "data" FROM "config" WHERE "collection" = :collection AND "name" IN ( :names__0 ); Array
(
    [:collection] =>
    [:names__0] => views.view.awards鎈'"\(
)
 in Drupal\Core\Config\DatabaseStorage->readMultiple() (line 111 of /var/www/<site>/public/core/lib/Drupal/Core/Config/DatabaseStorage.php).

Steps to reproduce

To reproduce this, you can just hit this url: https:///views/ajax?view_name=awards鎈'"\(&view_display_id=page_1&_drupal_ajax=1
..and pass in some weird character with the view name (in this case the user passed in the view name 'awards' with some (chinese??) character and some other characters after it.

Proposed resolution

Validate the view_name inside ViewAjaxController::ajaxView() before Drupal tries to use it to load a config entity.

CommentFileSizeAuthor
#6 3457963-validate-ajaxview-arguments.patch2.26 KBdevdits
#5 validate_ajaxview_arguments.patch853 bytesdevdits

Issue fork drupal-3457963

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ramil g created an issue. See original summary.

ramil g’s picture

ramil g commented
Component: ajax system » views.module
ramil g’s picture

ramil g commented
Issue summary: View changes

joelpittet made their first commit to this issue’s fork.

devdits’s picture

devdits commented
StatusFileSize
new validate_ajaxview_arguments.patch853 bytes

This patch can solve the problem until someone will create better solution.

devdits’s picture

devdits commented
StatusFileSize
new 3457963-validate-ajaxview-arguments.patch2.26 KB
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Added devdits and ramil g to issue credit

ramil g’s picture

ramil g commented

joelpittet changed the visibility of the branch 11.x to hidden.

ramil g opened merge request !9053

joelpittet changed the visibility of the branch 3457963-validate-viewname-in to hidden.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 10.3.x-dev » 11.x-dev
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Seems to have a test failure so probably need test coverage for this one please

thanks!

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

There are more use-cases /node/add/öüä, /media/add/öüä, admin/structure/views/view/öüä. So any config name.

arunkumark made their first commit to this issue’s fork.

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

I updated the condition to check for a valid name and path in ViewAjaxController.php. The view may be without a path(In case of blocks).

Added new test cases for the Malicious strings in Name, Display Id, and Path.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Pipeline failures

arunkumark’s picture

arunkumark
Location Coimbatore
commented
Status: Needs work » Needs review

The pipeline issue has been resolved.

Moving status to Review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Test-only job runs without issue so tests still need work.

jannakha’s picture

jannakha commented
Related issues: +#3475540: Use a route requirement to prevent non-ASCII characters causing an exception when looking up a config entity

here's a solution to the underlying problem (which also solves #14)

https://www.drupal.org/project/drupal/issues/3475540

arunkumark’s picture

arunkumark
Location Coimbatore
commented

@jannakha The suggested fix for ASCII is already added,

/**
   * Validates given value to protect from unexpected requests.
   *
   * @param mixed $value
   *   Given value for validation.
   *
   * @return bool
   *   Returns TRUE if value valid, otherwise FALSE
   */
  private function isValid(mixed $value): bool {
    return is_scalar($value)
      && mb_check_encoding($value, 'ASCII');
  }
smustgrave’s picture

smustgrave commented

If #3475540: Use a route requirement to prevent non-ASCII characters causing an exception when looking up a config entity solves more problems including this one this should probably be closed.

arunkumark’s picture

arunkumark
Location Coimbatore
commented

@smustgrave
The handling in special character on #3475540: Use a route requirement to prevent non-ASCII characters causing an exception when looking up a config entity for Configuration settings. But here we patched for Ajax views links. Is the fix on Config will help for the Views URLs?

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.