From a5d9c5ecc12c66a2aa98832c9c0abecc3dc2e7c2 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 20:24:13 +0100 Subject: [PATCH 01/12] Add ability to use HTML in group descriptions and posts --- package.json | 6 +- .../handlebars/components/editor.handlebars | 15 ++ src/main/handlebars/group.handlebars | 16 +-- src/main/handlebars/index.handlebars | 2 +- src/main/handlebars/layout.handlebars | 32 ++++- src/main/js/editor.js | 101 +++++++++++++ src/main/php/de/thekid/crews/Markup.php | 91 ++++++++++++ src/main/php/de/thekid/crews/web/Group.php | 11 +- src/main/php/de/thekid/crews/web/Index.php | 5 +- .../de/thekid/crews/unittest/MarkupTest.php | 134 ++++++++++++++++++ 10 files changed, 393 insertions(+), 20 deletions(-) create mode 100755 src/main/handlebars/components/editor.handlebars create mode 100755 src/main/js/editor.js create mode 100755 src/main/php/de/thekid/crews/Markup.php create mode 100755 src/test/php/de/thekid/crews/unittest/MarkupTest.php diff --git a/package.json b/package.json index a4ed5f1..e188ddf 100755 --- a/package.json +++ b/package.json @@ -3,6 +3,10 @@ "htmx.org": "^1.9" }, "bundles": { - "vendor": {"htmx.org": ["dist/htmx.min.js", "dist/ext/ws.js"], "fonts://display=swap": "Rubik:wght@300"} + "vendor": { + "htmx.org": ["dist/htmx.min.js", "dist/ext/ws.js"], + "fonts://display=swap": ["Rubik:wght@300", "JetBrains+Mono:wght@400"], + ".": ["src/main/js/editor.js"] + } } } \ No newline at end of file diff --git a/src/main/handlebars/components/editor.handlebars b/src/main/handlebars/components/editor.handlebars new file mode 100755 index 0000000..a2384b0 --- /dev/null +++ b/src/main/handlebars/components/editor.handlebars @@ -0,0 +1,15 @@ +
+ +
{{& value}}
+
+ + + + | + + + + +
+
+ \ No newline at end of file diff --git a/src/main/handlebars/group.handlebars b/src/main/handlebars/group.handlebars index 151521d..e9406fc 100755 --- a/src/main/handlebars/group.handlebars +++ b/src/main/handlebars/group.handlebars @@ -8,17 +8,17 @@ {{/if}} -

+

