Problem/Motivation

Issue title says it all.

Proposed resolution

Change String::format() to not call SafeMarkup::set() on the result if there are one or more passthrough arguments that are not safe.

Remaining tasks

Review. Commit.

User interface changes

None.

API changes

No change to any documented API. But code that was relying on the broken behavior will now need to ensure that passthrough arguments are safe if it wants the result to be marked safe.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because SafeMarkup::isSafe() is returning incorrect information.
Issue priority Major because incorrectly marking a string as safe can lead to security vulnerabilities. Not critical because String::format() documents that passthrough arguments must already be safe, so this bug only affects code that violates that documentation. Prior versions of Drupal were released with the same expectation that sanitizing passthrough arguments is the responsibility of the caller, so this bug is not a regression.
Prioritized changes The main goal of this issue is security.
Disruption Only disruptive for modules that are passing strings not marked as safe and expecting the result to be treated as safe. There are some cases in which this is a not-insecure expectation, such as when the input string is known to be safe due to custom validation but isn't marked as such, but it isn't hard to fix such code to comply with D8 SafeMarkup rules, as is shown in the cases within the patch.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, string-format-safe.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
4.71 KB
2.76 KB

Status: Needs review » Needs work

The last submitted patch, 2: string-format-safe-2.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
26.98 KB
20.54 KB

This fixes most of the test failures, and in the process, removes the "passthrough" Twig filter, since we don't need a separate no-passthrough vs. passthrough distinction when we have autoescaping.

Status: Needs review » Needs work

The last submitted patch, 4: string-format-safe-4.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
27.71 KB
2.29 KB

Let's go one step further and se what happens if we make '@' use autoescaping as well.

Status: Needs review » Needs work

The last submitted patch, 6: string-format-safe-6.patch, failed testing.

The last submitted patch, 6: string-format-safe-6.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
28.15 KB
11.85 KB

Oh, why not do the same for % as well.

Status: Needs review » Needs work

The last submitted patch, 10: string-format-safe-10.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
959 bytes

Trying out an idea for a less disruptive approach.

Status: Needs review » Needs work

The last submitted patch, 12: string-format-safe-12.patch, failed testing.

effulgentsia’s picture

Title: Security harden String::format()'s !variable option to autoescape rather than not escape » String::format() marks a resulting string as safe even when passed an unsafe passthrough argument
Category: Task » Bug report
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Security improvements +Security
FileSize
7.55 KB
6.18 KB

Yep, I like this new approach better. Adjusting issue title, summary, category, and tags accordingly.

effulgentsia’s picture

FileSize
7.54 KB
420 bytes

Minor improvement to code clarity.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for pointing out towards this issue in the other issue.

