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
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. |
Comment | File | Size | Author |
---|---|---|---|
#23 | Details___Site-Install.png | 60.96 KB | joelpittet |
#15 | interdiff.txt | 420 bytes | effulgentsia |
#15 | string-format-safe-15.patch | 7.54 KB | effulgentsia |
#14 | interdiff.txt | 6.18 KB | effulgentsia |
#14 | string-format-safe-14.patch | 7.55 KB | effulgentsia |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedComment #4
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedLet's go one step further and se what happens if we make '@' use autoescaping as well.
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedOh, why not do the same for
%
as well.Comment #12
effulgentsia CreditAttribution: effulgentsia commentedTrying out an idea for a less disruptive approach.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedYep, I like this new approach better. Adjusting issue title, summary, category, and tags accordingly.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedMinor improvement to code clarity.
Comment #16
dawehnerThank 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.
Do we need age to be HTML? Just being curious here.
Great to see the test coverage being expanded.
Comment #17
alexpottCommitted 44313c6 and pushed to 8.0.x. Thanks!
Thanks for adding the eta evaluation to the issue summary.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #20
joelpittetPassthrough 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.
Comment #21
joelpittetOr do we have to loop through all incoming variables and mark them safe or escape them? Sounds expensive
Comment #22
webchickI 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.
Comment #23
joelpittetOne 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:
Comment #24
joelpittetThere 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.
Comment #25
joelpittetI'm sure I could be convinced otherwise and this borders that line pretty tight, but that's just how I feel.
Comment #26
joelpittetA bit more DX confusion happening here #2309215: HTML double-escaping in revision messages because of this.
Comment #27
Fabianx CreditAttribution: Fabianx commentedIn 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.
Comment #28
Fabianx CreditAttribution: Fabianx commentedComment #29
effulgentsia CreditAttribution: effulgentsia commentedString::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.Comment #30
effulgentsia CreditAttribution: effulgentsia commentedAdditionally, 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.Comment #31
Fabianx CreditAttribution: Fabianx commented#29: That makes a _lot_ of sense!
Thank you so much for the insightful approach!
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedI posted a draft CR: https://www.drupal.org/node/2445441. Setting this issue to "needs review" for feedback on it before publishing it.
Comment #33
Fabianx CreditAttribution: Fabianx commentedRTBC for the CR, looks great!
Comment #35
Fabianx CreditAttribution: Fabianx commentedThe RTBC is for the CR.
Comment #36
catchPublished the change notice.
Comment #38
xjmWe 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 addSafeMarkup::set()
calls, which should be for internal use.We discussed the following possible approach to ease up this change a bit:
!
in core, and convert them to@
instead where appropriate.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.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.
Comment #39
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #40
Fabianx CreditAttribution: Fabianx as a volunteer commentedThanks for sticking with it #39.
I really like the new ! approach TBH.
Comment #41
joelpittet@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:
Output:
I don't see where B would have any use-case any further. And C and D produce the same results.
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe docs of SafeMarkup::format() currently say:
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.Comment #43
joelpittet@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!Comment #44
xjmThis 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.Comment #45
xjmFiled #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand.
Comment #46
xjmComment #47
xjm#2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand is now critical, so this will be revisited before RC via that.