Page MenuHomePhabricator

HTML in Diffusion not escaped in certain circumstances
Closed, ResolvedPublic

Description

HTML in Diffusion source code listing is not escaped

https://phabricator.wikimedia.org/diffusion/MW/browse/master/tests/parser/parserTests.txt

  • have the syntax hilight turned on
  • the file is bigger than 256kB, thus syntax hilight is claimed in header to be turned off automatically, however, plaintext file as with regular (manual) syntax highlight off doesn't display, but the content is being parsed (WARNING: bunch of javascript alerts there, worth to kill page loading soon)

The latter condition may not be mandatory, I didn't find any suitable file to test.

Event Timeline

epriestley triaged this task as Unbreak Now! priority.
epriestley added a project: Security.

In the future, please report security issues via HackerOne:

https://hackerone.com/phabricator

If you'd like to file a duplicate report there, I'd be happy to award you a security bounty. This is one of the more severe security problems ever found in Phabricator.

This should be fixed in HEAD of master, and I've cherry-picked the patch to stable.

This attack is stored XSS, although it requires write access to a repository to execute. There is no way to mitigate it in configuration.

This issue was introduced in rP91a01e57 (January 6). All installs running a newer version of Phabricator are advised to upgrade.

If you can not upgrade immediately, I believe this local patch should apply cleanly to any affected version:

diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php
index 30c2b82..3ae076a 100644
--- a/src/applications/diffusion/controller/DiffusionBrowseController.php
+++ b/src/applications/diffusion/controller/DiffusionBrowseController.php
@@ -682,17 +682,21 @@ final class DiffusionBrowseController extends DiffusionController {
         $blame_commits,
         $show_blame);
     } else {
-      if ($can_highlight) {
-        require_celerity_resource('syntax-highlighting-css');
+      require_celerity_resource('syntax-highlighting-css');
 
+      if ($can_highlight) {
         $highlighted = PhabricatorSyntaxHighlighter::highlightWithFilename(
           $path,
           $file_corpus);
-        $lines = phutil_split_lines($highlighted);
       } else {
-        $lines = phutil_split_lines($file_corpus);
+        // Highlight as plain text to escape the content properly.
+        $highlighted = PhabricatorSyntaxHighlighter::highlightWithLanguage(
+          'txt',
+          $file_corpus);
       }
 
+      $lines = phutil_split_lines($highlighted);
+
       $rows = $this->buildDisplayRows(
         $lines,
         $blame_list,

The Phacility cluster has been patched.

Issues like this are rare, but they emphasize our lack of a good security notification channel. I think T7413 (have Phabricator periodically ask the upstream if there are outstanding, unpatched security issues affecting the install) is the most promising way forward there, although it probably makes sense to do that as part of T5055 (i.e., generically for all installed packages).

Also worth noting is that Content Security Policy (T4340) would have prevented this. CSP is obviously worth implementing, but I believe the last attack it would have prevented was D4534 in January 2013, so it's hard to prioritize if it only defuses one attack every 3-4 years.

We also had one self-XSS using "Editor" configuration in March 2014 (D8551) but I think CSP would not have prevented it because the URI mishandling was in Javascript, if I remember correctly.

For reference, @Danny_B filed a copy of this on HackerOne here so I could award a bounty for it:

https://hackerone.com/reports/148865