and (hopefully,) a final update... On Tue, Jul 19, 2005 at 10:52:43AM +0200, Martin Schulze wrote: > > 2 is trickier. we could either repeat the process i'm about finished > > with wrt mysql_foo for all the functions that pass variables to > > mysql_foo, or we could do the sanity checking in the function. as > > the former sounds ugly and even more time consuming i'm going to > > side with thte latter. > > The less work and the less intrusive the patch the better.
this is done now. turns out all the db-querying functions included a database.php file at the beginning of the function, which then included config.php, which then included sanitize.php. > > what i think i'm going to do is split sanitize.php into sanitize and > > sanitize-functions. sanitize will include_once sanitize-functions, > > so then sanitize can be included multiple times (otherwise i believe > > that php will bitch about functions being redefined), and i'll just > > slip in a line in each mysql-calling function to include sanitize, > > and add the variables in said functions to sanitize.php. > > Sounds good. this is done now. > > as for 3, well... there's a variable, which is stored in a cookie. > > the cookie name is cactilogin, and the value is an integer. want to > > guess what it does? a fix for this shouldn't be too hard, this kind > > of info should be stored in the session and not in the cookie. > > Hmm, having the user id stored in a cookie is common practice. > The variable obviously needs to be sanitised as well. the sanitizing functions now check all of get/post/cookie (and global scoped variables for one of them too because it changes their value), which after thinking about it seemed safer since there's no penalty for unset variables other than a few wasted cycles. as for the id thing, what i'm suspecting is that most/all pages are accepting the value of that cookie as the logged in userid without making sure the user is authenticated. but, i'm willing to keep my head in the sand and not dig deep enough to find out whether or not this really is a problem, because i'm just so fucking tired of dealing with this. anyway, all the goodies are at http://people.debian.org/~seanius/cacti/woody and attached is the interdiff between oldstable and my patch (i believe it also includes the latest changelog changes from you too, might want to doublecheck that). sean --
diff -u cacti-0.6.7/include/config.php cacti-0.6.7/include/config.php
--- cacti-0.6.7/include/config.php
+++ cacti-0.6.7/include/config.php
@@ -23,6 +23,21 @@
*/?>
<?
+/* whether or not we pull from a db, we need re-initilize */
+global $config;
+
+/* test for suspicious user-supplied variables that would otherwise
+ affect program execution, and if so zero out config for safety */
+if(isset($_GET["do_not_read_config"]) || isset($_POST["do_not_read_config"])
+ || isset($_GET["config"]) || isset($_POST["config"])){
+ $config = array();
+}
+$colors = array();
+
+## debian security backport ##
+require_once("sanitize.php");
+## debian security backport ##
+
## Debian packaging ##
include("/etc/cacti/config.php");
## Debian packaging ##
@@ -30,9 +45,6 @@
/* make sure this variable reflects your operating system type: 'unix' or
'win32' */
$cacti_server_os = "unix";
-/* whether or not we pull from a db, we need re-initilize */
-global $config;
-
if ($do_not_read_config != true) {
if (isset($config) == false) {
/* make a connection to the database */
diff -u cacti-0.6.7/debian/changelog cacti-0.6.7/debian/changelog
--- cacti-0.6.7/debian/changelog
+++ cacti-0.6.7/debian/changelog
@@ -1,3 +1,34 @@
+cacti (0.6.7-2.5) oldstable-security; urgency=high
+
+ * retooled version of the sanity checking patch, including a much
+ more comprehensive list of variable names to check. the patch also
+ checks for discrepencies in any of the three G/P/C variables, and
+ should address variables indirectly passed to sql queries.
+
+ -- sean finney <[EMAIL PROTECTED]> Tue, 19 Jul 2005 20:41:10 -0400
+
+cacti (0.6.7-2.4) oldstable-security; urgency=high
+
+ * Non-maintainer upload by the Security Team
+ * Switched to using $_REQUEST instead of $_GET and $_POST since this
+ version only uses $foo which is similar to $_REQUEST[foo]
+ * Reduced the number of tested variables to those actually used.
+
+ -- Martin Schulze <[EMAIL PROTECTED]> Fri, 15 Jul 2005 16:06:58 +0200
+
+cacti (0.6.7-2.3) oldstable-security; urgency=high
+
+ * update prepared for the security team by new maintainer.
+ * include backported updates against the two latest cacti security
+ releases. this includes the following CAN id's:
+ - CAN-2005-1524 (idefense remote file inclusion)
+ - CAN-2005-1525 (idefense SQL injection)
+ - CAN-2005-1526 (idefense remote code execution)
+ - CAN-2005-2148 (hardened-php advisories 032005 and 042005)
+ - CAN-2005-2149 (hardened-php advisory 052005)
+
+ -- sean finney <[EMAIL PROTECTED]> Mon, 11 Jul 2005 20:35:12 -0400
+
cacti (0.6.7-2.2) stable-security; urgency=medium
* Non-maintainer upload by Stable Release Manager
only in patch2:
unchanged:
--- cacti-0.6.7.orig/include/sanitize.php
+++ cacti-0.6.7/include/sanitize.php
@@ -0,0 +1,109 @@
+<?php
+
+include_once("sanitize-functions.php");
+
+/*
+ * the following fields are variables passed to sql queries
+ * that should always be numeric.
+ *
+ */
+$debsec_number_vars = array(
+ "autoscaleopts", "basevalue", "branch", "branch_id",
+ "cdeffunctionid", "cdefid", "colorid", "columnnumber",
+ "consolidationfunction", "data_source_id", "datasource_id",
+ "datasourcetypeid", "did", "dsid", "dstypeid", "fid", "gid",
+ "graph_height", "graphid", "graph_id", "graphpolicy",
+ "graph_start", "graph_template_id", "graphtypeid", "graph_width",
+ "heartbeat", "height", "hide", "hostid", "id", "imageformatid",
+ "isparent", "listviewtype", "loginopts", "lowerlimit",
+ "maxvalue", "menuid", "minvalue", "pagerefresh", "parent",
+ "rigid", "rra", "sequenceparent", "source_id",
+ "srcid", "step", "subdsid_old", "subfieldid", "tid", "timespan",
+ "treeid", "tree_id", "upperlimit", "userid", "user_id",
+ "viewtype", "width"
+ );
+
+/*
+ * the following fields are variables psased to sql queries
+ * that must meet a certain regex if they are set.
+ *
+ */
+$debsec_regex_vars = array(
+ "active" => "on", "autopadding" => "on", "autoscale" => "on",
+ "currentds" => "on", "export" => "on", "graphsettings" => "on",
+ "grouping" => "on", "hardreturn" => "on",
+ "mustchangepassword" => "on", "showlist" => "on",
+ "showpreview" => "on", "showtree" => "on", "updaterra" => "on",
+ "ip" => "^[0-9.]*$", "hostname" => "^[a-zA-Z0-9.-_]*$",
+ "inputoutput" => "^(in|out)$", "rraid" => "^([0-9]*|all)$"
+ );
+
+/*
+ * the following variables are user-supplied or other forms of
+ * free text, and are passed inside of quotes to sql queries
+ *
+ */
+$debsec_textq_vars = array(
+ "custom", "dataname", "dsname", "dspath", "fullname", "name",
+ "password_to_save", "section", "textformat", "title", "type",
+ "username", "value", "verticallabel", "password", "confirm",
+ "snmp_community", "snmp_ip"
+ );
+
+
+
+/*
+ * this will quit out if any fields are set and not numeric
+ *
+ */
+foreach($debsec_number_vars as $debsec_v){
+ if(isset($_GET[$debsec_v])){
+ input_validate_input_number($_GET[$debsec_v]);
+ }
+ if(isset($_POST[$debsec_v])){
+ input_validate_input_number($_POST[$debsec_v]);
+ }
+ if(isset($_COOKIE[$debsec_v])){
+ input_validate_input_number($_COOKIE[$debsec_v]);
+ }
+};
+
+
+/*
+ * this will quit out if any fields are set and don't match
+ * the given regular expressions
+ *
+ */
+foreach($debsec_regex_vars as $debsec_k => $debsec_re){
+ if(isset($_GET[$debsec_k])){
+ input_validate_input_regex($_GET[$debsec_k], $debsec_re);
+ }
+ if(isset($_POST[$debsec_k])){
+ input_validate_input_regex($_POST[$debsec_k], $debsec_re);
+ }
+ if(isset($_COOKIE[$debsec_k])){
+ input_validate_input_regex($_COOKIE[$debsec_k], $debsec_re);
+ }
+};
+
+/*
+ * this will clean up all of the above listed quoted text fields
+ *
+ */
+foreach($debsec_textq_vars as $debsec_v){
+ if(isset($_GET[$debsec_v])){
+ $_GET[$debsec_v] = ereg_replace('"', '\\"', $_GET[$debsec_v]);
+ }
+ if(isset($_POST[$debsec_v])){
+ $_POST[$debsec_v] = ereg_replace('"', '\\"', $_POST[$debsec_v]);
+ }
+ if(isset($_COOKIE[$debsec_v])){
+ $_COOKIE[$debsec_v] = ereg_replace('"', '\\"',
$_COOKIE[$debsec_v]);
+ }
+ global $$debsec_v;
+ if(isset($$debsec_v)){
+ $$debsec_v = ereg_replace('"', '\\"', $$debsec_v);
+ }
+};
+
+?>
only in patch2:
unchanged:
--- cacti-0.6.7.orig/include/sanitize-functions.php
+++ cacti-0.6.7/include/sanitize-functions.php
@@ -0,0 +1,68 @@
+<?php
+
+/*
+ * backported security-related changes from cacti
+ * by sean finney <[EMAIL PROTECTED]>
+ *
+ * to preserve my own sanity, all sanity checking functions are here, which
+ * is included by the main configuration, which is included by everything.
+ * variables that don't exist will not raise failures, so only in the case
+ * that the input exists and is not what it is supposed to be will there
+ * be an error.
+ */
+
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004 Ian Berry |
+ | |
+ | This program is free software; you can redistribute it and/or |
+ | modify it under the terms of the GNU General Public License |
+ | as published by the Free Software Foundation; either version 2 |
+ | of the License, or (at your option) any later version. |
+ | |
+ | This program is distributed in the hope that it will be useful, |
+ | but WITHOUT ANY WARRANTY; without even the implied warranty of |
+ | MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
+ | GNU General Public License for more details. |
+ +-------------------------------------------------------------------------+
+ | cacti: a php-based graphing solution |
+ +-------------------------------------------------------------------------+
+ | Most of this code has been designed, written and is maintained by |
+ | Ian Berry. See about.php for specific developer credit. Any questions |
+ | or comments regarding this code should be directed to: |
+ | - [EMAIL PROTECTED] |
+ +-------------------------------------------------------------------------+
+ | - raXnet - http://www.raxnet.net/ |
+ +-------------------------------------------------------------------------+
+*/
+
+function input_validate_input_number($value) {
+ if ((!is_numeric($value)) && ($value != "")) {
+ die_html_input_error();
+ }
+}
+
+function input_validate_input_regex($value, $regex) {
+ if ((!ereg($regex, $value)) && ($value != "")) {
+ die_html_input_error();
+ }
+}
+
+function die_html_input_error() {
+ global $config;
+
+ ?>
+ <table width="98%" align="center">
+ <tr>
+ <td>
+ Validation error.
+ </td>
+ </tr>
+ </table>
+ <?php
+
+ include_once("./include/bottom_footer.php");
+ exit;
+}
+
+?>
signature.asc
Description: Digital signature
