From b8f90717251d968c49dc77f8c1e5912e2fbe0dff Mon Sep 17 00:00:00 2001
From: Oliver Hader <oliver@typo3.org>
Date: Thu, 21 Sep 2023 17:24:21 +0200
Subject: [PATCH] [SECURITY] Deny processing instructions

XML processing instructions (e.g. `<?xml>`)
are denied during the sanitization process.
---
 src/Behavior.php              | 10 ++++++++++
 src/Builder/CommonBuilder.php |  6 +++++-
 src/Visitor/CommonVisitor.php |  6 ++++++
 tests/CommonBuilderTest.php   | 12 ++++++++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/Behavior.php b/src/Behavior.php
index d26c10c..ae2b829 100644
--- a/src/Behavior.php
+++ b/src/Behavior.php
@@ -62,6 +62,11 @@ class Behavior
      */
     public const ENCODE_INVALID_CDATA_SECTION = 32;
 
+    /**
+     * in case an unexpected processing instruction (e.g. `<?xml>`) was found, encode the whole node as HTML
+     */
+    public const ENCODE_INVALID_PROCESSING_INSTRUCTION = 64;
+
     /**
      * @var int
      */
@@ -224,6 +229,11 @@ public function shallEncodeInvalidCdataSection(): bool
         return ($this->flags & self::ENCODE_INVALID_CDATA_SECTION) === self::ENCODE_INVALID_CDATA_SECTION;
     }
 
+    public function shallEncodeInvalidProcessingInstruction(): bool
+    {
+        return ($this->flags & self::ENCODE_INVALID_PROCESSING_INSTRUCTION) === self::ENCODE_INVALID_PROCESSING_INSTRUCTION;
+    }
+
     public function shallRemoveUnexpectedChildren(): bool
     {
         return ($this->flags & self::REMOVE_UNEXPECTED_CHILDREN) === self::REMOVE_UNEXPECTED_CHILDREN;
diff --git a/src/Builder/CommonBuilder.php b/src/Builder/CommonBuilder.php
index 9e38f08..71daf88 100644
--- a/src/Builder/CommonBuilder.php
+++ b/src/Builder/CommonBuilder.php
@@ -76,7 +76,11 @@ public function build(): Sanitizer
     protected function createBehavior(): Behavior
     {
         return (new Behavior())
-            ->withFlags(Behavior::ENCODE_INVALID_TAG | Behavior::REMOVE_UNEXPECTED_CHILDREN)
+            ->withFlags(
+                Behavior::ENCODE_INVALID_TAG
+                | Behavior::REMOVE_UNEXPECTED_CHILDREN
+                | Behavior::ENCODE_INVALID_PROCESSING_INSTRUCTION
+            )
             ->withName('common')
             ->withTags(...array_values($this->createBasicTags()))
             ->withTags(...array_values($this->createMediaTags()))
diff --git a/src/Visitor/CommonVisitor.php b/src/Visitor/CommonVisitor.php
index 64381e0..d92a042 100644
--- a/src/Visitor/CommonVisitor.php
+++ b/src/Visitor/CommonVisitor.php
@@ -19,6 +19,7 @@
 use DOMComment;
 use DOMElement;
 use DOMNode;
+use DOMProcessingInstruction;
 use DOMText;
 use Psr\Log\LoggerAwareInterface;
 use Psr\Log\LoggerAwareTrait;
@@ -64,6 +65,10 @@ public function beforeTraverse(Context $context): void
 
     public function enterNode(DOMNode $domNode): ?DOMNode
     {
+        if ($domNode instanceof DOMProcessingInstruction) {
+            return $this->handleInvalidNode($domNode);
+        }
+
         if (!$domNode instanceof DOMCdataSection
             && !$domNode instanceof DOMComment
             && !$domNode instanceof DOMElement
@@ -219,6 +224,7 @@ protected function handleInvalidNode(DOMNode $domNode): ?DOMNode
         if (
             ($domNode instanceof DOMComment && $this->behavior->shallEncodeInvalidComment())
             || ($domNode instanceof DOMCdataSection && $this->behavior->shallEncodeInvalidCdataSection())
+            || ($domNode instanceof DOMProcessingInstruction && $this->behavior->shallEncodeInvalidProcessingInstruction())
         ) {
             $this->log('Found unexpected node {nodeName}', [
                 'behavior' => $this->behavior->getName(),
diff --git a/tests/CommonBuilderTest.php b/tests/CommonBuilderTest.php
index a4ca310..2e2da3e 100644
--- a/tests/CommonBuilderTest.php
+++ b/tests/CommonBuilderTest.php
@@ -263,6 +263,14 @@ public function isSanitizedDataProvider(): array
                 '<!-- &lt;&quot;comment&quot;&gt; -->',
                 '<!-- &lt;&quot;comment&quot;&gt; -->',
             ],
+            '#912' => [
+                '<!---><p>',
+                '<!---&gt;&lt;p&gt;-->',
+            ],
+            '#913' => [
+                '<!---!><p>',
+                '<!---!&gt;&lt;p&gt;-->',
+            ],
             '#915' => [
                 '#text',
                 '#text',
@@ -303,6 +311,10 @@ public function isSanitizedDataProvider(): array
                 '<p class="{&quot;json&quot;:true}">value</p>',
                 '<p class="{&quot;json&quot;:true}">value</p>',
             ],
+            '#941' => [
+                '<?xml >s<img src=x onerror=alert(1)> ?>',
+                '&lt;?xml &gt;s&lt;img src=x onerror=alert(1)&gt; ?&gt;',
+            ],
         ];
     }