Background information

This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team.

Problem/Motivation

Drupal allows HTML in PHP error messages.
According to a comment in _drupal_error_handler_real() this is intentional. But it can sometimes make some XSS easier to exploit (by allowing an attacker to inject an HTML payload) so maybe it could be worth hardening.

Steps to reproduce

Browse to /admin/config/development/logging and set "Error messages to display" to "Errors and warnings".

Run this code:

  $array = [];
  $array['<b>foo</b>'];

The error Warning: Undefined array key "foo" is displayed with "foo" displayed in bold.
This feels a bit weird because <b>foo</b> is a key name and is not supposed to be HTML.

Proposed resolution

The easiest fix would be to simply do this in _drupal_error_handler_real():

'@message' => $message,

But it might break some legitimate use cases with HTML in error messages?

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3603805

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

prudloff created an issue.

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

mohit_aghera opened merge request !16134

mohit_aghera’s picture

mohit_aghera commented

I've worked on the issue and added a test.

I have a slight doubt about the proposed solution

'@message' => $message,

Currently in errors.inc, we have something like this.
'@message' => Markup::create(Xss::filterAdmin($message)),
Xss::filterAdmin() already strips all the problematic tags hence there is no issue related to xss.
I've added a test with < script > tag.

If we remove Xss::filterAdmin() and render the plain text, will it have any impact from security perspective?
I tried on by doing like '@message' => $message and it displays the plain text.
However, the current Xss::filterAdmin() call seems more reasonable to me.