CVE-2023-2361: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Cross-site Scripting (XSS) - Stored in GitHub repository pimcore/pimcore prior to 10.5.21.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2023-04-28T08:15Z
Repository: https://github.com/pimcore/pimcore
Language: PHP
Files changed: doc/Development_Documentation/03_Documents/01_Editables/18_Link.md, doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md, lib/Security/SecurityHelper.php, models/Document/Editable/Link.php
Commit date: 2023-04-24 15:38:21 +0200
Commit message: [Security] Fix xss in link editable (#14986)

* 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>
Diff: diff --git a/doc/Development_Documentation/03_Documents/01_Editables/18_Link.md b/doc/Development_Documentation/03_Documents/01_Editables/18_Link.md
index 7c01e3fa30a9..5025dcc0231e 100644
--- a/doc/Development_Documentation/03_Documents/01_Editables/18_Link.md
+++ b/doc/Development_Documentation/03_Documents/01_Editables/18_Link.md
@@ -4,21 +4,23 @@
 
 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                                                             |
 |----------|----------|-------------------------------------------------------------------------|
 | `reload` | boolean  | Set to true to reload the page in editmode after changing the state.    |
 | `textPrefix` | string  | Add an icon or something else before Text    |
 | `textSuffix` | string  | Add an icon or something else after Text    |
 | `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                          |
 |-------------------|-------------|--------------------------------------|
 | `getHref()`       | string      | Get the path of this link            |
diff --git a/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md b/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
index 3a870e9a7932..6dd36a895cad 100644
--- a/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
+++ b/doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md
@@ -1,9 +1,10 @@
 # Upgrade Notes
 
 ## 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).
 
 
diff --git a/lib/Security/SecurityHelper.php b/lib/Security/SecurityHelper.php
index bc1f9f537c46..9deeb9d4dbdc 100644
--- a/lib/Security/SecurityHelper.php
+++ b/lib/Security/SecurityHelper.php
@@ -35,6 +35,15 @@ class SecurityHelper
             if (array_key_exists($key, $array)) {
                 $array[$key] = self::convertHtmlSpecialChars($array[$key]);
             }
         }
     }
+
+    public static function sanitizeHtmlAttributes(?string $text): ?string
+    {
+        if(is_string($text)) {
+            return preg_replace('/[\/"\'\\\]/', '', $text);
+        }
+
+        return null;
+    }
 }
diff --git a/models/Document/Editable/Link.php b/models/Document/Editable/Link.php
index d86fc3202b09..52cf9ee2f3d6 100644
--- a/models/Document/Editable/Link.php
+++ b/models/Document/Editable/Link.php
@@ -17,10 +17,11 @@ namespace Pimcore\Model\Document\Editable;
 
 use Pimcore\Logger;
 use Pimcore\Model;
 use Pimcore\Model\Asset;
 use Pimcore\Model\Document;
+use Pimcore\Security\SecurityHelper;
 
 /**
  * @method \Pimcore\Model\Document\Editable\Dao getDao()
  */
 class Link extends Model\Document\Editable implements IdRewriterInterface, EditmodeDataInterface
@@ -134,23 +135,11 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
                 'media',
                 'download',
                 'ping',
                 'type',
                 'referrerpolicy',
-                'xml:lang',
-                'onblur',
-                'onclick',
-                'ondblclick',
-                'onfocus',
-                'onmousedown',
-                'onmousemove',
-                'onmouseout',
-                'onmouseover',
-                'onmouseup',
-                'onkeydown',
-                'onkeypress',
-                'onkeyup',
+                'xml:lang'
             ];
             $defaultAttributes = [];
 
             if (!is_array($this->data)) {
                 $this->data = [];
@@ -164,13 +153,13 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
                 if ((is_string($value) || is_numeric($value))
                     && (strpos($key, 'data-') === 0 ||
                         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).'"';
                     }
                 }
             }
 
             $attribs = array_unique($attribs);
@@ -240,12 +229,11 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
         if (strlen($this->data['parameters'] ?? '') > 0) {
             $url .= (strpos($url, '?') !== false ? '&' : '?') . str_replace('?', '', $this->getParameters());
         }
 
         if (strlen($this->data['anchor'] ?? '') > 0) {
-            $anchor = $this->getAnchor();
-            $anchor = str_replace('"', urlencode('"'), $anchor);
+            $anchor = str_replace('"', urlencode('"'), $this->getAnchor());
             $url .= '#' . str_replace('#', '', $anchor);
         }
 
         return $url;
     }
@@ -297,10 +285,14 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
                     }
                 }
             }
         }
 
+        if($editmode) {
+            unset($this->data['attributes']);
+        }
+
         // sanitize attributes
         if (isset($this->data['attributes'])) {
             $this->data['attributes'] = htmlspecialchars($this->data['attributes'], HTML_ENTITIES);
         }
 
@@ -338,19 +330,19 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
     /**
      * @return string
      */
     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']) ?? '';
     }
 
     /**
      * @return string
      */
@@ -362,35 +354,35 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
     /**
      * @return string
      */
     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']) ?? '';
     }
 
     /**
      * @return mixed
      */
@@ -407,10 +399,18 @@ class Link extends Model\Document\Editable implements IdRewriterInterface, Editm
         $this->data = \Pimcore\Tool\Serialize::unserialize($data);
         if (!is_array($this->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;
     }
 
     /**
      * {@inheritdoc}

Vulnerable code: 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)), 
                'xml:lang',
                'onblur',
                'onclick',
                'ondblclick',
                'onfocus',
                'onmousedown',
                'onmousemove',
                'onmouseout',
                'onmouseover',
                'onmouseup',
                'onkeydown',
                'onkeypress',
                'onkeyup',
                        $attribs[] = $key.'="'. $this->data[$key] .' '. $this->config[$key] .'"';
                        $attribs[] = $key.'="'.$value.'"';
            $anchor = $this->getAnchor();
            $anchor = str_replace('"', urlencode('"'), $anchor);
        return $this->data['parameters'] ?? '';
        return $this->data['anchor'] ?? '';
        return $this->data['rel'] ?? '';
        return $this->data['tabindex'] ?? '';
        return $this->data['accesskey'] ?? '';
        return $this->data['class'] ?? '';
Fixed code: 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)), 
> 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!

- [Editable] Removed all `on*` attributes from the `$allowedAttributes` list due to security reasons. These attributes are not allowed anymore in the "attributes" field.

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

        return null;
    }
use Pimcore\Security\SecurityHelper;
                'xml:lang'
                        $attribs[] = $key.'="'. SecurityHelper::sanitizeHtmlAttributes($this->data[$key]) .' '. SecurityHelper::sanitizeHtmlAttributes($this->config[$key]) .'"';
                        $attribs[] = $key.'="'.SecurityHelper::sanitizeHtmlAttributes($value).'"';
            $anchor = str_replace('"', urlencode('"'), $this->getAnchor());
        if($editmode) {
            unset($this->data['attributes']);
        }

        return SecurityHelper::sanitizeHtmlAttributes($this->data['parameters']) ?? '';
        return SecurityHelper::sanitizeHtmlAttributes($this->data['anchor']) ?? '';
        return SecurityHelper::sanitizeHtmlAttributes($this->data['rel']) ?? '';
        return SecurityHelper::sanitizeHtmlAttributes($this->data['tabindex']) ?? '';
        return SecurityHelper::sanitizeHtmlAttributes($this->data['accesskey']) ?? '';
        return SecurityHelper::sanitizeHtmlAttributes($this->data['class']) ?? '';
        //sanitize fields
        $fieldsToExclude = ['path'];
        foreach($this->data as $key => $value) {
            if(!in_array($key, $fieldsToExclude)) {
                $this->data[$key] = SecurityHelper::sanitizeHtmlAttributes($value);
            }
        }