Security: fixes for the HTML filter to counter further XSS exploits:

HTML attachments containing 'data:' URLs, Internet Explorer-specifc
charset conversion exploits, and request forgery through included
images. Thanks to Mikhail Markin, Tomas Kuliavas and Michael Jordon
for reporting these issues. [CVE-2007-1262]

ChangeLog

@@ -193,6 +193,11 @@ Version 1.5.2 - SVN
     domains.  If plugins use this function, it fixes #1434043.
   - Add dynamic textarea sizing slider control to compose screen (default_advanced 
     skin)
+  - Security: fixes for the HTML filter to counter further XSS exploits:
+    HTML attachments containing 'data:' URLs, Internet Explorer-specifc
+    charset conversion exploits, and request forgery through included
+    images. Thanks to Mikhail Markin, Tomas Kuliavas and Michael Jordon
+    for reporting these issues. [CVE-2007-1262]
 
 Version 1.5.1 (branched on 2006-02-12)
 --------------------------------------

functions/mime.php

@@ -426,13 +426,16 @@ function formatBody($imap_stream, $message, $color, $wrap_at, $ent_num, $id, $ma
                 $body = $oTemplate->fetch('read_html_iframe.tpl');
             } else {
                 // old way of html rendering
-                $body = magicHTML($body, $id, $message, $mailbox);
                 /**
                  * convert character set. charset_decode does not remove html special chars
                  * applied by magicHTML functions and does not sanitize them second time if
                  * fourth argument is true.
                  */
-                $body = charset_decode($body_message->header->getParameter('charset'),$body,false,true);
+                $charset = $body_message->header->getParameter('charset');
+                if (!empty($charset)) {
+                    $body = charset_decode($charset,$body,false,true);
+                }
+                $body = magicHTML($body, $id, $message, $mailbox);
             }
         } else {
             translateText($body, $wrap_at,
@@ -1196,8 +1199,8 @@ function sq_fixIE_idiocy(&$attvalue) {
                         array('ʟ', 'ʟ' ,/* L UNICODE IPA Extension */
                               'ʀ', 'ʀ' ,/* R UNICODE IPA Extension */
                               'ɴ', 'ɴ' ,/* N UNICODE IPA Extension */
-                              'Ｅ', '&#65317' ,/* Unicode FULLWIDTH LATIN CAPITAL LETTER E */
-                              'ｅ', '&#65349' ,/* Unicode FULLWIDTH LATIN SMALL LETTER E */
+                              'Ｅ', 'Ｅ' ,/* Unicode FULLWIDTH LATIN CAPITAL LETTER E */
+                              'ｅ', 'ｅ' ,/* Unicode FULLWIDTH LATIN SMALL LETTER E */
                               'Ｘ', 'Ｘ',/* Unicode FULLWIDTH LATIN CAPITAL LETTER X */
                               'ｘ', 'ｘ',/* Unicode FULLWIDTH LATIN SMALL LETTER X */
                               'Ｐ', 'Ｐ',/* Unicode FULLWIDTH LATIN CAPITAL LETTER P */
@@ -1217,26 +1220,34 @@ function sq_fixIE_idiocy(&$attvalue) {
                               'Ｕ', 'Ｕ',/* Unicode FULLWIDTH LATIN CAPITAL LETTER U */
                               'ｕ', 'ｕ',/* Unicode FULLWIDTH LATIN SMALL LETTER U */
                               'ⁿ', 'ⁿ' ,/* Unicode SUPERSCRIPT LATIN SMALL LETTER N */
-                              '艤', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER E */   // in unicode this is some Chinese char range
-                              '芅', /* Shift JIS FULLWIDTH LATIN SMALL LETTER E */
-                              '艷', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER X */
-                              '芘', /* Shift JIS FULLWIDTH LATIN SMALL LETTER X */
-                              '良', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER P */
-                              '芐', /* Shift JIS FULLWIDTH LATIN SMALL LETTER P */
-                              '艱', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER R */
-                              '芒', /* Shift JIS FULLWIDTH LATIN SMALL LETTER R */
-                              '色', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER S */
-                              '芓', /* Shift JIS FULLWIDTH LATIN SMALL LETTER S */
-                              '艨', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER I */
-                              '芉', /* Shift JIS FULLWIDTH LATIN SMALL LETTER I */
-                              '艮', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER O */
-                              '芏', /* Shift JIS FULLWIDTH LATIN SMALL LETTER O */
-                              '艭', /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER N */
-                              '芎'), /* Shift JIS FULLWIDTH LATIN SMALL LETTER N */
+                              "\xEF\xBC\xA5", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER E */   // in unicode this is some Chinese char range
+                              "\xEF\xBD\x85", /* Shift JIS FULLWIDTH LATIN SMALL LETTER E */
+                              "\xEF\xBC\xB8", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER X */
+                              "\xEF\xBD\x98", /* Shift JIS FULLWIDTH LATIN SMALL LETTER X */
+                              "\xEF\xBC\xB0", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER P */
+                              "\xEF\xBD\x90", /* Shift JIS FULLWIDTH LATIN SMALL LETTER P */
+                              "\xEF\xBC\xB2", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER R */
+                              "\xEF\xBD\x92", /* Shift JIS FULLWIDTH LATIN SMALL LETTER R */
+                              "\xEF\xBC\xB3", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER S */
+                              "\xEF\xBD\x93", /* Shift JIS FULLWIDTH LATIN SMALL LETTER S */
+                              "\xEF\xBC\xA9", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER I */
+                              "\xEF\xBD\x89", /* Shift JIS FULLWIDTH LATIN SMALL LETTER I */
+                              "\xEF\xBC\xAF", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER O */
+                              "\xEF\xBD\x8F", /* Shift JIS FULLWIDTH LATIN SMALL LETTER O */
+                              "\xEF\xBC\xAE", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER N */
+                              "\xEF\xBD\x8E", /* Shift JIS FULLWIDTH LATIN SMALL LETTER N */
+                              "\xEF\xBC\xAC", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER L */
+                              "\xEF\xBD\x8C", /* Shift JIS FULLWIDTH LATIN SMALL LETTER L */
+                              "\xEF\xBC\xB5", /* Shift JIS FULLWIDTH LATIN CAPITAL LETTER U */
+                              "\xEF\xBD\x95", /* Shift JIS FULLWIDTH LATIN SMALL LETTER U */
+                              "\xE2\x81\xBF", /* Shift JIS FULLWIDTH SUPERSCRIPT N */
+                              "\xCA\x9F", /* L UNICODE IPA Extension */
+                              "\xCA\x80", /* R UNICODE IPA Extension */
+                              "\xC9\xB4"),  /* N UNICODE IPA Extension */
                        array('l', 'l', 'r','r','n','n',
-                             'E','E','e','e','X','X','x','x','P','P','p','p','S','S','s','s','I','I',
-                             'i','i','O','O','o','o','N','N','n','n','L','L','l','l','U','U','u','u','n',
-                             'E','e','X','x','P','p','S','s','I','i','O','o','N','n'));
+                             'E','E','e','e','X','X','x','x','P','P','p','p','R','R','r','r','S','S','s','s','I','I',
+                             'i','i','O','O','o','o','N','N','n','n','L','L','l','l','U','U','u','u','n','n',
+                             'E','e','X','x','P','p','R','r','S','s','I','i','O','o','N','n','L','l','U','u','n','l','r','n'));
     $attvalue = str_replace($aDangerousCharsReplacementTable[0],$aDangerousCharsReplacementTable[1],$attvalue);
 
     // Escapes are useful for special characters like "{}[]()'&. In other cases they are
