CVE-2012-4668: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: Cross-site scripting (XSS) vulnerability in Roundcube Webmail 0.8.1 and earlier allows remote attackers to inject arbitrary web script or HTML via the signature in an email.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
Published: 2012-08-25T10:29Z
Repository: https://github.com/roundcube/roundcubemail
Language: Other
Files changed: CHANGELOG, program/js/app.js, program/steps/mail/compose.inc
Commit date: 2012-08-15 11:20:40 +0200
Commit message: Fix XSS issue where plain signatures wasn't secured in HTML mode (#1488613)

Diff: diff --git a/CHANGELOG b/CHANGELOG
index 932680645..37170fc65 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,9 +1,10 @@
 CHANGELOG Roundcube Webmail
 ===========================
 
-- Fix XSS issue with href="javascript:" not being removed (#1488613)
+- Fix XSS issue where plain signatures wasn't secured in HTML mode (#1488613)
+- Fix XSS issue where href="javascript:" wasn't secured (#1488613)
 - Fix impossible to create message with empty plain text part (#1488610)
 - Fix stripped apostrophes when replying in plain text to HTML message (#1488606)
 - Fix inactive Save search option after advanced search (#1488607)
 - Fix Remove from group option is active for contact search result (#1488608)
 - Disable autocapitalization in login form on iPad/iPhone (#1488609)
diff --git a/program/js/app.js b/program/js/app.js
index de61b21f1..e8bb6c1a7 100644
--- a/program/js/app.js
+++ b/program/js/app.js
@@ -3296,12 +3296,11 @@ function rcube_webmail()
     var cursor_pos, p = -1,
       id = obj.options[obj.selectedIndex].value,
       input_message = $("[name='_message']"),
       message = input_message.val(),
       is_html = ($("input[name='_is_html']").val() == '1'),
-      sig = this.env.identity,
-      sig_separator = this.env.sig_above && (this.env.compose_mode == 'reply' || this.env.compose_mode == 'forward') ? '---' : '-- ';
+      sig = this.env.identity;
 
     // enable manual signature insert
     if (this.env.signatures && this.env.signatures[id]) {
       this.enable_command('insert-sig', true);
       this.env.compose_commands.push('insert-sig');
@@ -3310,29 +3309,22 @@ function rcube_webmail()
       this.enable_command('insert-sig', false);
 
     if (!is_html) {
       // remove the 'old' signature
       if (show_sig && sig && this.env.signatures && this.env.signatures[sig]) {
-
-        sig = this.env.signatures[sig].is_html ? this.env.signatures[sig].plain_text : this.env.signatures[sig].text;
+        sig = this.env.signatures[sig].text;
         sig = sig.replace(/\r\n/g, '\n');
 
-        if (!sig.match(/^--[ -]\n/m))
-          sig = sig_separator + '\n' + sig;
-
         p = this.env.sig_above ? message.indexOf(sig) : message.lastIndexOf(sig);
         if (p >= 0)
           message = message.substring(0, p) + message.substring(p+sig.length, message.length);
       }
       // add the new signature string
       if (show_sig && this.env.signatures && this.env.signatures[id]) {
-        sig = this.env.signatures[id]['is_html'] ? this.env.signatures[id]['plain_text'] : this.env.signatures[id]['text'];
+        sig = this.env.signatures[id].text;
         sig = sig.replace(/\r\n/g, '\n');
 
-        if (!sig.match(/^--[ -]\n/m))
-          sig = sig_separator + '\n' + sig;
-
         if (this.env.sig_above) {
           if (p >= 0) { // in place of removed signature
             message = message.substring(0, p) + sig + message.substring(p, message.length);
             cursor_pos = p - 1;
           }
@@ -3392,25 +3384,12 @@ function rcube_webmail()
 
           body.appendChild(sigElem);
         }
       }
 
-      if (this.env.signatures[id]) {
-        if (this.env.signatures[id].is_html) {
-          sig = this.env.signatures[id].text;
-          if (!this.env.signatures[id].plain_text.match(/^--[ -]\r?\n/m))
-            sig = sig_separator + '<br />' + sig;
-        }
-        else {
-          sig = this.env.signatures[id].text;
-          if (!sig.match(/^--[ -]\r?\n/m))
-            sig = sig_separator + '\n' + sig;
-          sig = '<pre>' + sig + '</pre>';
-        }
-
-        sigElem.innerHTML = sig;
-      }
+      if (this.env.signatures[id])
+        sigElem.innerHTML = this.env.signatures[id].html;
     }
 
     this.env.identity = id;
     return true;
   };
diff --git a/program/steps/mail/compose.inc b/program/steps/mail/compose.inc
index 71a1c0f21..1a1d244e1 100644
--- a/program/steps/mail/compose.inc
+++ b/program/steps/mail/compose.inc
@@ -527,21 +527,23 @@ function rcmail_compose_headers($attrib)
 }
 
 
 function rcmail_compose_header_from($attrib)
 {
-  global $MESSAGE, $OUTPUT;
+  global $MESSAGE, $OUTPUT, $RCMAIL, $compose_mode;
 
   // pass the following attributes to the form class
   $field_attrib = array('name' => '_from');
   foreach ($attrib as $attr => $value)
     if (in_array($attr, array('id', 'class', 'style', 'size', 'tabindex')))
       $field_attrib[$attr] = $value;
 
   if (count($MESSAGE->identities))
   {
     $a_signatures = array();
+    $separator    = $RCMAIL->config->get('sig_above')
+      && ($compose_mode == RCUBE_COMPOSE_REPLY || $compose_mode == RCUBE_COMPOSE_FORWARD) ? '---' : '-- ';
 
     $field_attrib['onchange'] = JS_OBJECT_NAME.".change_identity(this)";
     $select_from = new html_select($field_attrib);
 
     // create SELECT element
@@ -551,17 +553,31 @@ function rcmail_compose_header_from($attrib)
       $select_from->add(format_email_recipient($sql_arr['email'], $sql_arr['name']), $identity_id);
 
       // add signature to array
       if (!empty($sql_arr['signature']) && empty($COMPOSE['param']['nosig']))
       {
-        $a_signatures[$identity_id]['text'] = $sql_arr['signature'];
-        $a_signatures[$identity_id]['is_html'] = ($sql_arr['html_signature'] == 1) ? true : false;
-        if ($a_signatures[$identity_id]['is_html'])
-        {
-            $h2t = new html2text($a_signatures[$identity_id]['text'], false, false);
-            $a_signatures[$identity_id]['plain_text'] = trim($h2t->get_text());
+        $text = $html = $sql_arr['signature'];
+
+        if ($sql_arr['html_signature']) {
+            $h2t  = new html2text($sql_arr['signature'], false, false);
+            $text = trim($h2t->get_text());
+        }
+        else {
+            $html = htmlentities($html, ENT_NOQUOTES, RCMAIL_CHARSET);
+        }
+
+        if (!preg_match('/^--[ -]\r?\n/m', $text)) {
+            $text = $separator . "\n" . $text;
+            $html = $separator . "<br>" . $html;
+        }
+
+        if (!$sql_arr['html_signature']) {
+            $html = "<pre>" . $html . "</pre>";
         }
+
+        $a_signatures[$identity_id]['text'] = $text;
+        $a_signatures[$identity_id]['html'] = $html;
       }
     }
 
     $out = $select_from->show($MESSAGE->compose['from']);
 

Vulnerable code: - Fix XSS issue with href="javascript:" not being removed (#1488613)
      sig = this.env.identity,
      sig_separator = this.env.sig_above && (this.env.compose_mode == 'reply' || this.env.compose_mode == 'forward') ? '---' : '-- ';

        sig = this.env.signatures[sig].is_html ? this.env.signatures[sig].plain_text : this.env.signatures[sig].text;
        if (!sig.match(/^--[ -]\n/m))
          sig = sig_separator + '\n' + sig;

        sig = this.env.signatures[id]['is_html'] ? this.env.signatures[id]['plain_text'] : this.env.signatures[id]['text'];
        if (!sig.match(/^--[ -]\n/m))
          sig = sig_separator + '\n' + sig;

      if (this.env.signatures[id]) {
        if (this.env.signatures[id].is_html) {
          sig = this.env.signatures[id].text;
          if (!this.env.signatures[id].plain_text.match(/^--[ -]\r?\n/m))
            sig = sig_separator + '<br />' + sig;
        }
        else {
          sig = this.env.signatures[id].text;
          if (!sig.match(/^--[ -]\r?\n/m))
            sig = sig_separator + '\n' + sig;
          sig = '<pre>' + sig + '</pre>';
        }

        sigElem.innerHTML = sig;
      }
  global $MESSAGE, $OUTPUT;
        $a_signatures[$identity_id]['text'] = $sql_arr['signature'];
        $a_signatures[$identity_id]['is_html'] = ($sql_arr['html_signature'] == 1) ? true : false;
        if ($a_signatures[$identity_id]['is_html'])
        {
            $h2t = new html2text($a_signatures[$identity_id]['text'], false, false);
            $a_signatures[$identity_id]['plain_text'] = trim($h2t->get_text());
Fixed code: - Fix XSS issue where plain signatures wasn't secured in HTML mode (#1488613)
- Fix XSS issue where href="javascript:" wasn't secured (#1488613)
      sig = this.env.identity;
        sig = this.env.signatures[sig].text;
        sig = this.env.signatures[id].text;
      if (this.env.signatures[id])
        sigElem.innerHTML = this.env.signatures[id].html;
  global $MESSAGE, $OUTPUT, $RCMAIL, $compose_mode;
    $separator    = $RCMAIL->config->get('sig_above')
      && ($compose_mode == RCUBE_COMPOSE_REPLY || $compose_mode == RCUBE_COMPOSE_FORWARD) ? '---' : '-- ';
        $text = $html = $sql_arr['signature'];

        if ($sql_arr['html_signature']) {
            $h2t  = new html2text($sql_arr['signature'], false, false);
            $text = trim($h2t->get_text());
        }
        else {
            $html = htmlentities($html, ENT_NOQUOTES, RCMAIL_CHARSET);
        }

        if (!preg_match('/^--[ -]\r?\n/m', $text)) {
            $text = $separator . "\n" . $text;
            $html = $separator . "<br>" . $html;
        }

        if (!$sql_arr['html_signature']) {
            $html = "<pre>" . $html . "</pre>";

        $a_signatures[$identity_id]['text'] = $text;
        $a_signatures[$identity_id]['html'] = $html;