Problem/Motivation

If an attacker gets access to error logs and CsrfTokenGenerator::validate() throws an exception, it could leak valid CSRF tokens in the stack trace:

RuntimeException:
Missing $settings['hash_salt'] in settings.php.

  at core/lib/Drupal/Core/Site/Settings.php:173
  at Drupal\Core\Site\Settings::getHashSalt()
     (core/lib/Drupal/Core/Access/CsrfTokenGenerator.php:115)
  at Drupal\Core\Access\CsrfTokenGenerator->computeToken('yZyPqDjxGaPr_PKQ_u5GrSzhZ6cxTX8yw_w6RSFdsLs', '')
     (core/lib/Drupal/Core/Access/CsrfTokenGenerator.php:89)
  at Drupal\Core\Access\CsrfTokenGenerator->validate('valid_token')
     (modules/custom/test/test.module:50)

Steps to reproduce

Very crude way to trigger an exception;

  ini_set('zend.exception_ignore_args', FALSE);
  new Settings([]);
  \Drupal::csrfToken()->validate('valid_token');

Proposed resolution

Add #[SensitiveParameter] to the $token argument.

Maybe the $seed argument in computeToken() should also be considered sensitive.
(I don't think you can do anything dangerous with only the seed, you would also need the private and the hash salt.)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3582512

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:

Comments

prudloff created an issue. See original summary.

sourav_paul’s picture

Working on this...

sourav_paul’s picture

Status: Active » Needs review
phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

I don’t see a problem here.

  • amateescu committed a79fcb2f on 11.x
    task: #3582512 Add SensitiveParameter attribute to CsrfTokenGenerator::...

  • amateescu committed b528aa65 on main
    task: #3582512 Add SensitiveParameter attribute to CsrfTokenGenerator::...
amateescu’s picture

Status: Reviewed & tested by the community » Fixed

Makes sense to me as well. And I agree that $seed from computeToken() should not be considered sensitive because it's useless without the private key and hash salt.

Committed and pushed b528aa6577d to main and a79fcb2f690 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

amateescu’s picture

Version: main » 11.x-dev

  • amateescu committed 662dd45f on 11.3.x
    task: #3582512 Add SensitiveParameter attribute to CsrfTokenGenerator::...

  • amateescu committed a52059c2 on 10.6.x
    task: #3582512 Add SensitiveParameter attribute to CsrfTokenGenerator::...
amateescu’s picture

Version: 11.x-dev » 10.6.x-dev

Discussed this issue with the release managers, and decided to backport it all the way back to 10.6.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.