Config translation needs to be validated on input for XSS (like other t string input)

I may be misunderstanding what the issue is based on how easy it seemed to be to solve.

Here is a patch that adds a validateForm method to core/modules/config_translation/src/Form/ConfigTranslationFormBase.php

The validation checks to see if the config values submitted are safe or not using locale_string_is_safe(), and if they are not then it displays a form error.

One thing, I was not sure of the best way to display the correct form field title (such as 'label'), since the labels from the $form object contain HTML markup. (ex: "Label <span class="hidden">(English)</span>", which is how the form error message would display it)

Tests are failing, and produce some warning. One of the cause is likely that locale_string_is_safe($value) expect a string, while $value may be an array. Something using <a href="http://php.net/manual/en/function.array-walk-recursive.php">array_walk_recursive()</a> should be able to validate strings nested in any level of nested arrays.

@mikemiles86, I'm at the #DrupalNorth sprint if you wish to do pair programming.

I've updated my patch, since the previous one was failing tests.

It will now only validate against submitted string values. @pbuyle in comment #7 mentioned that the submitted field value could be an array and we might want to iterate through it. I agree, but I was not sure of the best approach to do so. Potentially how deep could a form field values array be?

I also updated to grab the field label and #name from the $form array to pass to the form error message.

In general we should add a test which checks that you can actually not enter invalid data as part of the translation.

Bugcrowd asked me to comment on this issue.

Also:

+++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
@@ -195,6 +195,25 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
+          $form_state->setErrorByName($field_name, $this->t('The submitted %label value contains disallowed HTML', array('%label' => $label)));

This gives me The submitted Label <span class="visually-hidden">(Dutch)</span> value contains disallowed HTML. Seems the #title is already rendered and can contain html.

And:

Potentially how deep could a form field values array be?

/admin/structure/views/view/frontpage/translate goes about 8 levels deep and the patch only protects the top level Label and Description. Setting to needs work.

Seems the #title is already rendered and can contain html.

You are correct. It seems that when the config form gets built, the field label contains that language span tag. Previously I was just using the $key value from the form_state values array, however that value does not always match the field label that is displayed.

I have yet to be able to find a reliable way to either get the field title without the language specific HTML or strip out the HTML. Any ideas are welcomed.

/admin/structure/views/view/frontpage/translate goes about 8 levels deep and the patch only protects the top level Label and Description

Thanks for this info. Could write a function that recursively checks the $value array to make sure it contains no unsafe strings. I'm not sure the best place for that function, a private function within the ConfigTranslationFormBase.php class?

Something such as:

<?php
  /**
   * Recursive function to check that item contains only safe strings.
   * 
   * @param  string|array $item
   *   The item to check.
   *
   * @return bool
   *   Return boolean TRUE or FALSE.
   */
  private function validateSafeString($item) {
    $is_safe = TRUE;
    // Is the passed item an array?
    if (is_array($item)) {
      // Loop through each value in array.
      foreach ($item as $value) {
        // Recursive call to this fuction with  value.
        if (!$this->validateSafeString($value)) {
          // Returned as not safe.
          $is_safe = FALSE;
          // Stop here, already know an unsafe value is present.
          break;
        }
      }
    }
    // Else, have a string and it contains unsafe values?
    elseif (is_string($item) && !locale_string_is_safe($item)) {
      $is_safe = FALSE;
    }
    
    return $is_safe;
  }
?>

Removing old hashed tag.

You don't really need a recursive function, array_walk_recursive() will do recursive part for you.

Bring on D8MI sprint.

I don't think that following #2512460: "Translate user edited configuration" permission needs to be marked as restricted that landed earlier last week this one would be still critical given that XSS attacks from trusted permissions are not considered XSS attacks. I still believe that this filtering would ideally be added, but don't think this is a critical still.

Ok I've updated my patch to use array_walk_recursive() to check the submitted value (is it is an array).

I have also reworded the error message to be a bit more generic, so that do not have to worry about the display of the field label in the message.

Still having a hard time with Drupal highlighting the field with the error on it. I'm following the conventions for targeting the field by name, but it does not seem to work. Anyone able to help with this? find the right pattern?

Also as @dawehner mentioned and tagged, we should create a test for this use case. I'm not the best at writing tests so I'd welcome others on that front.

+++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
@@ -195,6 +195,40 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
+          $field_name = 'translation[config_names][' . $name . '][' . $key;

As per the docs:

If the #parents property of your form element is array('foo', 'bar', 'baz') then you may set an error on 'foo' or 'foo][bar][baz'.

