The issue was reported first on security.drupal.org where we agreed to fix it in the public queue. Members who suggested it: greggles, larowlan and mlhess.

Problem/Motivation

l() function does not strip dangerous protocols when external parameter is set to TRUE.
When it returns the output it only calls check_plain for sanitize the path that is given from url() - because comment also indicates it's a plain text URL, it should be sanitized before, but currently it's not.
For reproduction steps, you can try one of the following:
Scenario No 1:
1. Create a new test module in drupal.
2. Implement a hook_menu that will call a custom function.
3. In the page callback return the output of l() like this:
l('link', "javascript:alert('XSS')", array('external' => TRUE))
Scenario No 2:
1. Turn on PHP module.
2. Create a new article content and use the php filter format and insert to body the following:
print l('link', "javascript:alert('XSS')", array('external' => TRUE));
3. Save the content, visit its page and click on the 'link' link, then the alert will be executed.

Both of the above scenarios need more permissions on the website that could lead more serious issues, that's also the reason why we turned to public hardening.

Proposed resolution

As l() function is widely used in drupal core, also in contribs, changing the way how it prints the output could lead to breaking functionalities, the proposal is to sanitize the path before it's given to sanitization functions that do the other jobs. See currently attached patch where the drupal_strip_dangerous_protocols() function is directly called on $path.

Remaining tasks

* Agree on proposed solution
* Adapt or even extend tests for edge cases of usage of external parameter of l().
* Manual review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#11 interdiff_9-11.txt395 bytespoker10
#11 2990723-11.patch1.22 KBpoker10
#11 2990723-11_test-only.patch811 bytespoker10
#9 2990723-9.patch1.21 KBpoker10
#9 2990723-9_test-only.patch811 bytespoker10
#4 2990723-img.png46.09 KBminakshiPh
#2 2990723-2.patch426 bytestatarbj
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tatarbj created an issue. See original summary.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
CreditAttribution: tatarbj commented
Status: Active » Needs review
FileSize
2990723-2.patch426 bytes
minakshiPh’s picture

minakshiPh CreditAttribution: minakshiPh as a volunteer commented
Assigned: Unassigned » minakshiPh
minakshiPh’s picture

minakshiPh CreditAttribution: minakshiPh as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
2990723-img.png46.09 KB

@tatarbj,

The patch #2 applied successfully

$ git apply -v 2990723-2.patch.txt
Checking patch includes/common.inc...
Hunk #1 succeeded at 2545 (offset 6 lines).
Applied patch includes/common.inc cleanly.

Please find the attached screen shot

2990723-img.png

after applying the patch.

minakshiPh’s picture

minakshiPh CreditAttribution: minakshiPh as a volunteer commented
Assigned: minakshiPh » Unassigned
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Should there be test coverage for this? Or is the fact that there's coverage for drupal_strip_dangerous_protocols() in modules/simpletest/tests/common.test good enough?

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
CreditAttribution: tatarbj commented

Hi @DamienMcKenna,
I believe the as testBadProtocolStripping() checks with check_url() it should be OK to consider this improvement already covered by tests.
Bests,
Balazs.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
CreditAttribution: tatarbj at Petend for European Commission and European Union Institutions, Agencies and Bodies commented
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
FileSize
2990723-9_test-only.patch811 bytes
2990723-9.patch1.21 KB

Patch #2 look good, thanks! I have added a simple test to check if l() is sanitizing the external URLs correctly. Patch #2 is unchanged, only one test case added.

The last submitted patch, 9: 2990723-9_test-only.patch, failed testing. View results

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Reviewed & tested by the community » Needs review
FileSize
2990723-11_test-only.patch811 bytes
2990723-11.patch1.22 KB
interdiff_9-11.txt395 bytes

It seems like that some calls to l() use NULL as the second parameter which causes problems in PHP 8.1. Added cast to string in the drupal_strip_dangerous_protocols() call.

The last submitted patch, 11: 2990723-11_test-only.patch, failed testing. View results

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record, +RTBM

This could break some sites' functionality if they're using l() to produce unusual links e.g. javascript:foo();.

I think that's outweighed by the benefit of the extra sanitisation.

However, it'll need a Change Record.

TIL that telnet:// is one of the allowed protocols :)

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Should the test coverage be extended to include telnet://, for protocol compliance? The only test coverage that lists telnet:// is modules/filter/filter.test.

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Issue tags: -Needs change record

I have drafted a change record here: https://www.drupal.org/node/3307791

@DamienMcKenna Yes, it seems like that currently the CommonXssUnitTest::testBadProtocolStripping() function check only javascript protocol stripping (using drupal_strip_dangerous_protocols()). It doesn't check all allowed protocols whether they are kept or stripped. I think we can extend this function and add an array of strings with various protocols (similar to what is used in the FilterUnitTestCase::testUrlFilter()) to test the stripping with the drupal_strip_dangerous_protocols() completely. But I propose to create a new follow-up issue for this (can create that).

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

@poker10: Sounds like a reasonable approach. I made a few minor edits on the change record.

  • mcdruid committed 9b89fa9 on 7.x 
    Issue #2990723 by poker10, tatarbj, DamienMcKenna: Security improvement...
mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -RTBM

Thanks Balazs (@tatarbj) and everyone that worked on this!

poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented

I have created a follow-up issue for updating the tests, as was mentioned in #15: #3308471: [D7] Update CommonXssUnitTest::testBadProtocolStripping() to check other allowed / dangerous protocols

Status: Fixed » Closed (fixed)

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