So code, which calls String::format() with arbitrary HTML, has to take care that the raw HTML string is already safe, makes sense.

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -136,12 +136,12 @@ public function form(array $form, FormStateInterface $form_state) {
         '!age' => $this->dateFormatter->formatInterval(REQUEST_TIME - $view->lock->updated),
...
-        '#children' => $this->t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="!break">break this lock</a>.', $lock_message_substitutions),
+        '#children' => $this->t('This view is being edited by user !user, and is therefore locked from editing by others. This lock is !age old. Click here to <a href="@url">break this lock</a>.', $lock_message_substitutions),

Do we need age to be HTML? Just being curious here.

Great to see the test coverage being expanded.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 44313c6 and pushed to 8.0.x. Thanks!

Thanks for adding the eta evaluation to the issue summary.

  • alexpott committed 44313c6 on 8.0.x
    Issue #2399037 by effulgentsia: String::format() marks a resulting...
David_Rothstein’s picture

Title: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument » (change notice, etc.) String::format() marks a resulting string as safe even when passed an unsafe passthrough argument
Status: Fixed » Needs work
Issue tags: +Needs change record

This is a good security improvement but breaks a number of things (see #2409477: Various ! placeholders throughout core are now showing escaped HTML (especially <a href="!..."> examples)) so I think it really needs a change record, or maybe an update to an existing change record.

Also, the current function documentation really only makes clear that you are responsible for sanitizing user-supplied text before using it with a !-placeholder, not that you're responsible for "sanitizing" (or at least telling Drupal about) something that is already 100% guaranteed to be safe - the latter is a new requirement introduced by this issue. See the issue I linked to above for more details.

joelpittet’s picture

Passthrough should be passthrough and assumed safe as it's explicitly set. I think that change needs to be reverted. The other ! to @ changes should be fine.

joelpittet’s picture

Or do we have to loop through all incoming variables and mark them safe or escape them? Sounds expensive

webchick’s picture

I actually wonder if we should roll this back until it's a bit more baked, and at least covers 80%+ of the brokenness introduced. We've been fighting with #2297711: Fix HTML escaping due to Twig autoescape for over 6 months; we don't really need to open another front on that war while we're in beta.

joelpittet’s picture

FileSize
60.96 KB

One of the fixes that is super overkill and still doesn't totally fix #2409881: Exception log messages are double-escaped when viewing a single entry:

diff --git a/core/modules/dblog/src/Controller/DbLogController.php b/core/modules/dblog/src/Controller/DbLogController.php
index e8b1ba5..175bc31 100644
--- a/core/modules/dblog/src/Controller/DbLogController.php
+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -9,6 +9,7 @@

 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Unicode;
+use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\String;
 use Drupal\Component\Utility\Xss;
 use Drupal\Core\Controller\ControllerBase;
@@ -342,11 +343,12 @@ public function formatMessage($row) {
     if (isset($row->message) && isset($row->variables)) {
       // Messages without variables or user specified text.
       if ($row->variables === 'N;') {
-        $message = $row->message;
+        $message = SafeMarkup::checkAdminXss($row->message);
       }
       // Message to translate with injected variables.
       else {
-        $message = $this->t($row->message, unserialize($row->variables));
+        $variables = array_map('Drupal\Component\Utility\SafeMarkup::checkAdminXss', unserialize($row->variables));
+        $message = $this->t($row->message, $variables);
       }
     }
     else {

joelpittet’s picture

There is a bit of an issue with explicit intent on this one too. There is an intent to show RAW values, regardless of their safety. And when the internal logic subverts this request it undermines the intent written with the !.

We are babysitting quite a bit, but there has got to be some limit where we need to give up a bit of control/security for DX. I personally think this cross that line. It will create more DX WTFs than it will help with security, IMO.

joelpittet’s picture

I'm sure I could be convinced otherwise and this borders that line pretty tight, but that's just how I feel.

joelpittet’s picture

A bit more DX confusion happening here #2309215: HTML double-escaping in revision messages because of this.

Fabianx’s picture

In general I am +1 to making the APIs not easy to fail.

I however would have implemented this differently, by just running ! values through SafeMarkup::check() in the net effect this is the same, because:

- the offending string is check-plained

However it has the advantage / disadvantage that only the offending part is check plained, while with the current patch the whole string is check plained.

So if the string is gonna be check plained anyway in most cases, we can just use the check function in the first place would be my argument.

The disadvantage is that !url will probably work in 99% of the cases, but still internally check plained in most cases.

Though given this is the most breaking thing, I cannot see why we don't just can make the Link Generator or whatever is used for !url return SafeMarkup if that is so common.

So this patch very easily highlights insecure strings.

On the other hand, people start putting the output of t() to #markup and hence trying to circumvent the system, which is not a good sign, because this means contrib authors will try the same.

On the other hand of the other hand, Using String::format('!foo') is something that we also do not want to encourage obviously.

Another possibility is to just run the SafeMarkup::checkAdminXSS on the non-safe ! values like we do for #prefix and #suffix.

@effulgentsia: It would be great if you could chime in on this again.

To summarize again there are three ways forward that I can currently see:

a) Leave things as is, possibly return SafeMarkup for the most used cases for !url and !link and fix the remainder
b) Use SafeMarkup::check() and just check_plain unescaped ! strings
c) Use SafeMarkup::checkAdminXss() and just check unescaped ! strings

Even in b), c) link functions should probably return SafeMarkup if those need to be safe in many cases in core.

Thanks @all for making Drupal more secure.

Fabianx’s picture

Assigned: Unassigned » effulgentsia
effulgentsia’s picture

I however would have implemented this differently, by just running ! values through SafeMarkup::check() in the net effect this is the same, because:

- the offending string is check-plained

However it has the advantage / disadvantage that only the offending part is check plained, while with the current patch the whole string is check plained.

String::format() is sometimes called on strings that are not intended for HTML display. For these strings, it is important to preserve ! as being a pure passthrough. Unfortunately, the problems we're seeing is that ! is also used in places where we do want HTML output, and should therefore be using @, but aren't, because of D7's behavior of @ double-escaping an already escaped/rendered string. So let's fix that in #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped.

effulgentsia’s picture

String::format() is sometimes called on strings that are not intended for HTML display.

Additionally, String::format() is sometimes called on a string where the result will then itself be passed as a @ placeholder to another String::format(). And therefore, ! needs to be a pure passthrough, unless we fix #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped. So regardless of if we want to change !, we need to do #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped in any case.

Fabianx’s picture

#29: That makes a _lot_ of sense!

Thank you so much for the insightful approach!

effulgentsia’s picture

Status: Needs work » Needs review

I posted a draft CR: https://www.drupal.org/node/2445441. Setting this issue to "needs review" for feedback on it before publishing it.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC for the CR, looks great!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: string-format-safe-15.patch, failed testing.

Fabianx’s picture

Status: Needs work » Reviewed & tested by the community

The RTBC is for the CR.

catch’s picture

Title: (change notice, etc.) String::format() marks a resulting string as safe even when passed an unsafe passthrough argument » String::format() marks a resulting string as safe even when passed an unsafe passthrough argument
Status: Reviewed & tested by the community » Fixed