{{#with group}} - {{#*fragment "description"}}{{description}}{{/fragment}} + {{#*fragment "description"}}{{& description}}{{/fragment}} {{/with}} -

+
- + {{> components/editor id="post" field="body"}}
@@ -28,7 +28,7 @@ {{#each posts}} {{#*fragment "post"}}
-

{{body}}

+

{{& body}}

Posted by {{editor.name}} on {{date created}}{{#with updated}}, updated {{date .}}{{/with}} @@ -48,7 +48,7 @@ {{#*inline "edit"}}
- + {{> components/editor id=_id field="body" value=body}}
@@ -58,9 +58,7 @@ {{/inline}} {{#*inline "update"}} -
- -
+ {{> components/editor id=_id field="description" value=description}}
diff --git a/src/main/handlebars/index.handlebars b/src/main/handlebars/index.handlebars index 4bcb1f1..06e29a9 100755 --- a/src/main/handlebars/index.handlebars +++ b/src/main/handlebars/index.handlebars @@ -39,7 +39,7 @@
- + {{> components/editor id="group" field="description"}}
diff --git a/src/main/handlebars/layout.handlebars b/src/main/handlebars/layout.handlebars index c6af382..2ff47d5 100755 --- a/src/main/handlebars/layout.handlebars +++ b/src/main/handlebars/layout.handlebars @@ -11,6 +11,10 @@ font-family: 'Rubik', sans-serif; } + pre, code { + font-family: 'JetBrains Mono', monospace; + } + body { background-color: #ede6e1; margin: 0; @@ -34,7 +38,7 @@ form { display: flex; flex-direction: column; - width: max-content; + width: 100%; align-items: flex-start; gap: .5rem; } @@ -48,6 +52,25 @@ width: 100%; } + div.editor { + width: 100%; + } + + div[contenteditable] { + background-color: white; + padding: .25rem; + width: 100%; + outline: 1px solid black; + } + + div[contenteditable] p { + margin: 0; + } + + div[contenteditable] p + p { + margin-block-start: 1rem; + } + button[type='submit'] { padding: .25rem .5rem; } @@ -117,8 +140,8 @@ padding: .5rem; } - .post p { - white-space: pre-wrap; + .post p:empty { + display: none; } .post p.emoji { @@ -173,5 +196,8 @@ {{> content}} + {{#> scripts}} + + {{/scripts}} diff --git a/src/main/js/editor.js b/src/main/js/editor.js new file mode 100755 index 0000000..027211c --- /dev/null +++ b/src/main/js/editor.js @@ -0,0 +1,101 @@ +// Replacement for Sanitizer API, see https://web.dev/articles/sanitizer +// but also includes the possibility to rewrite tags to others. +class Markup { + EXPAND = true; + REMOVE = false; + #handle = { + 'H1' : {}, + 'H2' : {}, + 'H3' : {}, + 'B' : {}, + 'I' : {}, + 'U' : {}, + 'P' : {}, + 'CODE' : {}, + 'PRE' : {}, + 'STRIKE' : {}, + 'STRONG' : {emit: 'b'}, + 'EM' : {emit: 'i'}, + 'SECTION' : {emit: 'p'}, + 'ARTICLE' : {emit: 'p'}, + 'A' : {attributes: ['href']}, + 'DIV' : this.EXPAND, + 'SCRIPT' : this.REMOVE, + 'EMBED' : this.REMOVE, + 'OBJECT' : this.REMOVE, + 'IFRAME' : this.REMOVE, + '#comment' : this.REMOVE, + '#text' : (n) => n.wholeText, + }; + + html(text) { + return text.replace(/<>&'"/, m => `&#${m.charCodeAt(0)};`); + } + + transform(nodes) { + let transformed = ''; + for (const node of nodes) { + // console.log(node.nodeName, node); + + const handle = this.#handle[node.nodeName] ?? null; + if (null === handle) { + transformed += this.html(node.innerText); + } else if (this.REMOVE === handle) { + // NOOP + } else if (this.EXPAND === handle) { + transformed += this.transform(node.childNodes); + } else if ('function' === typeof handle) { + transformed += handle(node); + } else { + const tag = handle.emit ?? node.nodeName; + transformed += `<${tag}`; + for (const attribute of handle.attributes ?? []) { + if (node.hasAttribute(attribute)) { + transformed += ` ${attribute}="${this.html(node.getAttribute(attribute))}"`; + } + } + transformed += `>${this.transform(node.childNodes)}`; + } + } + return transformed; + } +} + +class Editor { + static markup = new Markup(); + + constructor($node) { + const $field = $node.querySelector('input[type="hidden"]'); + const $editable = $node.querySelector('div[contenteditable="true"]'); + + for (const $button of $node.querySelectorAll('button')) { + $button.addEventListener('click', e => { + e.preventDefault(); + if ($button.dataset.command) { + document.execCommand($button.dataset.command, false, $button.name); + } else { + document.execCommand($button.name, false, null); + } + }); + } + + // Synchronize hidden field + $editable.addEventListener('input', e => { + $field.value = $editable.innerHTML; + }) + + // Transform HTML, inserting text as-is. Todo: images, see + // https://web.dev/patterns/clipboard/paste-images/ + $editable.addEventListener('paste', e => { + console.log(e); + e.preventDefault(); + if (-1 !== e.clipboardData.types.indexOf('text/html')) { + const doc = document.implementation.createHTMLDocument('Pasted text'); + doc.write(e.clipboardData.getData('text/html')); + document.execCommand('insertHTML', false, Editor.markup.transform(doc.body.childNodes)); + } else { + document.execCommand('insertText', false, e.clipboardData.getData('text/plain')); + } + }); + } +} diff --git a/src/main/php/de/thekid/crews/Markup.php b/src/main/php/de/thekid/crews/Markup.php new file mode 100755 index 0000000..346691e --- /dev/null +++ b/src/main/php/de/thekid/crews/Markup.php @@ -0,0 +1,91 @@ + [], + 'h2' => [], + 'h3' => [], + 'p' => [], + 'b' => [], + 'i' => [], + 'u' => [], + 's' => [], + 'pre' => [], + 'code' => [], + 'strike' => [], + 'hr' => [], + 'br' => [], + 'a' => ['attributes' => ['href']], + 'img' => ['attributes' => ['src']], + 'strong' => ['emit' => 'b'], + 'em' => ['emit' => 'i'], + 'div' => self::EXPAND, + '#comment' => self::REMOVE, + 'script' => self::REMOVE, + 'embed' => self::REMOVE, + 'object' => self::REMOVE, + 'iframe' => self::REMOVE, + ]; + + public function __construct(array $handle= []) { + $this->handle+= $handle; + } + + private function process($nodes) { + $text= ''; + foreach ($nodes as $node) { + $handle= $this->handle[$node->nodeName] ?? null; + if (null === $handle) { + $text.= htmlspecialchars($node->textContent); + } else if (self::REMOVE === $handle) { + // NOOP + } else if (self::EXPAND === $handle) { + $text.= $this->process($node->childNodes); + } else { + $tag= $handle['emit'] ?? $node->nodeName; + $text.= "<{$tag}"; + foreach ($handle['attributes'] ?? [] as $attribute) { + if ($node->hasAttribute($attribute)) { + $text.= ' '.$attribute.'="'.htmlspecialchars($node->getAttribute($attribute)).'"'; + } + } + $text.= ">{$this->process($node->childNodes)}"; + } + } + return $text; + } + + public function transform(string $input): string { + if (strlen($input) === strspn($input, "\r\n\t ")) return $input; + + libxml_clear_errors(); + $useInternal= libxml_use_internal_errors(true); + $entityLoader= libxml_get_external_entity_loader(); + libxml_set_external_entity_loader(fn() => null); + + try { + $doc= new DOMDocument('1.0', 'utf-8'); + $doc->loadHTML( + "{$input}", + LIBXML_NONET + ); + } finally { + libxml_use_internal_errors($useInternal); + libxml_set_external_entity_loader($entityLoader); + } + + return $this->process($doc->getElementsByTagName('body')->item(0)->childNodes); + } +} \ No newline at end of file diff --git a/src/main/php/de/thekid/crews/web/Group.php b/src/main/php/de/thekid/crews/web/Group.php index 89fab38..38a730a 100755 --- a/src/main/php/de/thekid/crews/web/Group.php +++ b/src/main/php/de/thekid/crews/web/Group.php @@ -1,13 +1,14 @@ groups= $db->collection('groups'); @@ -36,7 +37,9 @@ public function group(ObjectId $group, string $view) { #[Put] public function describe(#[Value] User $user, ObjectId $group, #[Param] string $description) { - $result= $this->groups->modify($user->where($group, 'owner'), ['$set' => ['description' => $description]]); + $result= $this->groups->modify($user->where($group, 'owner'), ['$set' => [ + 'description' => $this->markup->transform($description), + ]]); return View::named('group#description')->with($result->document()->properties()); } @@ -44,7 +47,7 @@ public function describe(#[Value] User $user, ObjectId $group, #[Param] string $ public function create(#[Value] User $user, ObjectId $group, #[Param] string $body) { $insert= $this->posts->insert(new Document([ 'group' => $group, - 'body' => $body, + 'body' => $this->markup->transform($body), 'editor' => $user->reference(), 'created' => Date::now(), ])); @@ -71,7 +74,7 @@ public function delete(#[Value] User $user, ObjectId $group, ObjectId $id) { #[Put('/posts/{id}')] public function update(#[Value] User $user, ObjectId $group, ObjectId $id, #[Param] string $body) { $this->posts->update($user->where($id, 'editor'), ['$set' => [ - 'body' => $body, + 'body' => $this->markup->transform($body), 'updated' => Date::now(), ]]); $this->events->publish($user, $group, ['update' => $id]); diff --git a/src/main/php/de/thekid/crews/web/Index.php b/src/main/php/de/thekid/crews/web/Index.php index 7d59c9c..1fe99ed 100755 --- a/src/main/php/de/thekid/crews/web/Index.php +++ b/src/main/php/de/thekid/crews/web/Index.php @@ -1,13 +1,14 @@ groups= $db->collection('groups'); @@ -42,7 +43,7 @@ public function create(#[Value] User $user, #[Param] $name, #[Param] $descriptio // Create, then trigger redirect $insert= $this->groups->insert(new Document([ 'name' => $name, - 'description' => $description, + 'description' => $this->markup->transform($description), 'owner' => $user->reference(), 'created' => Date::now(), ])); diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php new file mode 100755 index 0000000..1c4a3c7 --- /dev/null +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -0,0 +1,134 @@ +transform('')); + } + + #[Test] + public function whitespace_only() { + $fixture= new Markup(); + Assert::equals("\r\n\t ", $fixture->transform("\r\n\t ")); + } + + #[Test] + public function text() { + $fixture= new Markup(); + Assert::equals('This is a test', $fixture->transform('This is a test')); + } + + #[Test] + public function markup() { + $fixture= new Markup(); + Assert::equals('This is a test', $fixture->transform('This is a test')); + } + + #[Test, Values([ + ['test', 'test'], + ['test', 'test'], + ])] + public function rewrites($input, $expected) { + $fixture= new Markup(); + Assert::equals($expected, $fixture->transform($input)); + } + + #[Test] + public function text_escaping() { + $fixture= new Markup(); + Assert::equals('1 < 2', $fixture->transform('1 < 2')); + } + + #[Test, Values([ + ['Test <&">€&unknown; Works'], + ['Test <&">€&unknown; Works'], + ])] + public function html_entities_are_escaped($input) { + $fixture= new Markup(); + Assert::equals('Test <&">€&unknown; Works', $fixture->transform($input)); + } + + #[Test, Values([ + ['Ãœbercoder'], + ['👉🙂'], + ['中国'], // "China" + ])] + public function loads_utf8($input) { + $fixture= new Markup(); + Assert::equals($input, $fixture->transform($input)); + } + + #[Test, Values([ + [''], + [''], + [''], + [''], + ])] + public function removes_script_tags($input) { + $fixture= new Markup(); + Assert::equals('', $fixture->transform($input)); + } + + #[Test] + public function removes_unsupported_attributes() { + $fixture= new Markup(); + Assert::equals( + 'test', + $fixture->transform('test') + ); + } + + #[Test] + public function removes_divs() { + $fixture= new Markup(); + Assert::equals( + 'The container is gone', + $fixture->transform('
The container is gone
') + ); + } + + #[Test] + public function removes_comments() { + $fixture= new Markup(); + Assert::equals( + 'The is gone', + $fixture->transform('The is gone') + ); + } + + #[Test, Values([ + [''], + [''] + ])] + public function does_not_parse_doctype_with($entity) { + $fixture= new Markup(); + Assert::equals( + "\n]>\n

&xxe;

", + $fixture->transform("\n

&xxe;

") + ); + } + + #[Test] + public function svg_mutation_xss() { + $fixture= new Markup(); + $exploit= '

'; + Assert::equals('<img src onerror=alert(1)>', $fixture->transform($exploit)); + } +} \ No newline at end of file From 4391aac22269b50d0e004467557c30eda998dbb4 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 20:35:01 +0100 Subject: [PATCH 02/12] Check for libxml_get_external_entity_loader() This function is only available in PHP 8.2+ --- src/main/php/de/thekid/crews/Markup.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/php/de/thekid/crews/Markup.php b/src/main/php/de/thekid/crews/Markup.php index 346691e..68164a0 100755 --- a/src/main/php/de/thekid/crews/Markup.php +++ b/src/main/php/de/thekid/crews/Markup.php @@ -72,7 +72,7 @@ public function transform(string $input): string { libxml_clear_errors(); $useInternal= libxml_use_internal_errors(true); - $entityLoader= libxml_get_external_entity_loader(); + $entityLoader= PHP_VERSION_ID >= 80200 ? libxml_get_external_entity_loader() : null; libxml_set_external_entity_loader(fn() => null); try { @@ -83,7 +83,7 @@ public function transform(string $input): string { ); } finally { libxml_use_internal_errors($useInternal); - libxml_set_external_entity_loader($entityLoader); + $entityLoader && libxml_set_external_entity_loader($entityLoader); } return $this->process($doc->getElementsByTagName('body')->item(0)->childNodes); From b56380f1780abc4052c8f71c5f8b873914ccd8e3 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 20:36:22 +0100 Subject: [PATCH 03/12] Fix implementation vagaries in DomXML --- src/test/php/de/thekid/crews/unittest/MarkupTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php index 1c4a3c7..454b396 100755 --- a/src/test/php/de/thekid/crews/unittest/MarkupTest.php +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -113,8 +113,8 @@ public function removes_comments() { public function does_not_parse_doctype_with($entity) { $fixture= new Markup(); Assert::equals( - "\n]>\n

&xxe;

", - $fixture->transform("\n

&xxe;

") + "]>\n

&xxe;

", + ltrim($fixture->transform("\n

&xxe;

")) ); } From f501c5e07140c08b96773475513931e17eff524f Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 20:49:34 +0100 Subject: [PATCH 04/12] Rewrite test for raw entities input --- .../de/thekid/crews/unittest/MarkupTest.php | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php index 454b396..b96f11a 100755 --- a/src/test/php/de/thekid/crews/unittest/MarkupTest.php +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -43,19 +43,27 @@ public function rewrites($input, $expected) { Assert::equals($expected, $fixture->transform($input)); } - #[Test] - public function text_escaping() { - $fixture= new Markup(); - Assert::equals('1 < 2', $fixture->transform('1 < 2')); - } - #[Test, Values([ - ['Test <&">€&unknown; Works'], + ['1 < 2'], + ['1 & 2'], ['Test <&">€&unknown; Works'], ])] - public function html_entities_are_escaped($input) { + public function does_not_contain_unescaped_entities($input) { $fixture= new Markup(); - Assert::equals('Test <&">€&unknown; Works', $fixture->transform($input)); + $transformed= $fixture->transform($input); + + foreach (['//', '/"/', '/&(?![a-z]+;)/'] as $pattern) { + Assert::equals(0, preg_match($pattern, $fixture->transform($input))); + } + } + + #[Test] + public function html_entities_are_escaped() { + $fixture= new Markup(); + Assert::equals( + 'Test <&">€&unknown; Works', + $fixture->transform('Test <&">€&unknown; Works') + ); } #[Test, Values([ From 5c56e90e305c621fb1259e4ce307f08863f28da0 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 21:06:12 +0100 Subject: [PATCH 05/12] Rename components -> traits See https://github.com/thekid/crews/pull/4#discussion_r1442169705 --- src/main/handlebars/group.handlebars | 6 +++--- src/main/handlebars/index.handlebars | 2 +- .../handlebars/{components => traits}/editor.handlebars | 0 3 files changed, 4 insertions(+), 4 deletions(-) rename src/main/handlebars/{components => traits}/editor.handlebars (100%) diff --git a/src/main/handlebars/group.handlebars b/src/main/handlebars/group.handlebars index e9406fc..a7620f5 100755 --- a/src/main/handlebars/group.handlebars +++ b/src/main/handlebars/group.handlebars @@ -18,7 +18,7 @@
- {{> components/editor id="post" field="body"}} + {{> traits/editor id="post" field="body"}}
@@ -48,7 +48,7 @@ {{#*inline "edit"}}
- {{> components/editor id=_id field="body" value=body}} + {{> traits/editor id=_id field="body" value=body}}
@@ -58,7 +58,7 @@ {{/inline}} {{#*inline "update"}} - {{> components/editor id=_id field="description" value=description}} + {{> traits/editor id=_id field="description" value=description}}
diff --git a/src/main/handlebars/index.handlebars b/src/main/handlebars/index.handlebars index 06e29a9..72d6099 100755 --- a/src/main/handlebars/index.handlebars +++ b/src/main/handlebars/index.handlebars @@ -39,7 +39,7 @@
- {{> components/editor id="group" field="description"}} + {{> traits/editor id="group" field="description"}}
diff --git a/src/main/handlebars/components/editor.handlebars b/src/main/handlebars/traits/editor.handlebars similarity index 100% rename from src/main/handlebars/components/editor.handlebars rename to src/main/handlebars/traits/editor.handlebars From 0ed6fe35ffc3da3cf7ff30fd899f351eac805b56 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 21:27:34 +0100 Subject: [PATCH 06/12] If the enclosing form is being reset, reset the editable --- src/main/js/editor.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/main/js/editor.js b/src/main/js/editor.js index 027211c..aeea7cc 100755 --- a/src/main/js/editor.js +++ b/src/main/js/editor.js @@ -82,7 +82,12 @@ class Editor { // Synchronize hidden field $editable.addEventListener('input', e => { $field.value = $editable.innerHTML; - }) + }); + + // If the enclosing form is being reset, reset the editable + $editable.closest('form').addEventListener('reset', e => { + $editable.innerText= ''; + }); // Transform HTML, inserting text as-is. Todo: images, see // https://web.dev/patterns/clipboard/paste-images/ From f0db43551fae74f954012c2f4f21d7042627906d Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 21:34:22 +0100 Subject: [PATCH 07/12] Fix HTML replacement --- src/main/js/editor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/js/editor.js b/src/main/js/editor.js index aeea7cc..326358c 100755 --- a/src/main/js/editor.js +++ b/src/main/js/editor.js @@ -29,7 +29,7 @@ class Markup { }; html(text) { - return text.replace(/<>&'"/, m => `&#${m.charCodeAt(0)};`); + return text.replace(/[<>&'"]/g, m => `&#${m.charCodeAt(0)};`); } transform(nodes) { From cf6d32b67366337d9bf294f0a9465c7a2c9ba934 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 21:46:45 +0100 Subject: [PATCH 08/12] Simplify test code, add local XXE test --- src/main/php/de/thekid/crews/Markup.php | 1 + .../de/thekid/crews/unittest/MarkupTest.php | 50 +++++++------------ 2 files changed, 19 insertions(+), 32 deletions(-) diff --git a/src/main/php/de/thekid/crews/Markup.php b/src/main/php/de/thekid/crews/Markup.php index 68164a0..d73c68f 100755 --- a/src/main/php/de/thekid/crews/Markup.php +++ b/src/main/php/de/thekid/crews/Markup.php @@ -8,6 +8,7 @@ * @test de.thekid.crews.unittest.MarkupTest * @see https://research.securitum.com/dompurify-bypass-using-mxss/ * @see https://research.securitum.com/mutation-xss-via-mathml-mutation-dompurify-2-0-17-bypass/ + * @see https://knowledge-base.secureflag.com/vulnerabilities/xml_injection/xml_entity_expansion_php.html */ class Markup { const REMOVE= false; diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php index b96f11a..a538d8d 100755 --- a/src/test/php/de/thekid/crews/unittest/MarkupTest.php +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -12,26 +12,22 @@ public function can_create() { #[Test] public function empty_text() { - $fixture= new Markup(); - Assert::equals('', $fixture->transform('')); + Assert::equals('', new Markup()->transform('')); } #[Test] public function whitespace_only() { - $fixture= new Markup(); - Assert::equals("\r\n\t ", $fixture->transform("\r\n\t ")); + Assert::equals("\r\n\t ", new Markup()->transform("\r\n\t ")); } #[Test] public function text() { - $fixture= new Markup(); - Assert::equals('This is a test', $fixture->transform('This is a test')); + Assert::equals('This is a test', new Markup()->transform('This is a test')); } #[Test] public function markup() { - $fixture= new Markup(); - Assert::equals('This is a test', $fixture->transform('This is a test')); + Assert::equals('This is a test', new Markup()->transform('This is a test')); } #[Test, Values([ @@ -39,8 +35,7 @@ public function markup() { ['test', 'test'], ])] public function rewrites($input, $expected) { - $fixture= new Markup(); - Assert::equals($expected, $fixture->transform($input)); + Assert::equals($expected, new Markup()->transform($input)); } #[Test, Values([ @@ -49,20 +44,18 @@ public function rewrites($input, $expected) { ['Test <&">€&unknown; Works'], ])] public function does_not_contain_unescaped_entities($input) { - $fixture= new Markup(); - $transformed= $fixture->transform($input); + $transformed= new Markup()->transform($input); foreach (['//', '/"/', '/&(?![a-z]+;)/'] as $pattern) { - Assert::equals(0, preg_match($pattern, $fixture->transform($input))); + Assert::equals(0, preg_match($pattern, new Markup()->transform($input))); } } #[Test] public function html_entities_are_escaped() { - $fixture= new Markup(); Assert::equals( 'Test <&">€&unknown; Works', - $fixture->transform('Test <&">€&unknown; Works') + new Markup()->transform('Test <&">€&unknown; Works') ); } @@ -72,8 +65,7 @@ public function html_entities_are_escaped() { ['中国'], // "China" ])] public function loads_utf8($input) { - $fixture= new Markup(); - Assert::equals($input, $fixture->transform($input)); + Assert::equals($input, new Markup()->transform($input)); } #[Test, Values([ @@ -83,60 +75,54 @@ public function loads_utf8($input) { [''], ])] public function removes_script_tags($input) { - $fixture= new Markup(); - Assert::equals('', $fixture->transform($input)); + Assert::equals('', new Markup()->transform($input)); } #[Test] public function removes_unsupported_attributes() { - $fixture= new Markup(); Assert::equals( 'test', - $fixture->transform('test') + new Markup()->transform('test') ); } #[Test] public function removes_divs() { - $fixture= new Markup(); Assert::equals( 'The container is gone', - $fixture->transform('
The container is gone
') + new Markup()->transform('
The container is gone
') ); } #[Test] public function removes_comments() { - $fixture= new Markup(); Assert::equals( 'The is gone', - $fixture->transform('The is gone') + new Markup()->transform('The is gone') ); } #[Test, Values([ [''], - [''] + [''], + [''], ])] public function does_not_parse_doctype_with($entity) { - $fixture= new Markup(); Assert::equals( "]>\n

&xxe;

", - ltrim($fixture->transform("\n

&xxe;

")) + new Markup()->transform("\n

&xxe;

") ); } #[Test] public function svg_mutation_xss() { - $fixture= new Markup(); $exploit= '

'; - Assert::equals('<img src onerror=alert(1)>', $fixture->transform($exploit)); + Assert::equals('<img src onerror=alert(1)>', new Markup()->transform($exploit)); } } \ No newline at end of file From 0ecb98dbfc0960339984b7023f84b34522baf68c Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 22:02:23 +0100 Subject: [PATCH 09/12] Verify CVE-2023-3823 --- src/main/php/de/thekid/crews/Markup.php | 1 + src/test/php/de/thekid/crews/unittest/MarkupTest.php | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/main/php/de/thekid/crews/Markup.php b/src/main/php/de/thekid/crews/Markup.php index d73c68f..4f9f375 100755 --- a/src/main/php/de/thekid/crews/Markup.php +++ b/src/main/php/de/thekid/crews/Markup.php @@ -6,6 +6,7 @@ * Transforms HTML markup to a subset we can render in the pages * * @test de.thekid.crews.unittest.MarkupTest + * @see https://github.com/php/php-src/security/advisories/GHSA-3qrf-m4j2-pcrr * @see https://research.securitum.com/dompurify-bypass-using-mxss/ * @see https://research.securitum.com/mutation-xss-via-mathml-mutation-dompurify-2-0-17-bypass/ * @see https://knowledge-base.secureflag.com/vulnerabilities/xml_injection/xml_entity_expansion_php.html diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php index a538d8d..51467b6 100755 --- a/src/test/php/de/thekid/crews/unittest/MarkupTest.php +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -110,7 +110,16 @@ public function removes_comments() { public function does_not_parse_doctype_with($entity) { Assert::equals( "]>\n

&xxe;

", - new Markup()->transform("\n

&xxe;

") + new Markup()->transform("\n

&xxe;

"), + ); + } + + #[Test] + public function CVE_2023_3823() { + $entity= ' %remote; %intern; n%trick;'; + Assert::equals( + ' %remote; %intern; n%trick;]>', + new Markup()->transform(""), ); } From 0f2ccca200612f4c81cf6bd48a23bdad024df189 Mon Sep 17 00:00:00 2001 From: Timm Friebe Date: Thu, 4 Jan 2024 22:29:21 +0100 Subject: [PATCH 10/12] Use PHP 8.4 HTML5 parser --- src/main/php/de/thekid/crews/Markup.php | 15 ++++++++---- .../de/thekid/crews/unittest/MarkupTest.php | 24 +++++++++++++++---- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/main/php/de/thekid/crews/Markup.php b/src/main/php/de/thekid/crews/Markup.php index 4f9f375..3ac1993 100755 --- a/src/main/php/de/thekid/crews/Markup.php +++ b/src/main/php/de/thekid/crews/Markup.php @@ -1,11 +1,13 @@ = 80200 ? libxml_get_external_entity_loader() : null; libxml_set_external_entity_loader(fn() => null); + // Use https://wiki.php.net/rfc/domdocument_html5_parser for PHP 8.4+ + $html= "{$input}"; try { - $doc= new DOMDocument('1.0', 'utf-8'); - $doc->loadHTML( - "{$input}", - LIBXML_NONET - ); + if (PHP_VERSION_ID >= 80400) { + $doc= HTMLDocument::createFromString($html); + } else { + $doc= new DOMDocument('1.0', 'utf-8'); + $doc->loadHTML($html, LIBXML_NONET); + } } finally { libxml_use_internal_errors($useInternal); $entityLoader && libxml_set_external_entity_loader($entityLoader); diff --git a/src/test/php/de/thekid/crews/unittest/MarkupTest.php b/src/test/php/de/thekid/crews/unittest/MarkupTest.php index 51467b6..9412500 100755 --- a/src/test/php/de/thekid/crews/unittest/MarkupTest.php +++ b/src/test/php/de/thekid/crews/unittest/MarkupTest.php @@ -1,6 +1,7 @@

">'; Assert::equals('<a id="">', new Markup()->transform($exploit)); } - #[Test] - public function namespace_confusion_xss() { + #[Test, Runtime(php: '>=8.4-dev')] + public function svg_mutation_xss_html5() { + $exploit= '

'; Assert::equals('<img src onerror=alert(1)>', new Markup()->transform($exploit)); } + + #[Test, Runtime(php: '>=8.4-dev')] + public function namespace_confusion_xss_html5() { + $exploit= '