@@ -1751,38 +1762,34 @@ function sq_fixatts($tagname,
                             preg_replace($valmatch, $valrepl, $attvalue);
                         if ($newvalue != $attvalue){
                             $attary{$attname} = $newvalue;
+                            $attvalue = $newvalue;
                         }
                     }
                 }
             }
         }
-
-        /**
-         * Replace empty src tags with the blank image.  src is only used
-         * for frames, images, and image inputs.  Doing a replace should
-         * not affect them working as should be, however it will stop
-         * IE from being kicked off when src for img tags are not set
-         */
-        if (($attname == 'src') && ($attvalue == '""')) {
-            $attary{$attname} = '"' . SM_PATH . 'images/blank.png"';
-        }
-
-        /**
-         * Turn cid: urls into http-friendly ones.
-         */
-        if (preg_match("/^[\'\"]\s*cid:/si", $attvalue)){
-            $attary{$attname} = sq_cid2http($message, $id, $attvalue, $mailbox);
+        if ($attname == 'style') {
+            if (preg_match('/[\0-\37\200-\377]+/',$attvalue)) {
+                // 8bit and control characters in style attribute values can be used for XSS, remove them
+                $attary{$attname} = '"disallowed character"';
+            }
+            preg_match_all("/url\s*\((.+)\)/si",$attvalue,$aMatch);
+            if (count($aMatch)) {
+                foreach($aMatch[1] as $sMatch) {
+                    // url value
+                    $urlvalue = $sMatch;
+                    sq_fix_url($attname, $urlvalue, $message, $id, $mailbox,"'");
+                    $attary{$attname} = str_replace($sMatch,$urlvalue,$attvalue);
+                }
+            }
         }
-
         /**
-         * "Hack" fix for Outlook using propriatary outbind:// protocol in img tags.
-         * One day MS might actually make it match something useful, for now, falling
-         * back to using cid2http, so we can grab the blank.png.
+         * Use white list based filtering on attributes which can contain url's
          */
-        if (preg_match("/^[\'\"]\s*outbind:\/\//si", $attvalue)) {
-            $attary{$attname} = sq_cid2http($message, $id, $attvalue, $mailbox);
+        else if ($attname == 'href' || $attname == 'src' || $attname == 'background') {
+            sq_fix_url($attname, $attvalue, $message, $id, $mailbox);
+            $attary{$attname} = $attvalue;
         }
-
     }
     /**
      * See if we need to append any attributes to this tag.
@@ -1795,6 +1802,98 @@ function sq_fixatts($tagname,
     return $attary;
 }
 
+/**
+ * This function filters url's
+ *
+ * @param  $attvalue        String with attribute value to filter
+ * @param  $message         message object
+ * @param  $id               message id
+ * @param  $mailbox         mailbox
+ * @param  $sQuote          quoting characters around url's
+ */
+function sq_fix_url($attname, &$attvalue, $message, $id, $mailbox,$sQuote = '"') {
+    $attvalue = trim($attvalue);
+    if ($attvalue && ($attvalue[0] =='"'|| $attvalue[0] == "'")) {
+        // remove the double quotes
+        $sQuote = $attvalue[0];
+        $attvalue = trim(substr($attvalue,1,-1));
+    }
+
+    if( !sqgetGlobalVar('view_unsafe_images', $view_unsafe_images, SQ_GET) ) {
+        $view_unsafe_images = false;
+    }
+    $secremoveimg = '../images/' . _("sec_remove_eng.png");
+
+    /**
+     * Replace empty src tags with the blank image.  src is only used
+     * for frames, images, and image inputs.  Doing a replace should
+     * not affect them working as should be, however it will stop
+     * IE from being kicked off when src for img tags are not set
+     */
+    if ($attvalue == '') {
+        $attvalue = '"' . SM_PATH . 'images/blank.png"';
+    } else {
+        // first, disallow 8 bit characters and control characters
+        if (preg_match('/[\0-\37\200-\377]+/',$attvalue)) {
+            switch ($attname) {
+                case 'href':
+                    $attvalue = $sQuote . 'http://invalid-stuff-detected.example.com' . $sQuote;
+                    break;
+                default:
+                    $attvalue = $sQuote . SM_PATH . 'images/blank.png'. $sQuote;
+                    break;
+            }
+        } else {
+            $aUrl = parse_url($attvalue);
+            if (isset($aUrl['scheme'])) {
+                switch(strtolower($aUrl['scheme'])) {
+                    case 'http':
+                    case 'https':
+                    case 'ftp':
+                        if ($attname != 'href') {
+                            if ($view_unsafe_images == false) {
+                                $attvalue = $sQuote . $secremoveimg . $sQuote;
+                            } else {
+                                if (isset($aUrl['path'])) {
+                                    // validate image extension.
+                                    $ext = strtolower(substr($aUrl['path'],strrpos($aUrl['path'],'.')));
+                                    if (!in_array($ext,array('.jpeg','.jpg','xjpeg','.gif','.bmp','.jpe','.png','.xbm'))) {
+                                        $attvalue = $sQuote . SM_PATH . 'images/blank.png'. $sQuote;
+                                    }
+                                } else {
+                                    $attvalue = $sQuote . SM_PATH . 'images/blank.png'. $sQuote;
+                                }
+                            }
+                        }
+                        break;
+                    case 'outbind':
+                        /**
+                         * "Hack" fix for Outlook using propriatary outbind:// protocol in img tags.
+                         * One day MS might actually make it match something useful, for now, falling
+                         * back to using cid2http, so we can grab the blank.png.
+                         */
+                        $attvalue = sq_cid2http($message, $id, $attvalue, $mailbox);
+                        break;
+                    case 'cid':
+                        /**
+                            * Turn cid: urls into http-friendly ones.
+                            */
+                        $attvalue = sq_cid2http($message, $id, $attvalue, $mailbox);
+                        break;
+                    default:
+                        $attvalue = $sQuote . SM_PATH . 'images/blank.png' . $sQuote;
+                        break;
+                }
+            } else {
+                if (!(isset($aUrl['path']) && $aUrl['path'] == $secremoveimg)) {
+                    // parse_url did not lead to satisfying result
+                    $attvalue = $sQuote . SM_PATH . 'images/blank.png' . $sQuote;
+                }
+            }
+        }
+    }
+}
+
 /**
  * This function edits the style definition to make them friendly and
  * usable in SquirrelMail.
@@ -1886,6 +1985,12 @@ function sq_fixstyle($body, $pos, $message, $id, $mailbox){
     //   $content = preg_replace("|url\s*\(\s*([\'\"])\s*\S+script\s*:.*?([\'\"])\s*\)|si",
     //                           "url(\\1$secremoveimg\\2)", $content);
 
+    // first check for 8bit sequences and disallowed control characters
+    if (preg_match('/[\16-\37\200-\377]+/',$content)) {
+        $content = '<!-- style block removed by html filter due to presence of 8bit characters -->';
+        return array($content, $newpos);
+    }
+
     // IE Sucks hard. We have a special function for it.
     sq_fixIE_idiocy($content);
 
@@ -1895,48 +2000,19 @@ function sq_fixstyle($body, $pos, $message, $id, $mailbox){
     // translate ur\l and variations (IE parses that)
     // TODO check if the sq_fixIE_idiocy function already handles this.
     $content = preg_replace("/(\\\\)?u(\\\\)?r(\\\\)?l(\\\\)?/i", 'url', $content);
-    // NB I insert NUL characters to keep to avoid an infinite loop. They are removed after the loop.
-    while (preg_match("/url\s*\(\s*[\'\"]?([^:]+):(.*)?[\'\"]?\s*\)/si", $content, $matches)) {
-        $sProto = strtolower($matches[1]);
-        switch ($sProto) {
-            /**
-             * Fix url('https*://.*) declarations but only if $view_unsafe_images
-             * is false.
-             */
-            case 'https':
-            case 'http':
-                if (!$view_unsafe_images){
-
-                    $sExpr = "/url\s*\(\s*[\'\"]?\s*$sProto*:.*[\'\"]?\s*\)/si";
-                    $content = preg_replace($sExpr, "u\0r\0l(\\1$secremoveimg\\2)", $content);
-
-                } else {
-                    $content = preg_replace('/url/i',"u\0r\0l",$content);
-                }
-                break;
-            /**
-             * Fix urls that refer to cid:
-             */
-            case 'cid':
-                $cidurl = 'cid:'. $matches[2];
-                $httpurl = sq_cid2http($message, $id, $cidurl, $mailbox);
-                // escape parentheses that can modify the regular expression
-                $cidurl = str_replace(array('(',')'),array('\\(','\\)'),$cidurl);
-                $content = preg_replace("|url\s*\(\s*$cidurl\s*\)|si",
-                                        "u\0r\0l($httpurl)", $content);
-                break;
-            default:
-                /**
-                 * replace url with protocol other then the white list
-                 * http,https and cid by an empty string.
-                 */
-                $content = preg_replace("/url\s*\(\s*[\'\"]?([^:]+):(.*)?[\'\"]?\s*\)/si",
-                                 "", $content);
-                break;
+    preg_match_all("/url\s*\((.+)\)/si",$content,$aMatch);
+    if (count($aMatch)) {
+        $aValue = $aReplace = array();
+        foreach($aMatch[1] as $sMatch) {
+            // url value
+            $urlvalue = $sMatch;
+            sq_fix_url('style',$urlvalue, $message, $id, $mailbox,"'");
+            $aValue[] = $sMatch;
+            $aReplace[] = $urlvalue;
         }
+        $content = str_replace($aValue,$aReplace,$content);
     }
-    // remove NUL
-    $content = str_replace("\0", "", $content);
+
    /**
     * Remove any backslashes, entities, and extraneous whitespace.
     */
@@ -2373,7 +2449,7 @@ function magicHTML($body, $id, $message, $mailbox = 'INBOX', $take_mailto_links
                     "idiocy",
                     "idiocy",
                     "idiocy",
-                    "",
+                    "idiocy",
                     "url",
                     "url(\\1#\\1)",
                     "url(\\1#\\1)",
@@ -2419,7 +2495,7 @@ function magicHTML($body, $id, $message, $mailbox = 'INBOX', $take_mailto_links
                            $id,
                            $mailbox
                            );
-    if (preg_match("|$secremoveimg|i", $trusted)){
+    if (strpos($trusted,$secremoveimg)){
         $has_unsafe_images = true;
     }
 

src/compose.php

@@ -45,31 +45,37 @@ sqgetGlobalVar('delayed_errors',  $delayed_errors,  SQ_SESSION);
 $oErrorHandler->setDelayedErrors(true);
 
 /** SESSION/POST/GET VARS */
-sqgetGlobalVar('session',$session);
-sqgetGlobalVar('mailbox',$mailbox);
-if(!sqgetGlobalVar('identity',$identity)) {
+sqgetGlobalVar('send', $send, SQ_POST);
+// Send can only be achieved by setting $_POST var. If Send = true then
+// retrieve other form fields from $_POST
+if (isset($send) && $send) {
+    $SQ_GLOBAL = SQ_POST;
+} else {
+    $SQ_GLOBAL = SQ_FORM;
+}
+sqgetGlobalVar('session',$session, $SQ_GLOBAL);
+sqgetGlobalVar('mailbox',$mailbox, $SQ_GLOBAL);
+if(!sqgetGlobalVar('identity',$identity, $SQ_GLOBAL)) {
     $identity=0;
 }
-sqgetGlobalVar('send_to',$send_to);
-sqgetGlobalVar('send_to_cc',$send_to_cc);
-sqgetGlobalVar('send_to_bcc',$send_to_bcc);
-sqgetGlobalVar('subject',$subject);
-sqgetGlobalVar('body',$body);
-sqgetGlobalVar('mailprio',$mailprio);
-sqgetGlobalVar('request_mdn',$request_mdn);
-sqgetGlobalVar('request_dr',$request_dr);
-sqgetGlobalVar('html_addr_search',$html_addr_search);
-sqgetGlobalVar('mail_sent',$mail_sent);
-sqgetGlobalVar('passed_id',$passed_id);
-sqgetGlobalVar('passed_ent_id',$passed_ent_id);
-sqgetGlobalVar('send',$send);
-
-sqgetGlobalVar('attach',$attach);
-
-sqgetGlobalVar('draft',$draft);
-sqgetGlobalVar('draft_id',$draft_id);
-sqgetGlobalVar('ent_num',$ent_num);
-sqgetGlobalVar('saved_draft',$saved_draft);
+sqgetGlobalVar('send_to',$send_to, $SQ_GLOBAL);
+sqgetGlobalVar('send_to_cc',$send_to_cc, $SQ_GLOBAL);
+sqgetGlobalVar('send_to_bcc',$send_to_bcc, $SQ_GLOBAL);
+sqgetGlobalVar('subject',$subject, $SQ_GLOBAL);
+sqgetGlobalVar('body',$body, $SQ_GLOBAL);
+sqgetGlobalVar('mailprio',$mailprio, $SQ_GLOBAL);
+sqgetGlobalVar('request_mdn',$request_mdn, $SQ_GLOBAL);
+sqgetGlobalVar('request_dr',$request_dr, $SQ_GLOBAL);
+sqgetGlobalVar('html_addr_search',$html_addr_search, $SQ_GLOBAL);
+sqgetGlobalVar('mail_sent',$mail_sent, $SQ_GLOBAL);
+sqgetGlobalVar('passed_id',$passed_id, $SQ_GLOBAL);
+sqgetGlobalVar('passed_ent_id',$passed_ent_id, $SQ_GLOBAL);
+
+sqgetGlobalVar('attach',$attach, SQ_POST);
+sqgetGlobalVar('draft',$draft, SQ_POST);
+sqgetGlobalVar('draft_id',$draft_id, $SQ_GLOBAL);
+sqgetGlobalVar('ent_num',$ent_num, $SQ_GLOBAL);
+sqgetGlobalVar('saved_draft',$saved_draft, SQ_FORM);
 
 if ( sqgetGlobalVar('delete_draft',$delete_draft) ) {
     $delete_draft = (int)$delete_draft;

src/view_text.php

@@ -65,11 +65,11 @@ if (isset($languages[$squirrelmail_language]['XTRA_CODE']) &&
 
 if ($type1 == 'html' || (isset($override_type1) &&  $override_type1 == 'html')) {
     $ishtml = TRUE;
-    $body = MagicHTML( $body, $passed_id, $message, $mailbox);
     // html attachment with character set information
     if (! empty($charset)) {
         $body = charset_decode($charset,$body,false,true);
     }
+    $body = MagicHTML( $body, $passed_id, $message, $mailbox);
 } else {
     $ishtml = FALSE;
     translateText($body, $wrap_at, $charset);