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. See original summary.

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.

prudloff’s picture

prudloff commented

Xss::filterAdmin() already strips all the problematic tags hence there is no issue related to xss.

Indeed, XSS is not possible here.
The problem here just an HTML injection (which can break the layout when an error is displayed or in some scenarios be chained with another XSS vulnerability).

If we remove Xss::filterAdmin() and render the plain text, will it have any impact from security perspective?

In theory if we stop using Markup::create(), the HTML will be escaped automatically so we don't need to filter it anymore. But this needs to be tested.

prudloff’s picture

prudloff commented
Status: Active » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

This looks great to me.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

#642160: Make debug() message work better (usable) is the original issue that mentions making the error messages readable HTML. Question: is any of that is relevant today? I'm thinking no, other than maybe in watchdog logs?

smustgrave’s picture

smustgrave commented

What's a good way to find an answer for #8?