Problem/Motivation

This was originally reported to the security team who decided it could be public
This issue was reported to IBM as a vulnerability in the IBM API Connect Developer Portal, however the actual vulnerability is in the underlying Drupal Core translation modules. The vulnerability allows one Drupal user with administrative permissions to create a page which can force another Drupal user to make a call to a server under the control of an attacker. I have tested this on a vanilla Drupal 10 site and the vulnerability is present there.

Steps to reproduce

1. Set up a HTTP server somewhere, this will be the target of the malicious request (I used "ncat -l -p 80 --keep-open" just to see the request come in, obviously an attacker would use a server that they control) This probably isn't needed, as reviewing browser network traffic is enough to see that a request was made.

2. Enable the four Drupal Core modules Language, Interface Translation, Content Translation and Configuration Translation

3. Add a second language under Manage->Configuration->Region and language->Languages

4. Navigate to Manage->People->Roles and create a new role with a name of

">'><img src="http://127.0.0.1/evil">

5. Navigate to the roles list and from the action menu for our malicious role select "Translate"

6. Choose a language and press the add button

The browser will make a call to the malicious server -- in my example I can clearly see the request come in:

Screenshot of browser network traffic log

Proposed resolution

Change $value = '<span lang="' . $source_language->getId() . '">' . nl2br($source_config) . '</span>'; to render as plain text string with \Drupal\Component\Utility\Html::escape

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#5 Screenshot 2025-02-06 at 12.21.45 PM.png456.62 KBakalata

Issue fork drupal-3440399

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

larowlan credited MrSzymon.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
akalata’s picture

akalata commented
Issue summary: View changes

Updating steps to reproduce.

akalata’s picture

akalata commented
Issue summary: View changes
StatusFileSize
new Screenshot 2025-02-06 at 12.21.45 PM.png456.62 KB

akalata opened merge request !11138

akalata’s picture

akalata commented
Title: SSRF vis role name on content translation screen » SSRF via role name on content translation screen
Assigned: Unassigned » akalata

Edit issue title

akalata’s picture

akalata commented
Status: Active » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: SSRF via role name on content translation screen » Malicious fingerprinting of visitors via role name on content translation screen

The title is SSRF (server side request forgery) which I understand to mean a request from the server to other machines on the server's network as described in portswigger and owasp.

The description includes:

The vulnerability allows one Drupal user with administrative permissions to create a page which can force another Drupal user to make a call to a server under the control of an attacker.

and

The browser will make a call to the malicious server

That is not a server-side request.

I can see how this would let an admin gain data about the site visitors, which is not ideal, but on most sites an admin who can enter translations can probably already make other changes to a site that would also gain information about a visitor.

I'm surprised if this is the only place where a user with only the ability to translate could insert an image. I think other strings are similarly not filtered.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems the test-only failure is passing when would expect to fail so think that needs tweaking https://git.drupalcode.org/issue/drupal-3440399/-/jobs/4273224

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

prudloff made their first commit to this issue’s fork.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I adjusted the test to fail without the fix.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

this does a strip tags, that seems wrong and the escape then is pointless?

if the source config has some HTML, you want to see that as the translation needs the same.

It should just escape and should then also assert the escaped markup

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I removed the strip_tags() call, I think it was added in order to not break ConfigTranslationUiModulesTest.
And I added an assertion with the escaped string (it needs to be precise because other parts of the page already contain the escaped payload in other places).

smustgrave’s picture

smustgrave commented
Assigned: akalata » Unassigned
Status: Needs review » Reviewed & tested by the community

Both tests appearing to be failing as expected

1) Drupal\Tests\config_translation\Functional\ConfigTranslationUiModulesTest::testBooleanFieldConfigTranslation
Behat\Mink\Exception\ExpectationException: The string "On label (with &lt;em&gt;HTML&lt;/em&gt; &amp; things) Boolean settings" was not found anywhere in the HTML response of the current page.
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:363
/builds/issue/drupal-3440399/core/tests/Drupal/Tests/WebAssert.php:559
/builds/issue/drupal-3440399/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiModulesTest.php:280
FAILURES!
Tests: 7, Assertions: 198, Failures: 1.

New test

1) Drupal\Tests\config_translation\Functional\ConfigTranslationUiTest::testLabelEscaping
Behat\Mink\Exception\ExpectationException: The string "<img src="http://127.0.0.1/evil">" appears in the HTML response of this page, but it should not.
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:380
/builds/issue/drupal-3440399/core/tests/Drupal/Tests/WebAssert.php:571
/builds/issue/drupal-3440399/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php:353
FAILURES!
Tests: 9, Assertion

Solution matches what's in the summary which seems to be written by security team.

Believe this one is good.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Do we know why the nl2br() was originally there? Is it possible that there are newlines in the string and we need to care about representing them correctly still?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Traced this back to an issue in the original config_translation module, where nl2br() was explicitly added to fix a bug: #1945184: Original value display is broken

Therefore I think we should keep this feature? (and probably add a test for it...) I think nl2br(Html::escape(...)) will work as expected.

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I added nl2br back.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Thanks! There was no test for the nl2br() before, let's not hold this up on adding one.

  • catch committed 1826fe3a on 10.5.x 
    Issue #3440399 by prudloff, akalata, larowlan, longwave, smustgrave,...

  • catch committed f583ccda on 10.6.x 
    Issue #3440399 by prudloff, akalata, larowlan, longwave, smustgrave,...

  • catch committed c51b3eb5 on 11.2.x 
    Issue #3440399 by prudloff, akalata, larowlan, longwave, smustgrave,...

  • catch committed db7346b6 on 11.x 
    Issue #3440399 by prudloff, akalata, larowlan, longwave, smustgrave,...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Just escaping the HTML looks sensible, it's needed anyway for translators.

Committed/pushed to 11.x and cherry-picked back through to 10.5.x, thanks!

catch closed merge request !11138

Status: Fixed » Closed (fixed)

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