- Fixed problems with filter formats and problem with XML-RPC server.

includes/xmlrpc.inc

@@ -61,12 +61,6 @@
     $xmlrpcArray => 2,
     $xmlrpcStruct => 3);
 
-$xmlEntities=array(   "amp" => "&",
-    "quot" => '"',
-    "lt" => "<",
-    "gt" => ">",
-    "apos" => "'");
-
 $xmlrpcerr["unknown_method"]=1;
 $xmlrpcstr["unknown_method"]="Unknown method";
 $xmlrpcerr["invalid_return"]=2;
@@ -92,9 +86,6 @@
 // let XML parse errors start at 100
 $xmlrpcerrxml=100;
 
-// formulate backslashes for escaping regexp
-$xmlrpc_backslash=chr(92).chr(92);
-
 // used to store state during parsing
 // quick explanation of components:
 //   st - used to build up a string for evaluation
@@ -109,36 +100,6 @@
 
 $_xh=array();
 
-function xmlrpc_entity_decode($string) {
-  $top=split("&", $string);
-  $op="";
-  $i=0;
-  while($i<sizeof($top)) {
-    if (ereg("^([#a-zA-Z0-9]+);", $top[$i], $regs)) {
-      $op.=ereg_replace("^[#a-zA-Z0-9]+;",
-          xmlrpc_lookup_entity($regs[1]),
-          $top[$i]);
-    } else {
-      if ($i==0)
-        $op=$top[$i];
-      else
-        $op.="&" . $top[$i];
-    }
-    $i++;
-  }
-  return $op;
-}
-
-function xmlrpc_lookup_entity($ent) {
-  global $xmlEntities;
-
-  if (isset($xmlEntities[strtolower($ent)]))
-    return $xmlEntities[strtolower($ent)];
-  if (ereg("^#([0-9]+)$", $ent, $regs))
-    return chr($regs[1]);
-  return "?";
-}
-
 function xmlrpc_se($parser, $name, $attrs) {
   global $_xh, $xmlrpcDateTime, $xmlrpcString;
 
@@ -237,9 +198,9 @@ function xmlrpc_ee($parser, $name) {
       case "BASE64":
       if ($_xh[$parser]['qt']==1) {
         // we use double quotes rather than single so backslashification works OK
-        $_xh[$parser]['st'].="\"". $_xh[$parser]['ac'] . "\"";
+        $_xh[$parser]['st'].="'". $_xh[$parser]['ac'] ."'";
       } else if ($_xh[$parser]['qt']==2) {
-        $_xh[$parser]['st'].="base64_decode('". $_xh[$parser]['ac'] . "')";
+        $_xh[$parser]['st'].="base64_decode('". $_xh[$parser]['ac'] ."')";
       } else if ($name=="BOOLEAN") {
         $_xh[$parser]['st'].=$_xh[$parser]['ac'];
       } else {
@@ -262,12 +223,12 @@ function xmlrpc_ee($parser, $name) {
       // deal with a string value
       if (strlen($_xh[$parser]['ac'])>0 &&
           $_xh[$parser]['vt']==$xmlrpcString) {
-        $_xh[$parser]['st'].="\"". $_xh[$parser]['ac'] . "\"";
+        $_xh[$parser]['st'].="'". $_xh[$parser]['ac'] . "'";
       }
     // This if() detects if no scalar was inside <VALUE></VALUE>
     // and pads an empty "".
     if($_xh[$parser]['st'][strlen($_xh[$parser]['st'])-1] == '(') {
-      $_xh[$parser]['st'].= '""';
+      $_xh[$parser]['st'].= "''";
     }
     $_xh[$parser]['st'].=", '" . $_xh[$parser]['vt'] . "')";
     if ($_xh[$parser]['cm']) $_xh[$parser]['st'].=",";
@@ -306,7 +267,7 @@ function xmlrpc_ee($parser, $name) {
 
 function xmlrpc_cd($parser, $data)
 {
-  global $_xh, $xmlrpc_backslash;
+  global $_xh;
 
   //if (ereg("^[\n\r \t]+$", $data)) return;
   // print "adding [${data}]\n";
@@ -323,12 +284,23 @@ function xmlrpc_cd($parser, $data)
     }
     // replace characters that eval would
     // do special things with
-    $_xh[$parser]['ac'].=str_replace('$', '\$',
-        str_replace('"', '\"', str_replace(chr(92),
-            $xmlrpc_backslash, $data)));
+    $_xh[$parser]['ac'].= xmlrpc_escape_php($data);
   }
 }
 
+/**
+ * Escapes a piece of text so it can be placed literally between single quotes
+ * as a string inside PHP code.
+ *
+ * A single slash is converted to a double slash, a single quote converted to
+ * a slash followed by a quote.
+ */
+function xmlrpc_escape_php($data) {
+  return str_replace(array('\\', "'"),
+                     array('\\\\', "\\'"),
+                     $data);
+}
+
 function xmlrpc_dh($parser, $data)
 {
   global $_xh;
@@ -337,9 +309,7 @@ function xmlrpc_dh($parser, $data)
       $_xh[$parser]['qt']=1;
       $_xh[$parser]['lv']=2;
     }
-    $_xh[$parser]['ac'].=str_replace('$', '\$',
-        str_replace('"', '\"', str_replace(chr(92),
-            $xmlrpc_backslash, $data)));
+    $_xh[$parser]['ac'].= xmlrpc_escape_php($data);
   }
 }
 

includes/xmlrpcs.inc

@@ -246,7 +246,7 @@ function parseRequest($data="") {
     for($i=0; $i<sizeof($_xh[$parser]['params']); $i++) {
       //print "<!-- " . $_xh[$parser]['params'][$i]. "-->\n";
       $plist.="$i - " .  $_xh[$parser]['params'][$i]. " \n";
-      eval('$m->addParam(' . $_xh[$parser]['params'][$i]. ");");
+      $m->addParam(eval('return '. $_xh[$parser]['params'][$i] .';'));
     }
     // uncomment this to really see what the server's getting!
     // xmlrpc_debugmsg($plist);
@@ -265,13 +265,12 @@ function parseRequest($data="") {
       }
       if ( (!isset($dmap[$methName]['signature']))
            || $sr[0]) {
+        $f = $dmap[$methName]['function'];
         // if no signature or correct signature
         if ($sysCall) {
-          eval('$r=' . $dmap[$methName]['function'] .
-               '($this, $m);');
+          $r = $f($this, $m);
         } else {
-          eval('$r=' . $dmap[$methName]['function'] .
-               '($m);');
+          $r = $f($m);
         }
       } else {
         $r=new xmlrpcresp(0,

modules/block.module

@@ -104,7 +104,7 @@ function block_block($op = 'list', $delta = 0, $edit = array()) {
     case 'view':
       $block = db_fetch_object(db_query('SELECT * FROM {boxes} WHERE bid = %d', $delta));
       $data['subject'] = check_plain($block->title);
-      $data['content'] = check_output($block->body, $block->format);
+      $data['content'] = check_output($block->body, $block->format, FALSE);
       return $data;
   }
 }

modules/block/block.module

@@ -104,7 +104,7 @@ function block_block($op = 'list', $delta = 0, $edit = array()) {
     case 'view':
       $block = db_fetch_object(db_query('SELECT * FROM {boxes} WHERE bid = %d', $delta));
       $data['subject'] = check_plain($block->title);
-      $data['content'] = check_output($block->body, $block->format);
+      $data['content'] = check_output($block->body, $block->format, FALSE);
       return $data;
   }
 }

modules/comment.module

@@ -280,7 +280,7 @@ function comment_nodeapi(&$node, $op, $arg = 0) {
       $text = '';
       $comments = db_query('SELECT subject, comment, format FROM {comments} WHERE nid = %d AND status = %d', $node->nid, COMMENT_PUBLISHED);
       while ($comment = db_fetch_object($comments)) {
-        $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_output($comment->comment, $comment->format);
+        $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_output($comment->comment, $comment->format, FALSE);
       }
       return $text;
 
@@ -440,6 +440,7 @@ function comment_validate_form($edit) {
     // 1) Filter it into HTML
     // 2) Strip out all HTML tags
     // 3) Convert entities back to plain-text.
+	// Note: format is checked by check_output().
     $edit['subject'] = truncate_utf8(decode_entities(strip_tags(check_output($edit['comment'], $edit['format']))), 29, TRUE);
   }
 
@@ -501,7 +502,9 @@ function comment_preview($edit) {
   $comment->name = check_plain($user->name ? $user->name : $comment->name);
 
   // Preview the comment.
-  $output .= theme('comment_preview', $comment, theme('links', module_invoke_all('link', 'comment', $comment, 1)));
+  if (!form_get_errors()) {
+    $output .= theme('comment_preview', $comment, theme('links', module_invoke_all('link', 'comment', $comment, 1)));
+  }
   $output .= theme('comment_form', $edit, t('Reply'));
 
   if ($edit['pid']) {
@@ -982,7 +985,7 @@ function comment_delete($cid) {
                     t('Any replies to this comment will be lost. This action cannot be undone.'),
                     t('Delete'));
     // Show comment that is being deleted
-    $comment->comment = check_output($comment->comment, $comment->format);
+    $comment->comment = check_output($comment->comment, $comment->format, FALSE);
     $output .= theme('comment', $comment);
 
   }
@@ -1447,7 +1450,7 @@ function theme_comment_view($comment, $links = '', $visible = 1) {
 
   // Switch to folded/unfolded view of the comment
   if ($visible) {
-    $comment->comment = check_output($comment->comment, $comment->format);
+    $comment->comment = check_output($comment->comment, $comment->format, FALSE);
     $output .= theme('comment', $comment, $links);
   }
   else {

modules/comment/comment.module

@@ -280,7 +280,7 @@ function comment_nodeapi(&$node, $op, $arg = 0) {
       $text = '';
       $comments = db_query('SELECT subject, comment, format FROM {comments} WHERE nid = %d AND status = %d', $node->nid, COMMENT_PUBLISHED);
       while ($comment = db_fetch_object($comments)) {
-        $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_output($comment->comment, $comment->format);
+        $text .= '<h2>'. check_plain($comment->subject) .'</h2>'. check_output($comment->comment, $comment->format, FALSE);
       }
       return $text;
 
@@ -440,6 +440,7 @@ function comment_validate_form($edit) {
     // 1) Filter it into HTML
     // 2) Strip out all HTML tags
     // 3) Convert entities back to plain-text.
+	// Note: format is checked by check_output().
     $edit['subject'] = truncate_utf8(decode_entities(strip_tags(check_output($edit['comment'], $edit['format']))), 29, TRUE);
   }
 
@@ -501,7 +502,9 @@ function comment_preview($edit) {
   $comment->name = check_plain($user->name ? $user->name : $comment->name);
 
   // Preview the comment.
-  $output .= theme('comment_preview', $comment, theme('links', module_invoke_all('link', 'comment', $comment, 1)));
+  if (!form_get_errors()) {
+    $output .= theme('comment_preview', $comment, theme('links', module_invoke_all('link', 'comment', $comment, 1)));
+  }
   $output .= theme('comment_form', $edit, t('Reply'));
 
   if ($edit['pid']) {
@@ -982,7 +985,7 @@ function comment_delete($cid) {
                     t('Any replies to this comment will be lost. This action cannot be undone.'),
                     t('Delete'));
     // Show comment that is being deleted
-    $comment->comment = check_output($comment->comment, $comment->format);
+    $comment->comment = check_output($comment->comment, $comment->format, FALSE);
     $output .= theme('comment', $comment);
 
   }
@@ -1447,7 +1450,7 @@ function theme_comment_view($comment, $links = '', $visible = 1) {
 
   // Switch to folded/unfolded view of the comment
   if ($visible) {
-    $comment->comment = check_output($comment->comment, $comment->format);
+    $comment->comment = check_output($comment->comment, $comment->format, FALSE);
     $output .= theme('comment', $comment, $links);
   }
   else {

modules/filter.module

@@ -646,13 +646,34 @@ function filter_list_format($format) {
  * @{
  * Modules which need to have content filtered can use these functions to
  * interact with the filter system.
+ *
+ * For more info, see the hook_filter() documentation.
+ *
+ * Note: because filters can inject JavaScript or execute PHP code, security is
+ * vital here. When a user supplies a $format, you should validate it with
+ * filter_access($format) before accepting/using it. This is normally done in
+ * the validation stage of the node system. You should for example never make a
+ * preview of content in a disallowed format.
  */
 
 /**
  * Run all the enabled filters on a piece of text.
- */
-function check_output($text, $format = FILTER_FORMAT_DEFAULT) {
-  if (isset($text)) {
+ *
+ * @param $text
+ *    The text to be filtered.
+ * @param $format
+ *    The format of the text to be filtered. Specify FILTER_FORMAT_DEFAULT for
+ *    the default format.
+ * @param $check
+ *    Whether to check the $format with filter_access() first. Defaults to TRUE.
+ *    Note that this will check the permissions of the current user, so you
+ *    should specify $check = FALSE when viewing other people's content.  When
+ *    showing content that is not (yet) stored in the database (eg. upon preview),
+ *    set to TRUE so the user's permissions are checked.
+ */
+function check_output($text, $format = FILTER_FORMAT_DEFAULT, $check = TRUE) {
+  // When $check = true, do an access check on $format.
+  if (isset($text) && (!$check || filter_access($format))) {
     if ($format == FILTER_FORMAT_DEFAULT) {
       $format = variable_get('filter_default_format', 1);
     }

modules/filter/filter.module

@@ -646,13 +646,34 @@ function filter_list_format($format) {
  * @{
  * Modules which need to have content filtered can use these functions to
  * interact with the filter system.
+ *
+ * For more info, see the hook_filter() documentation.
+ *
+ * Note: because filters can inject JavaScript or execute PHP code, security is
+ * vital here. When a user supplies a $format, you should validate it with
+ * filter_access($format) before accepting/using it. This is normally done in
+ * the validation stage of the node system. You should for example never make a
+ * preview of content in a disallowed format.
  */
 
 /**
  * Run all the enabled filters on a piece of text.
- */
-function check_output($text, $format = FILTER_FORMAT_DEFAULT) {
-  if (isset($text)) {
+ *
+ * @param $text
+ *    The text to be filtered.
+ * @param $format
+ *    The format of the text to be filtered. Specify FILTER_FORMAT_DEFAULT for
+ *    the default format.
+ * @param $check
+ *    Whether to check the $format with filter_access() first. Defaults to TRUE.
+ *    Note that this will check the permissions of the current user, so you
+ *    should specify $check = FALSE when viewing other people's content.  When
+ *    showing content that is not (yet) stored in the database (eg. upon preview),
+ *    set to TRUE so the user's permissions are checked.
+ */
+function check_output($text, $format = FILTER_FORMAT_DEFAULT, $check = TRUE) {
+  // When $check = true, do an access check on $format.
+  if (isset($text) && (!$check || filter_access($format))) {
     if ($format == FILTER_FORMAT_DEFAULT) {
       $format = variable_get('filter_default_format', 1);
     }

modules/node.module

@@ -498,6 +498,13 @@ function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {
   if ($links) {
     $node->links = module_invoke_all('link', 'node', $node, !$page);
   }
+  // unset unused $node part so that a bad theme can not open a security hole
+  if ($teaser) {
+    unset($node->body);
+  }
+  else {
+    unset($node->teaser);
+  }
 
   return theme('node', $node, $teaser, $page);
 }
@@ -508,10 +515,10 @@ function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {
 function node_prepare($node, $teaser = FALSE) {
   $node->readmore = (strlen($node->teaser) < strlen($node->body));
   if ($teaser == FALSE) {
-    $node->body = check_output($node->body, $node->format);
+    $node->body = check_output($node->body, $node->format, FALSE);
   }
   else {
-    $node->teaser = check_output($node->teaser, $node->format);
+    $node->teaser = check_output($node->teaser, $node->format, FALSE);
   }
   return $node;
 }
@@ -567,7 +574,7 @@ function node_search($op = 'search', $keys = null) {
                            'user' => format_name($node),
                            'date' => $node->changed,
                            'extra' => $extra,
-                           'snippet' => search_excerpt($keys, check_output($node->body, $node->format)));
+                           'snippet' => search_excerpt($keys, check_output($node->body, $node->format, FALSE)));
       }
       return $results;
   }
@@ -1470,7 +1477,9 @@ function node_preview($node) {
 
     // Display a preview of the node:
     // Previewing alters $node so it needs to be cloned.
-    $output = theme('node_preview', drupal_clone($node));
+    if (!form_get_errors()) {
+      $output = theme('node_preview', drupal_clone($node));
+    }
 
     $output .= node_form($node);
 

modules/node/node.module

@@ -498,6 +498,13 @@ function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {
   if ($links) {
     $node->links = module_invoke_all('link', 'node', $node, !$page);
   }
+  // unset unused $node part so that a bad theme can not open a security hole
+  if ($teaser) {
+    unset($node->body);
+  }
+  else {
+    unset($node->teaser);
+  }
 
   return theme('node', $node, $teaser, $page);
 }
@@ -508,10 +515,10 @@ function node_view($node, $teaser = FALSE, $page = FALSE, $links = TRUE) {
 function node_prepare($node, $teaser = FALSE) {
   $node->readmore = (strlen($node->teaser) < strlen($node->body));
   if ($teaser == FALSE) {
-    $node->body = check_output($node->body, $node->format);
+    $node->body = check_output($node->body, $node->format, FALSE);
   }
   else {
-    $node->teaser = check_output($node->teaser, $node->format);
+    $node->teaser = check_output($node->teaser, $node->format, FALSE);
   }
   return $node;
 }
@@ -567,7 +574,7 @@ function node_search($op = 'search', $keys = null) {
                            'user' => format_name($node),
                            'date' => $node->changed,
                            'extra' => $extra,
-                           'snippet' => search_excerpt($keys, check_output($node->body, $node->format)));
+                           'snippet' => search_excerpt($keys, check_output($node->body, $node->format, FALSE)));
       }
       return $results;
   }
@@ -1470,7 +1477,9 @@ function node_preview($node) {
 
     // Display a preview of the node:
     // Previewing alters $node so it needs to be cloned.
-    $output = theme('node_preview', drupal_clone($node));
+    if (!form_get_errors()) {
+      $output = theme('node_preview', drupal_clone($node));
+    }
 
     $output .= node_form($node);