From 54bcbdaba94a3573e128c6498816dbfa41d3a9cb Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 13 Dec 2019 10:31:44 -0800
Subject: [PATCH] Fix an XSS issue with certain high-priority remarkup rules
 embedded inside lower-priority link rules

Summary:
See <https://hackerone.com/reports/758002>. The link rules don't test that their parameters are flat text before using them in unsafe contexts.

Since almost all rules are lower-priority than these link rules, this behavior isn't obvious. However, two rules have broadly higher priority (monospaced text, and one variation of link rules has higher priority than the other), and the latter can be used to perform an XSS attack with input in the general form `()[ [[ ... | ... ]] ]` so that the inner link rule is evaluated first, then the outer link rule uses non-flat text in an unsafe way.

Test Plan:
Tested examples in HackerOne report. A simple example of broken (but not unsafe) behavior is:

```
[[ `x` | `y` ]]
```

Differential Revision: https://secure.phabricator.com/D20937
---
 .../markup/PhrictionRemarkupRule.php          | 22 ++++++++++++++-----
 .../PhutilRemarkupDocumentLinkRule.php        |  8 +++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/src/applications/phriction/markup/PhrictionRemarkupRule.php b/src/applications/phriction/markup/PhrictionRemarkupRule.php
index d17de331e5..11994e0aa5 100644
--- a/src/applications/phriction/markup/PhrictionRemarkupRule.php
+++ b/src/applications/phriction/markup/PhrictionRemarkupRule.php
@@ -16,8 +16,23 @@ final class PhrictionRemarkupRule extends PhutilRemarkupRule {
   }
 
   public function markupDocumentLink(array $matches) {
+    $name = trim(idx($matches, 2, ''));
+    if (empty($matches[2])) {
+      $name = null;
+    }
+
+    $path = trim($matches[1]);
+
+    if (!$this->isFlatText($name)) {
+      return $matches[0];
+    }
+
+    if (!$this->isFlatText($path)) {
+      return $matches[0];
+    }
+
     // If the link contains an anchor, separate that off first.
-    $parts = explode('#', trim($matches[1]), 2);
+    $parts = explode('#', $path, 2);
     if (count($parts) == 2) {
       $link = $parts[0];
       $anchor = $parts[1];
@@ -48,11 +63,6 @@ final class PhrictionRemarkupRule extends PhutilRemarkupRule {
       }
     }
 
-    $name = trim(idx($matches, 2, ''));
-    if (empty($matches[2])) {
-      $name = null;
-    }
-
     // Link is now used for slug detection, so append a slash if one
     // is needed.
     $link = rtrim($link, '/').'/';
diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php
index a6effa00ac..2170d9ae5e 100644
--- a/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php
+++ b/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php
@@ -136,6 +136,14 @@ final class PhutilRemarkupDocumentLinkRule extends PhutilRemarkupRule {
     $uri = trim($matches[1]);
     $name = trim(idx($matches, 2));
 
+    if (!$this->isFlatText($uri)) {
+      return $matches[0];
+    }
+
+    if (!$this->isFlatText($name)) {
+      return $matches[0];
+    }
+
     // If whatever is being linked to begins with "/" or "#", or has "://",
     // or is "mailto:" or "tel:", treat it as a URI instead of a wiki page.
     $is_uri = preg_match('@(^/)|(://)|(^#)|(^(?:mailto|tel):)@', $uri);