So you should replace the [ after translation with ][. Basicaly ][ is the separator between the parent keys here.

D'oh. Wow, I cannot believe I missed that. Thanks for pointing it out Gabor.

New patch with interdiff attacched. All we need now is a test!

Let's run testbot on it!

@pwolanin: do we release SAs for attacks performed with trusted/restricted permissions? I don't think so based on https://www.drupal.org/security-advisory-policy

Here is an updated figure based on the one I posted at #2512460: "Translate user edited configuration" permission needs to be marked as restricted.

I think I made my points. Marking for maintainer triage.

Been told that the tag is superfluous and the issue will be triaged anyway.

@pwolanin: "resolving" this issue with the XSS filtering used in locale will only make it as safe as locale, which still allows translators to own a site which is why we say it as such on https://www.drupal.org/security-advisory-policy, so I am not sure if before this issue XSS was possible and after this XSS is possible would qualify this as a critical issue.

How bad of a performance hit would it be to xss filter all translations in TranslationManager?
That way it doesn't matter what translation permissions people have or where the po files came from.

Could this be made to be an option? (slow safe translations vs fast unsafe translations)

@JvE: that would not solve the problem either AFAIS. The underlying issue is that we cannot assume HTML output and that a translated string will be used at a HTML tag level. The output may be XML, JSON, a HTTP header, whatever and even in HTML it may be in an attribute. Do you see a way to do API level filtering for ensuring values that do not allow you to break out of HTML attributes or XML or JSON and inject malicious content or inject HTTP headers or unwanted HTML tags and whatever other output? The validation used in locale only covers the HTML tag case and so would this issue.

The XSS filtering in locale on form input and .po import is a best effort safety measure for the 80% case of attacks but is far from being a complete solution, therefore the explanation in https://www.drupal.org/security-advisory-policy that interface translation allows you to own the site. As explained in #25, the overlap with config translation makes config translation equally dangerous. I did make a mistake in #2512460: "Translate user edited configuration" permission needs to be marked as restricted assuming that marking it as a restricted permission will signal this but the letter of the policy does not indicate that, it would need to be listed on its own in https://www.drupal.org/security-advisory-policy.

I don't believe that the proposed ways so far to resolve this issue would resolve the full spectrum of attacks possible (which we did not see good ways with locale either, therefore the listing as a dangerous permission), so that is why I don't see how it is a critical issue to port the best effort validation for the common cases from locale, because it is not complete there either. It does not matter if the inadequate filtering is done on the API level or the UI level, its inadequate.

BTW the HTML attribute question is mitigated mostly by AttributeString::__toString() in Drupal 8 escaping quotes. So that works for rendered markup. At least the possible attack vectors I tried did not work... There are still plenty of places where HTML is assembled manually, eg. looking through hook_help() may uncover opportunities for going into HTML attributes, and then of course the wast array of contribs. (And then this is just the HTML stuff).

Agreed with Gabor with one possible exception documented below. We should do this, but it's 'just' hardening with the permissions change.

The only thing that makes me slightly hesitant is that existing Drupal 8 sites might have assigned this permission, without it having been restricted - then later someone untrustworthy gets granted the role and bam!.

Without the validation, that's a potential lurking vulnerability in sites installed prior to #2512460: "Translate user edited configuration" permission needs to be marked as restricted. It's also something that could survive a complete re-install since you could have that role/permission in exported config.

With the validation, then only a site that was actually compromised prior to #2512460: "Translate user edited configuration" permission needs to be marked as restricted would be affected, and tough luck for them 'cos beta.

I don't mind either way, but wanted to document that nagging feeling on the issue. This isn't really about the hardening itself being critical or not, but whether we provide an upgrade path/backwards compatibility (via the hardening) for existing installs/default config.

Alright, downgrading based on discussion with @catch, @Gábor Hojtsy, @effulgentsia, and @alexpott as per the above.

I do agree that we should create that change record. I reopened #2512460: "Translate user edited configuration" permission needs to be marked as restricted for that.

Based on the comment thread, this issue seems to be tied to a bigger discussion. With the scope of this issue around validating when config translations are input, is the patch/approach I have provided still viable? Or do we need a patch that goes into a different direction?

@mikemiles86: I think the patch makes sense as a hardening for config translation. Locale does not provide a more complete solution either. The only caveat I see with the validation is there are the text_format typed elements, which result in config filtered on output with an input format, so will filter/escape on output for sure (and may need to allow for inline JS, etc). So those should be skipped in validation.

1. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
   @@ -195,6 +195,40 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
   +    $form_values = $form_state->getValue(array('translation', 'config_names'));
   +    foreach ($this->mapper->getConfigNames() as $name) {
   +      foreach ($form_values[$name] as $key => $value) {
   

   Minor, but is the call out to getConfigNames() really necessary? I.e. are we expecting something in $form_values that is not in getConfigNames()?

2. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
   @@ -195,6 +195,40 @@ public function buildForm(array $form, FormStateInterface $form_state, Request $
   +          $form_state->setErrorByName($field_name, $this->t('The submitted string contains disallowed HTML.'));
   

   Haven't tried this out but it seems the error message is confusing in case the value was in fact an array?

Yeah, the error setting is not going to fly in that way. See the attached screenshot. If malicious input is found inside of a nested element all elements within the top-level one are being marked as having an error.

We should add the validation to the form elements themselves, i.e. we need a corresponding method on config translation's ElementInterface.

Working on that now.

Here we go. I think something along these lines is a little bit cleaner, and as proven by the screenshot solves the problem noted above.

Still needs tests.

Looks better! Also looks like the error message text needs HTML removed still :)

Here we go.

Re, the error message: Yes, but that's a separate issue, no?

#fail

"Re, the error message: Yes, but that's a separate issue, no?", this issue introduces the error message, so it should not introduce one with escaped HTML in it AFAIS.

Here we go. Fixed the escaping issue.

Meh, FormErrorHandler really is a weird beast. But it really makes no sense to escape form element titles as is proven by this example.

I think I've found an acceptable - albeit not exactly super nice - solution.

1. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -83,8 +83,9 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
   +        $error_links[] = SafeMarkup::escape($this->l(['#markup' => $title], Url::fromRoute('<none>', [], ['fragment' => $form_element['#id'], 'external' => TRUE])));
   

   So this allows to also use HTML, but still have some sort of safe HTML.

2. +++ b/core/lib/Drupal/Core/Form/FormErrorHandler.php
   @@ -98,6 +99,7 @@ protected function displayErrorMessages(array $form, FormStateInterface $form_st
          ]);
   +
          $this->drupalSetMessage($message, 'error');
   

   Nitpick: out of scope ...

3. +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
   index e9d48e4..a593fff 100644
   --- a/core/modules/config_translation/src/ConfigFieldMapper.php
   
   --- a/core/modules/config_translation/src/ConfigFieldMapper.php
   +++ b/core/modules/config_translation/src/ConfigFieldMapper.php
   
   +++ b/core/modules/config_translation/src/ConfigFieldMapper.php
   +++ b/core/modules/config_translation/src/ConfigFieldMapper.php
   @@ -7,6 +7,8 @@
   

   It would be great to document that/why we went with the approach to merge the two translation pagers to one.

4. +++ b/core/modules/config_translation/src/FormElement/FormElementBase.php
   @@ -170,10 +179,25 @@ protected function getTranslationElement(LanguageInterface $translation_language
   +   * @param $element
   +   *   The form element to validate.
   ...
   +  public static function validateInput($element, FormStateInterface $form_state) {
   

   Do we know whether this is an array?

5. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
   @@ -610,6 +626,46 @@ public function testViewsTranslationUI() {
   +    $this->assertText(SafeMarkup::checkPlain($translatable_field_setting));
   ...
   +    $this->assertText(SafeMarkup::checkPlain($translatable_storage_setting));
   

   What about using htmlspecialchars so we don't need another Safemarkup call

Thanks for the review. This should fix the mentioned items and hopefully also the tests. I had included the patch from #2409701: Field storage configuration is not exposed to config translation UI in the last one, that's why the removal of that is now in the interdiff. This one should be cleaner.

Let's see if this fixes the test.

Hi, maybe late, but just some thoughts about this one:

- Whatever validation we do, it should be applied to the original config strings too (the one editable by the admin). Otherwise we end up with config strings that just cannot be translated as they deserve. I.e. source string containing HTML that cannot be translated. Wouldn't it make sense to just prevent this HTML in the original string in the first place?

- An alternate, more flexible solution would be to apply validation or not depending on user role/permissions. This way, if the admin wants to add the same -dirty- html to the translation that is in the original string, he could. So there are strings that cannot be translated by regular translators but the admin should be able to translate them. Same for locale.

- Provided "Translate configuration" is a restricted permission... I don't think this makes too much sense anyway. (Gábor #16). Let's say regular translators go through some validation with the locale UI, but "configuration translators" can skip that validation -and yes, add their js or their XSS-

Whatever, my thought is: if we want to say Drupal 8 is fully translatable, we don't allow strings anywhere that are not translatable. Or if we allow a string in the configuration it *must* be translatable.

Can we just make an effort, relax, and assume "restricted permissions" allow users to do their own XSS?

@Jose: I don't think we can (or should) filter all config for XSS generally, that would be possibly problematic for things with an input format for example (eg. views header text), so at least it would need to be element specific. Also elements may be entirely escaped when used and may want to allow "HTML tags", eg. if your label for a views table column includes "Uses of

@Gábor,

Ok, I get it.

The main issue I'm pointing out is that if we don't allow the same markup for the configuration translation than for the original configuration the functionality is inconsistent too. And worse, it is a more limiting issue.

On top of that... what is a configuration translation / what is the original configuration? Well, that depends on the default language of the site actually, this adds up to the inconsistencies.

About locale translation / UI consistency maybe we better just keep strings out of it when the source translation doesn't pass 'locale_string_is_safe' validation. That would be better consistency if that is our main concern. And better usability, because it is not easily understandable when there's a source string you cannot translate using the same format.

I agree with @Jose Reyero: If we don't check whether the source config passes the locale_string_is_safe() test, we are presenting the user with an impossible to complete UI if make the translation pass that test.

Here's an attempt at solving that: If the source config is not safe, simply display an error message. The rest of the config can still be translated normally. If people agree here on the basic approach, we should probably get a usability review on this.

I also updated the PluralVariant form element for the validation added here now that #2454829: Configuration translation UI does not support plural sources/targets is in.

Still needs tests btw.

@tstoeckler,

That's certainly some usability improvement like "hey guy, do not try to translate this, you'll get stuck". We shouldn't be able to reach this point though. The same validation should be used for config editor and config translator (either for both of them or for none of them).

IMO we better forget about configuration translation and just fix it in the locale translation UI. Since both of them need different permissions we should be fine.

Then, what about locale_string_is_safe() being used for validating *source* strings:
a) in po extractor
b) before adding any source string to locale tables
and throwing some big "You cannot use t() for that ugly string" error (Feel free to use the string in your module without 't' but don't mess with translators' life please).

So, short points:

1. Untranslatable strings should never be fed into the locale system. This is a BUG.

2. If they can be added to configuration, they must be translatable somehow. (They are now so we are fine as it is)

There's still the issue of (1) untranslatable strings being added to configuration, (2) configuration exported to module/profile, and (3) module/profile installed somewhere else. At this point the locale system will try to add that source string and it should throw at least a WARNING: You are intalling some configuration that is not translatable.

I still think this is no security issue, just a usability one.

(There's also the BIG usability issue of putting HTML In front of translators who may know a few languages but none of them is HTML. This one is for Drupal 9 I guess...)

That's certainly some usability improvement like "hey guy, do not try to translate this, you'll get stuck". We shouldn't be able to reach this point though. The same validation should be used for config editor and config translator (either for both of them or for none of them).

I fail to see how that would be possible. In Config Translation it's both possible and needed because the forms are entirely auto-generated, so we cannot employ custom validation for those strings that we know will end up on the user interface. I don't think it's possible to perform this validation generically for all configuration.

Regarding your suggestion: Are you basically proposing to move the locale_string_is_safe() check into LocaleConfigSubscriber::saveCustomizedTranslation()? The problem with that is that you would enter things in Config Translation and they would either end up in Locale or not depending on what you put in. I think that could confuse users?

>...The same validation should be used for config editor and config translator (either for both of them or for none of them).

I fail to see how that would be possible...

Well, the 'none of them' option is an empty patch :-)

For the other option, it would mean generalizing the 'locale_string_is_safe' to something like 'config_string_is_safe' to be used for config/string validation that would need to be used also when Locale is not enabled, certainly way too complex... And anyway as an occasional site admin the last thing I'd like to see is my configuration options being limited . But as a site translator I would see not being able to translate that configuration string as a bug.

The point I'm trying to make here is why this cannot be fixed by only limiting "configuration translation" because that would introduce worse inconsistencies. Still, we can fix inconsistencies in the locale system not allowing such strings into it.

Maybe the title of this issue is just misleading and instead of "Config translation needs to be validated..." it should be "Some configuration strings cannot be translated using the Interface translation page / with regular interface translation permission"

No, I don't think config translation needs to be validated at all -at least no more than configuration itself.

Removing from sprint. Last worked on 5 months ago.

Tagging to yield some eyes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled patch against 10.1.x.Please review

CI failures.

Did not review yet.

Fixed custom command failure

Uploading patch after fixing custom command failure

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I noticed that the error pattern #^Variable \$form_output in empty\(\) always exists and is not falsy\.$# is being ignored in the file /var/www/html/core/modules/system/tests/src/Functional/Form/FormTest.php. I understand that this is a false positive caused by the Drupal FormTestBase class. This class includes a method called assertEmpty(), which checks that a variable is both empty and not falsey.

Rerolled 2512466-83.patch against 10.1.x and updated dictionary.txt file.

Left a small comment and fails appear legit.

Glancing at the issue summary and seem it could use a little love.

Specifically what is the proposed solution.
Testing steps still the same?

Not 100% sure if the new function for the FormElement may need a change record.