Drupal Security Lessons: XSS in field_display_label

If you’re not making mistakes, then you’re not doing anything. 
I’m positive that a doer makes mistakes.

 - John Wooden

This is the first blog post in what will hopefully become a new series where we look at old Drupal 7 & 8 security advisories (at least 3 months ago so they should be patched everywhere) and try to learn from the mistakes of others.

As a first post I’d like to pick an older vulnerability, one I’ve used in presenations to demonstrate how hard it can be to properly apply HTML encoding for Drupal.

Advisory

What was the vulnerability?

From the vulnerability description:

This module enables you to use a different label for displaying fields from the label used when viewing the field in a form.

The module doesn’t sufficiently sanitize the alternate field label in content types settings.

This vulnerability is mitigated by the fact that an attacker must have a role with the permission to add or edit fields on an entity.

Field Display Label (FDL) is a simple little module that adds a ‘display_label’ to the field instance editting and then with preprocessor similar to hook_preprocess_HOOK sets the label to be the ‘display_label’. See also the vulnerable module file.

However what the author missed is that Drupal (the ‘field’ module in this case) automatically encodes the label with check_plain while the author did not. Meaning that if the admin or some evil user (or the admin is tricked with something like ClickJacking) in the backend inputs something like “alert(‘xss’);” all users will get an alert.

How was it fixed?

By check_plaining the ‘display_label’ before setting it to the label value.

See also the commit that fixed the vulnerability.

What can we learn?

We should always aim to encode as late as possible, which in this case would mean doing the check_plain in the template and NOT in a preprocess function. This because encoding content that has already been encoded will cause the user to see HTML entities or even worse may actually allow an attacker to use that to negate the encoding. 

However in this case Drupal already encodes the label and expects it to be used as it it was encoded already.

I think the lesson here is twofold:

1. If you’re adding your own variables, save your check_plain for the template.

2. If you’re overwriting Drupal added variables, check whether Drupal expects it to be encoded. And leave a comment in the code letting the rest of us know your results.

See the OWASP page on Encoding for more information.

Discuss

Please let me know what you think about this post and continuing this as a series on this reddit post!