This issue is related to tokens support.

Problem/Motivation

token_replace() function accepts "sanitize" option, which is a boolean flag indicating that tokens should be sanitized for display to a web browser and defaults to TRUE. But, when token generated value is replaced with translated one from "Title" module in hook_tokens_alter(), this option is ignored, and unsanitized value returns any time.

In this way, html entities in title aren't encoded and are lost on UI, and also it provides ability of XSS attack.

Proposed resolution

I propose to obtain "sanitize" option in title_tokens_alter() function and process it like "[node:title]" token processes it:

        case 'title':
          $replacements[$original] = $sanitize ? check_plain($node->title) : $node->title;
          break;

Other possible solutions

It is possible to pass "sanitize" option to syncronization get callback, to obtain not "value" of field, but the "safe_value" with field format and sanitization settings applied. But it requires more work, testing and changes in module API.

CommentFileSizeAuthor
#25 title-text_formats-2757739-23.patch3.65 KBplach
#25 title-text_formats-2757739-23.interdiff.txt2.44 KBplach
#14 2757739.patch3.62 KBpol
#12 2757739.patch3.16 KBpol
#8 2757739.patch2.34 KBpol
#2 title-token_sanitization_support-2757739-2.patch989 bytesdewalt

Comments

dewalt created an issue. See original summary.

dewalt’s picture

dewalt commented
Status: Active » Needs review
StatusFileSize
new title-token_sanitization_support-2757739-2.patch989 bytes

Here is proposed patch

pol’s picture

pol
Location Brussels
commented
Status: Needs review » Active

Hi,

Patch is perfectly working, and should be included in Title asap.

Thanks!

pol’s picture

pol
Location Brussels
commented
Status: Active » Reviewed & tested by the community
alexverb’s picture

alexverb commented
Status: Reviewed & tested by the community » Needs work

When using a filter format on the title field and entering a value in plain text field without wysiwyg I receive the following errors:

  • Warning: html_entity_decode() expects parameter 1 to be string, array given in decode_entities() (line 463 of /.../includes/unicode.inc).
  • Warning: Illegal offset type in pathauto_cleanstring() (line 237 of /.../contrib/pathauto/pathauto.inc).
  • Notice: Array to string conversion in token_replace() (line 102 of /.../includes/token.inc).

To reproduce, do the following:

  • edit replaced title field
  • change text processing to filtered text
  • add new content
  • click "disable rich text" link (cause wysiwyg filters the script anyway I think)
  • enter "
    alert("Hello, World");

    " as value

  • save content
pol’s picture

pol
Location Brussels
commented

Hi Alex,

Are you still having the popup "Hello World" after that ?

alexverb’s picture

alexverb commented

Hey guys, sorry. It seems like the warnings were project specific.
When testing a vanilla drupal 7.51 no warnings are shown and the alias is generated fine.

pol’s picture

pol
Location Brussels
commented
StatusFileSize
new 2757739.patch2.34 KB

After long discussion with my colleagues,

Here's another patch. I'll leave this here for comment.
It will fix the security issue from this thread and allows you to use a filter format properly.

Let me know what you think.

ademarco’s picture

ademarco commented
Status: Needs work » Reviewed & tested by the community

Patch #8 works well, marking it as RTBC.

jyraya’s picture

jyraya commented

I've tested the patch 8.

I get the following notice message: "Notice: Undefined index: format in title_field_text_sync_get() (line 136 of .../modules/contrib/title/title.core.inc)."

When I run the following scenario:

  1. Create/Edit content where the title is replaced by a field with "Plain text" as Text processing
  2. Click save button.
  3. The notice message appears then.

The problem seems coming from the fact the function "title_field_text_sync_get" is called for synchronizing the node title value where no format is specified during the execution of title_field_attach_submit.

jyraya’s picture

jyraya commented
Status: Reviewed & tested by the community » Needs work
pol’s picture

pol
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new 2757739.patch3.16 KB

Hi,

Here's an updated patch with your recommandations.

Status: Needs review » Needs work

The last submitted patch, 12: 2757739.patch, failed testing.

pol’s picture

pol
Location Brussels
commented
Status: Needs work » Needs review
StatusFileSize
new 2757739.patch3.62 KB

Add more defensive code in case to avoid notice if array key doesn't exist.

pol’s picture

pol
Location Brussels
commented

Hi,

Sadly we cannot rely on the tests for now, they are broken and completely unrelated to this issue.

The patch to apply to reduce the number of tests failure is:

  1. #2813673: Tests broken since new permission in drupal core

With this patch applied, we are down to 13 fails. I'm hardly working to get this to zero, a little help would be very welcome but please, use the appropriate thread (#2813673: Tests broken since new permission in drupal core).

Status: Needs review » Needs work

The last submitted patch, 14: 2757739.patch, failed testing.

pol’s picture

pol
Location Brussels
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Security

Hi,

Tests are now green again, but you have to apply the patch from #2813673: Tests broken since new permission in drupal core first, then the patch from #14.

Updating the status of the issue.

dxvargas’s picture

dxvargas commented
Priority: Major » Critical
Related issues: +#2813673: Tests broken since new permission in drupal core

Like @Pol said, tests are failing because of issue #2813673. which already has a patch RTBC.
After applying both patches (2813673-title-tests-fail.patch and this one 2757739_1.patch), tests are all green.

Here are the versions we used:
Drupal: 7.53
Title: 7.x-1.0-alpha8+0-dev
Entity: 7.x-1.8
Entity translation: 7.x-1.0-beta5

Before the patches:
*281* passes, *30* fails, 0 exceptions, and 98 debug messages

After the patches:
*311* passes, *0* fails, 0 exceptions, and 104 debug messages

Test steps:

  1. Enable token_filter
  2. Enable "Replace tokens" for Full HTML
  3. Create a page with title <script>alert("Hello, World");</script> and body [node:title] with Full HTML text format
  4. Save
  5. Before patch I had an alert, after patch I don't have an alert anymore

So I confirm the RTBC.

I will raise the Priority to Critical because of obvious reasons.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

The patch in #14 seems a bit of a big change for a security fix. I'd be inclined to commit the patch in #2, publish a security update release and then commit #14 to 7.x-1.x-dev.

Thoughts?

pol’s picture

pol
Location Brussels
commented

Hi Francesco,

The patch in #14 is indeed introducing some changes.

As the title field is a real Drupal field now, we need to decide if it can be used with an input format or not (just plain text).
If we decide to only use it as a plain text input, then, this patch needs to be rewritten accordingly.
On the other hand, if we decide to allow different input formats, I think this patch is the way to go with maybe a couple of small changes.

We are already using the patch at work for less or more 150 sites and no problem has been reported so far.

Let me know what to do so I can push forward this asap.

Thanks!

  • plach committed 5c81d2c on 7.x-1.x authored by dewalt 
    Issue #2757739 by Pol, dewalt: Token value is not sanitized, when...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Spoke with Pol about this: we will move on with the plan outlined in #19.

Committed and push #2, thanks!

pol’s picture

pol
Location Brussels
commented

Cool thanks!

I will open another issue for supporting input formats then.

  • plach committed 8119fa2 on 7.x-1.x authored by Pol 
    Issue #2757739 by Pol, jyraya, alexverb, plach, ademarco, dxvargas:...
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new title-text_formats-2757739-23.interdiff.txt2.44 KB
new title-text_formats-2757739-23.patch3.65 KB

Published security release at: https://www.drupal.org/project/title/releases/7.x-1.0-alpha9

Committed and pushed the attached patch, thank you all!

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
commented

Fixing credits

Status: Fixed » Closed (fixed)

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