From 54a3712adaf941db7ad647db6588f12e6e58138a Mon Sep 17 00:00:00 2001
From: Thomas Bruederli <thomas@roundcube.net>
Date: Fri, 18 Aug 2017 09:50:39 +0200
Subject: [PATCH] Modify links in html messages during Washtml DOM traversal

This is a more safe approach than using regex and mitigates
possible vulnerabilities using malformed html markup.
---
 program/steps/mail/func.inc | 24 ++++++++++++-----------
 tests/MailFunc.php          | 38 +++++++++++++++++++++++++++++++++++--
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/program/steps/mail/func.inc b/program/steps/mail/func.inc
index 0e13c8453..79f911ead 100644
--- a/program/steps/mail/func.inc
+++ b/program/steps/mail/func.inc
@@ -860,6 +860,15 @@ function rcmail_wash_html($html, $p, $cid_replaces)
         $washer->add_callback('style', 'rcmail_washtml_callback');
     }
 
+    // modify HTML links to open a new window if clicked
+    if (!$p['skip_washer_link_callback']) {
+        $washer->add_callback('a', 'rcmail_washtml_link_callback');
+        $washer->add_callback('area', 'rcmail_washtml_link_callback');
+
+        if ($p['safe'])
+            $washer->add_callback('link', 'rcmail_washtml_link_callback');
+    }
+
     // Remove non-UTF8 characters (#1487813)
     $html = rcube_charset::clean($html);
 
@@ -1427,11 +1436,6 @@ function rcmail_html4inline($body, $container_id, $body_class='', &$attributes=n
         $last_style_pos = $pos2 + strlen($styles) - $len;
     }
 
-    // modify HTML links to open a new window if clicked
-    $GLOBALS['rcmail_html_container_id'] = $container_id;
-    $body = preg_replace_callback('/<(a|link|area)\s+([^>]+)>/Ui', 'rcmail_alter_html_link', $body);
-    unset($GLOBALS['rcmail_html_container_id']);
-
     $body = preg_replace(array(
             // add comments arround html and other tags
             '/(<!DOCTYPE[^>]*>)/i',
@@ -1510,13 +1514,11 @@ function rcmail_html4inline($body, $container_id, $body_class='', &$attributes=n
 /**
  * parse link (a, link, area) attributes and set correct target
  */
-function rcmail_alter_html_link($matches)
+function rcmail_washtml_link_callback($tag, $attribs, $content, $washtml)
 {
     global $RCMAIL;
 
-    $tag    = strtolower($matches[1]);
-    $attrib = html::parse_attrib_string($matches[2]);
-    $end    = '>';
+    $attrib = html::parse_attrib_string($attribs);
 
     // Remove non-printable characters in URL (#1487805)
     if ($attrib['href'])
@@ -1559,7 +1561,7 @@ function rcmail_alter_html_link($matches)
             $attrib['onclick'] = '';
         }
     }
-    else if (empty($attrib['href']) && !$attrib['name']) {
+    else if (empty($attrib['href']) && !isset($attrib['name'])) {
         $attrib['href']    = './#NOP';
         $attrib['onclick'] = 'return false';
     }
@@ -1576,7 +1578,7 @@ function rcmail_alter_html_link($matches)
     $allow = array('href','name','target','onclick','id','class','style','title',
         'rel','type','media','alt','coords','nohref','hreflang','shape');
 
-    return "<$tag" . html::attrib_string($attrib, $allow) . $end;
+    return html::tag($tag, $attrib, $content, $allow);
 }
 
 /**
diff --git a/tests/MailFunc.php b/tests/MailFunc.php
index f4fc3ed37..69b8a3517 100644
--- a/tests/MailFunc.php
+++ b/tests/MailFunc.php
@@ -22,12 +22,12 @@ class MailFunc extends PHPUnit_Framework_TestCase
     /**
      * Helper method to create a HTML message part object
      */
-    function get_html_part($body)
+    function get_html_part($body = null)
     {
         $part = new rcube_message_part;
         $part->ctype_primary = 'text';
         $part->ctype_secondary = 'html';
-        $part->body = file_get_contents(TESTS_DIR . $body);
+        $part->body = $body ? file_get_contents(TESTS_DIR . $body) : null;
         $part->replaces = array();
         return $part;
     }
@@ -184,6 +184,40 @@ class MailFunc extends PHPUnit_Framework_TestCase
         $this->assertRegExp('|src="http://other\.domain\.tld/img3\.gif"|', $html, "URI base resolving exception [2]");
     }
 
+    /**
+     * Test link attribute modifications
+     */
+    public function test_html_links()
+    {
+      // disable relative links
+      $html = '<a href="/">test</a>';
+      $body = rcmail_print_body($html, $this->get_html_part(), array('safe' => false, 'plain' => false));
+
+      $this->assertNotContains('href="/"', $body);
+      $this->assertContains('<a href="./#NOP"', $body);
+      $this->assertContains('onclick="return false"', $body);
+
+      $html = '<a href="https://roundcube.net">test</a>';
+      $body = rcmail_print_body($html, $this->get_html_part(), array('safe' => false, 'plain' => false));
+
+      // allow external links, add target and noreferrer
+      $this->assertContains('<a href="https://roundcube.net"', $body);
+      $this->assertContains(' target="_blank"', $body);
+      $this->assertContains(' rel="noreferrer"', $body);
+    }
+
+    /**
+     * Test potential XSS with invalid attributes
+     */
+    public function test_html_link_xss()
+    {
+      $html = '<a style="x:><img src=x onerror=alert(1)//">test</a>';
+      $body = rcmail_print_body($html, $this->get_html_part(), array('safe' => false, 'plain' => false));
+
+      $this->assertNotContains('onerror=alert(1)//">test', $body);
+      $this->assertContains('<a style="x: &gt;&lt;img src=x onerror=alert(1)//"', $body);
+    }
+
     /**
      * Test identities selection using Return-Path header
      */