Bug #82054

f:link.external does not filter insecure URI schemes

Added by Oliver Hader over 2 years ago. Updated 17 days 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:

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 over 2 years ago

  • Description updated (diff)

#2 Updated by Oliver Hader over 2 years ago

  • Description updated (diff)

#3 Updated by Jigal van Hemert over 2 years ago

Is this an acceptable solution?

#4 Updated by Helmut Hummel over 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 over 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 over 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 over 2 years ago

  • Status changed from New to Accepted

#8 Updated by Oliver Hader over 2 years ago

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

#9 Updated by Helmut Hummel over 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 over 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 over 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 over 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 over 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 over 2 years ago

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

#15 Updated by Oliver Hader over 2 years ago

  • Status changed from Accepted to Needs Feedback

#16 Updated by Riccardo De Contardi about 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 almost 2 years ago

Do we need proper deprecation here?

#18 Updated by Susanne Moog over 1 year ago

  • Sprint Focus set to On Location Sprint

#19 Updated by Benni Mack over 1 year ago

  • Target version set to Candidate for Major Version

Let's deprecate this one in v10.0

#20 Updated by Oliver Hader 6 months ago

  • Category set to Fluid

#21 Updated by Susanne Moog 17 days ago

  • Sprint Focus deleted (On Location Sprint)

Also available in: Atom PDF