Project

General

Profile

Actions

Bug #82054

closed

f:link.external does not filter insecure URI schemes

Added by Oliver Hader over 6 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Fluid
Start date:
2017-08-07
Due date:
% Done:

0%

Estimated time:
TYPO3 Version:
7
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:
Needs Decision

Description

The Fluid view-helper f:link.external directly uses the given URI without further sanitizing it as a link. In the TYPO3 core we strip away javascript: and data: URI schemes. This has been integrated into ContentObjectRender::typoLink with the following security bulletin https://typo3.org/teams/security/security-bulletins/typo3-core/typo3-core-sa-2015-012/

Payload:
  • <f:link.external uri="{uri}" target="_blank">Some link</f:link.external>
  • uri variable containing javascript:alert('XSS')

ELTS effects: 4.5, 6.2


Files

82054.diff (1.03 KB) 82054.diff Jigal van Hemert, 2017-08-08 22:54
Actions #1

Updated by Oliver Hader over 6 years ago

  • Description updated (diff)
Actions #2

Updated by Oliver Hader over 6 years ago

  • Description updated (diff)
Actions #3

Updated by Jigal van Hemert over 6 years ago

Is this an acceptable solution?

Actions #4

Updated by Helmut Hummel over 6 years ago

Do we need to do anything here at all?

I mean you can perfectly write this in your template:

<a href="{data.foo}">Bar</a>

There is no way on our side to prevent it.

In general the usefulness of <f:link.external> is close to 0.

So I'd rather deprecate the whole view helper instead of trying to fix it somehow.

Actions #5

Updated by Markus Klein over 6 years ago

So I'd rather deprecate the whole view helper instead of trying to fix it somehow.

Full ack!

Actions #6

Updated by Benni Mack over 6 years ago

  • Target version set to next-patchlevel
  • Affected Version set to 6.2, 7, 8, master

Needs to be fixed in uri.external and link.external by adding the same functionality as in ContentObejctRenderer.php (master line 5248)
if (in_array(strtolower(trim($linkHandlerKeyword)), ['javascript', 'data'], true)) { - do not link it - }

Actions #7

Updated by Oliver Hader over 6 years ago

  • Status changed from New to Accepted
Actions #8

Updated by Oliver Hader over 6 years ago

  • Target version changed from next-patchlevel to Release September 2017
Actions #9

Updated by Helmut Hummel over 6 years ago

I still kinda think that this does not need a fix.

ContentObjectRenderer is different because typolink
is very commonly used on user input from editors, where
the scope here rather is integrators/ admins.

If admins use this vh insecurely in a template it is in them
to fix it.

Fixing this here would be breaking for legit usages
if this vh (still wondering the usefulness of it in general
but why would a fixed string url with data: or javascript:
should be denied, while having it in a <a> tag not)

Actions #10

Updated by Helmut Hummel over 6 years ago

I would rather make this public and let "the community" decide whether
we should help admins not shoot in their foot

Actions #11

Updated by Benni Mack over 6 years ago

If we handle it in public, I'm actually for deprecating this viewhelper or actually removing it in v9 completely.

Actions #12

Updated by Oliver Hader over 6 years ago

I think the assumption is correct, that people rather would have used the TypolinkViewHelper instead of the ExternalViewHelper if it's user input in general. Thus, I'd opt for making this issue public then and let the community comment on it.

Actions #13

Updated by Oliver Hader over 6 years ago

  • Category deleted (OW-A07: Cross Site Scripting)

I think the assumption is correct, that people rather would have used the TypolinkViewHelper instead of the ExternalViewHelper if it's user input in general. Thus, I'd opt for making this issue public then and let the community comment on it.

Actions #14

Updated by Oliver Hader over 6 years ago

  • Project changed from 1716 to TYPO3 Core
  • Target version deleted (Release September 2017)
Actions #15

Updated by Oliver Hader over 6 years ago

  • Status changed from Accepted to Needs Feedback
Actions #16

Updated by Riccardo De Contardi over 6 years ago

I guilty neglected this one for too long - I am of the opinion to remove <f:link.external> too, as I don't remember much benefits respect to a plain <a href="">

Actions #17

Updated by Alexander Opitz about 6 years ago

Do we need proper deprecation here?

Actions #18

Updated by Susanne Moog over 5 years ago

  • Sprint Focus set to On Location Sprint
Actions #19

Updated by Benni Mack over 5 years ago

  • Target version set to Candidate for Major Version

Let's deprecate this one in v10.0

Actions #20

Updated by Oliver Hader over 4 years ago

  • Category set to Fluid
Actions #21

Updated by Susanne Moog over 4 years ago

  • Sprint Focus deleted (On Location Sprint)
Actions #22

Updated by Georg Ringer about 4 years ago

  • Sprint Focus set to Needs Decision
Actions #23

Updated by Benni Mack over 2 years ago

  • Status changed from Needs Feedback to Closed

This has been deprecated now in TYPO3 v11.

Actions

Also available in: Atom PDF