Skip to content

Commit

Permalink
[Security] Fix xss in link editable (#14986)
Browse files Browse the repository at this point in the history
* fixed xss in link editable

* added `sanitizeHtmlAttributes` to some other fields

* sanitized some more fields

* added update notes

* Update 18_Link.md

---------

Co-authored-by: robertSt7 <robert.steinkellner@pimcore.com>
  • Loading branch information
Corepex and robertSt7 committed Apr 24, 2023
1 parent b57ba39 commit 6970649
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 24 deletions.
Expand Up @@ -6,7 +6,7 @@ The link editable is used for dynamic link creation in documents.

## Configuration

You can pass every valid attribute an `<a>`-tag can have ([w3.org - Link](https://www.w3.org/TR/html52/textlevel-semantics.html#the-a-element)),
You can pass nearly every valid attribute an `<a>`-tag can have ([w3.org - Link](https://www.w3.org/TR/html52/textlevel-semantics.html#the-a-element)),
such as: `class`, `target`, `id`, `style`, `accesskey`, `name`, `title`, `data-*`, `aria-*` and additionally the following:

| Name | Type | Description |
Expand All @@ -17,6 +17,8 @@ such as: `class`, `target`, `id`, `style`, `accesskey`, `name`, `title`, `data-*
| `noText` | boolean | If you need only the `<a>` tag without text (or only with an textSuffix/TextPrefix) |
| `required` | boolean/string | (default: false) set to true to make link and text required for publish, set to "linkonly" to make only the link required for publish |

> For security reasons we created an [allow list](https://github.com/pimcore/pimcore/blob/9bf18aca55e5303661c68835c950412a428cf616/models/Document/Editable/Link.php#L115-L141) to filter harmfull html attributes. For example all `on*` attributes will be filtered out!
## Methods

| Name | Return | Description |
Expand Down
Expand Up @@ -2,6 +2,7 @@

## 10.5.21
- [Assets] The Asset `Import from Server` feature is now only available for admins. It will be removed in Pimcore 11
- [Editable] Removed all `on*` attributes from the `$allowedAttributes` list due to security reasons. These attributes are not allowed anymore in the "attributes" field.

## 10.5.13
- [Web2Print] Print document twig expressions are now executed in a sandbox with restrictive security policies (just like Sending mails and Dataobject Text Layouts introduced in 10.5.9).
Expand Down
9 changes: 9 additions & 0 deletions lib/Security/SecurityHelper.php
Expand Up @@ -37,4 +37,13 @@ public static function convertHtmlSpecialCharsArrayKeys(array &$array, array $ke
}
}
}

public static function sanitizeHtmlAttributes(?string $text): ?string
{
if(is_string($text)) {
return preg_replace('/[\/"\'\\\]/', '', $text);
}

return null;
}
}
46 changes: 23 additions & 23 deletions models/Document/Editable/Link.php
Expand Up @@ -19,6 +19,7 @@
use Pimcore\Model;
use Pimcore\Model\Asset;
use Pimcore\Model\Document;
use Pimcore\Security\SecurityHelper;

/**
* @method \Pimcore\Model\Document\Editable\Dao getDao()
Expand Down Expand Up @@ -136,19 +137,7 @@ public function frontend()
'ping',
'type',
'referrerpolicy',
'xml:lang',
'onblur',
'onclick',
'ondblclick',
'onfocus',
'onmousedown',
'onmousemove',
'onmouseout',
'onmouseover',
'onmouseup',
'onkeydown',
'onkeypress',
'onkeyup',
'xml:lang'
];
$defaultAttributes = [];

Expand All @@ -166,9 +155,9 @@ public function frontend()
strpos($key, 'aria-') === 0 ||
in_array($key, $allowedAttributes))) {
if (!empty($this->data[$key]) && !empty($this->config[$key])) {
$attribs[] = $key.'="'. $this->data[$key] .' '. $this->config[$key] .'"';
$attribs[] = $key.'="'. SecurityHelper::sanitizeHtmlAttributes($this->data[$key]) .' '. SecurityHelper::sanitizeHtmlAttributes($this->config[$key]) .'"';
} elseif (!empty($value)) {
$attribs[] = $key.'="'.$value.'"';
$attribs[] = $key.'="'.SecurityHelper::sanitizeHtmlAttributes($value).'"';
}
}
}
Expand Down Expand Up @@ -242,8 +231,7 @@ public function getHref()
}

if (strlen($this->data['anchor'] ?? '') > 0) {
$anchor = $this->getAnchor();
$anchor = str_replace('"', urlencode('"'), $anchor);
$anchor = str_replace('"', urlencode('"'), $this->getAnchor());
$url .= '#' . str_replace('#', '', $anchor);
}

Expand Down Expand Up @@ -299,6 +287,10 @@ private function updatePathFromInternal($realPath = false, $editmode = false)
}
}

if($editmode) {
unset($this->data['attributes']);
}

// sanitize attributes
if (isset($this->data['attributes'])) {
$this->data['attributes'] = htmlspecialchars($this->data['attributes'], HTML_ENTITIES);
Expand Down Expand Up @@ -340,15 +332,15 @@ public function getTarget()
*/
public function getParameters()
{
return $this->data['parameters'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['parameters']) ?? '';
}

/**
* @return string
*/
public function getAnchor()
{
return $this->data['anchor'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['anchor']) ?? '';
}

/**
Expand All @@ -364,31 +356,31 @@ public function getTitle()
*/
public function getRel()
{
return $this->data['rel'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['rel']) ?? '';
}

/**
* @return string
*/
public function getTabindex()
{
return $this->data['tabindex'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['tabindex']) ?? '';
}

/**
* @return string
*/
public function getAccesskey()
{
return $this->data['accesskey'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['accesskey']) ?? '';
}

/**
* @return mixed
*/
public function getClass()
{
return $this->data['class'] ?? '';
return SecurityHelper::sanitizeHtmlAttributes($this->data['class']) ?? '';
}

/**
Expand All @@ -409,6 +401,14 @@ public function setDataFromResource($data)
$this->data = [];
}

//sanitize fields
$fieldsToExclude = ['path'];
foreach($this->data as $key => $value) {
if(!in_array($key, $fieldsToExclude)) {
$this->data[$key] = SecurityHelper::sanitizeHtmlAttributes($value);
}
}

return $this;
}

Expand Down

0 comments on commit 6970649

Please sign in to comment.