Bug #82054

f:link.external does not filter insecure URI schemes

Added by Oliver Hader about 2 years ago. Updated 2 months ago.

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

0%

TYPO3 Version:
7
PHP Version:
Tags:
Complexity:
Is Regression:
Sprint Focus:
On Location Sprint

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

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

History

#1 Updated by Oliver Hader about 2 years ago

  • Description updated (diff)

#2 Updated by Oliver Hader about 2 years ago

  • Description updated (diff)

#3 Updated by Jigal van Hemert about 2 years ago

Is this an acceptable solution?

#4 Updated by Helmut Hummel about 2 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.

#5 Updated by Markus Klein about 2 years ago

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

Full ack!

#6 Updated by Benni Mack about 2 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 - }

#7 Updated by Oliver Hader about 2 years ago

  • Status changed from New to Accepted

#8 Updated by Oliver Hader about 2 years ago

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

#9 Updated by Helmut Hummel about 2 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)

#10 Updated by Helmut Hummel about 2 years ago

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

#11 Updated by Benni Mack about 2 years ago

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

#12 Updated by Oliver Hader about 2 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.

#13 Updated by Oliver Hader about 2 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.

#14 Updated by Oliver Hader about 2 years ago

  • Project changed from Core Security to TYPO3 Core
  • Target version deleted (Release September 2017)

#15 Updated by Oliver Hader about 2 years ago

  • Status changed from Accepted to Needs Feedback

#16 Updated by Riccardo De Contardi almost 2 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="">

#17 Updated by Alexander Opitz over 1 year ago

Do we need proper deprecation here?

#18 Updated by Susanne Moog 12 months ago

  • Sprint Focus set to On Location Sprint

#19 Updated by Benni Mack 12 months ago

  • Target version set to Candidate for Major Version

Let's deprecate this one in v10.0

#20 Updated by Oliver Hader 2 months ago

  • Category set to Fluid

Also available in: Atom PDF