diff options
| -rw-r--r-- | includes/xmlrpc.inc | 70 | ||||
| -rw-r--r-- | includes/xmlrpcs.inc | 9 | ||||
| -rw-r--r-- | modules/block.module | 2 | ||||
| -rw-r--r-- | modules/block/block.module | 2 | ||||
| -rw-r--r-- | modules/comment.module | 11 | ||||
| -rw-r--r-- | modules/comment/comment.module | 11 | ||||
| -rw-r--r-- | modules/filter.module | 27 | ||||
| -rw-r--r-- | modules/filter/filter.module | 27 | ||||
| -rw-r--r-- | modules/node.module | 17 | ||||
| -rw-r--r-- | modules/node/node.module | 17 |
10 files changed, 114 insertions, 79 deletions
diff --git a/includes/xmlrpc.inc b/includes/xmlrpc.inc index dce2ddcf2..21a440095 100644 --- a/includes/xmlrpc.inc +++ b/includes/xmlrpc.inc @@ -61,12 +61,6 @@ $xmlrpcTypes=array($xmlrpcI4 => 1, $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 @@ $xmlrpcerruser=800; // 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 @@ $xmlrpc_backslash=chr(92).chr(92); $_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); } } diff --git a/includes/xmlrpcs.inc b/includes/xmlrpcs.inc index c98e5dd51..6c1fe80aa 100644 --- a/includes/xmlrpcs.inc +++ b/includes/xmlrpcs.inc @@ -246,7 +246,7 @@ class xmlrpc_server { 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 @@ class xmlrpc_server { } 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, diff --git a/modules/block.module b/modules/block.module index 75a69b2f6..045cfe8f2 100644 --- a/modules/block.module +++ b/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; } } diff --git a/modules/block/block.module b/modules/block/block.module index 75a69b2f6..045cfe8f2 100644 --- a/modules/block/block.module +++ b/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; } } diff --git a/modules/comment.module b/modules/comment.module index 0bcb91448..539bc9f9a 100644 --- a/modules/comment.module +++ b/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 { diff --git a/modules/comment/comment.module b/modules/comment/comment.module index 0bcb91448..539bc9f9a 100644 --- a/modules/comment/comment.module +++ b/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 { diff --git a/modules/filter.module b/modules/filter.module index 80b9edcef..410667dd1 100644 --- a/modules/filter.module +++ b/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); } diff --git a/modules/filter/filter.module b/modules/filter/filter.module index 80b9edcef..410667dd1 100644 --- a/modules/filter/filter.module +++ b/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); } diff --git a/modules/node.module b/modules/node.module index e454de575..63e1ca02a 100644 --- a/modules/node.module +++ b/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); diff --git a/modules/node/node.module b/modules/node/node.module index e454de575..63e1ca02a 100644 --- a/modules/node/node.module +++ b/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); |