Published the change notice.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -Needs change record +Needs followup, +revisit before release candidate

We revisited this today in a call with @joelpittet, @effulgentsia, and @Cottser.

In general, I'm in agreement with #24: I think we're breaking the user intent of the ! token here. I'd prefer to allow ! to be truly raw over requiring developers to add SafeMarkup::set() calls, which should be for internal use.

We discussed the following possible approach to ease up this change a bit:

  1. Review all uses of ! in core, and convert them to @ instead where appropriate.
  2. Evaluate the remaining uses; check if they're being double-escaped.
  3. Possibly add another placeholder type to SafeMarkup::format() to distinguish between "don't escape this now, but let Twig do it" and "No really, I mean it, I want this variable to be printed unsanitized exactly as-is in the resultant string.
  4. Do not ever add the variable itself to the safe list -- only the resultant string.

I realize I'm posting on a closed issue. :) I'm not quite to the point of asking for a revert here without being more familiar with specifics, but tagging to make sure at least we come back to this.

effulgentsia’s picture

I think we're breaking the user intent of the ! token here.

I thought about it more since our call referenced in #38. I disagree about there being legitimate user intent for passing not-marked-safe variables through to the browser. In Drupal 7, ! was there because in the absence of a SafeMarkup system tracking already prepared/escaped strings, there needed to be some way to communicate to t() that the variable has already been prepared for html display. The docs for D7's format_string() says !variable: ...Only use this for text that has already been prepared for HTML display (for example, user-supplied text that has already been run through check_plain() previously, or is expected to contain some limited HTML tags and has already been run through filter_xss() previously). In D8, these kinds of preparations result in the variable being marked safe, in which case HEAD's ! behaves the same as D7's.

I agree with steps 1 and 2 from #38, but before doing 3 or 4, I want to see specific examples of where it's legitimate for the caller to send never-escaped variables to the browser. All the examples I've seen so far mentioned in this issue point to other bugs of how variables are prepared.

Related: #2282101: Remove |passthrough filter in Twig.

Fabianx’s picture

Thanks for sticking with it #39.

I really like the new ! approach TBH.

joelpittet’s picture

@effulgentsia Sorry for being such a hard sell but I'm slowly getting used to the idea.

Right now the @ and ! are so similar now it's hard to distinguish when I'd ever need to use !.

Can you think of a use-case to even have ! now that it's powers are neutered?

Here's a little example script:


use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Render\Renderer;

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">@var', ['@var' => '<a>icon</a>'])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">!var', ['!var' => '<b>icon</b>'])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">@var', ['@var' => SafeMarkup::set('<c>icon</c>')])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

$markup = [
  '#type' => 'inline_template',
  '#template' => '{{ data }}',
  '#context' => ['data' => SafeMarkup::format('<a href="#">!var', ['!var' => SafeMarkup::set('<d>icon</d>')])],
];
print \Drupal::service('renderer')->render($markup) . "\n";

Output:

<a href="#">&lt;a&gt;icon&lt;/a&gt;
&lt;a href=&quot;#&quot;&gt;&lt;b&gt;icon&lt;/b&gt;
<a href="#"><c>icon</c>
<a href="#"><d>icon</d>

I don't see where B would have any use-case any further. And C and D produce the same results.

effulgentsia’s picture

Can you think of a use-case to even have ! now that it's powers are neutered?

The docs of SafeMarkup::format() currently say:

   *   - !variable: Inserted as is, with no sanitization or formatting. Only
   *     use this when the resulting string is being generated for one of:
   *     - Non-HTML usage, such as a plain-text email.
   *     - Non-direct HTML output, such as a plain-text variable that will be
   *       printed as an HTML attribute value and therefore formatted with
   *       self::checkPlain() as part of that.
   *     - Some other special reason for suppressing sanitization.

It might be clearer if we remove/deprecate ! entirely and instead add a $strategy parameter to SafeMarkup::format() that can be set to 'text' for such use cases, but I'm not sure it's worth doing that this late in D8 development.

joelpittet’s picture

@effulgentsia Deprecating ! may be a good call because right now it's kind of unintuitive how variables can affect the surrounding string they are contained within. Or maybe at the very least for DX this is a job for that fancy Assert thing #2444003: Optimize Drupal\Core\Template\Attribute?

A render strategy for format would be awesome for placeholder because CLI could send context (in my dreams) that would prevent placeholder<em> from mucking up the terminal output for watchdog messages and other messages!

xjm’s picture

This came up again in #2506035: Do not add placeholders from SafeMarkup::format() to the safe list.

The behavior of ! placeholders is completely counterintuitive now. Instead of printing a string with exactly what you passed, it ultimately escapes every other piece of the markup in the string as well. That is a bug. We should either revert this or remove ! entirely.

xjm’s picture

xjm’s picture

Issue tags: -Needs followup
xjm’s picture

Issue tags: -revisit before release candidate