Quoting Jesse Becker <[EMAIL PROTECTED]>: > On Feb 11, 2008 6:46 PM, Bernard Li <[EMAIL PROTECTED]> wrote: >> Hi Alex: >> BTW, I am going to check in the patches for trunk, however, I will >> rename clean_float() to clean_number() since the function name and the >> comment seems a bit misleading based on its call to is_numeric (i.e. >> it is not really checking whether it is a valid float, just a valid >> number). > > In the meantime, I started on a patch to put all of the variable > checks and sanitation in one place.
Do you mean move the checks out of get_context.php and graph.php? > The various clean_* checks are > not directly part of it, and could be removed if not needed. This > brings up the question: are we "checking" or "cleaning?" Good point. clean_string() will actually change data, escaping unwanted HTML which might be present (cleaning). clean_number() will return NULL if the input value is not numeric (checking). > > For example, if the query string has the key/value pair "st=ABC123" > what do we want to do? The "st" variable is for the graph start time, > in epoch seconds, so it should always be an integer. Do we want the > validation routines to warn/fail because it isn't of the desired type, > or should it try to "clean" the data, and return what it can (in this > case, "123")? The former is more strict, and less likely to cause > strange problems due to malformed data (a graph from 123 seconds after > the epoch to "now" probably isn't what you want). I agree the 'check' approach is less error prone. If input seems to be malformed, it's a bad idea to try to guess what the user intended. Perhaps we ought to add some logic after the input validations whereby if any validations failed, the script exits with an error message. I didn't want to make that far-reaching of a change when adding the initial set of input validations, but it's a more secure way to proceed long-term. Right now, the input validations make a best attempt to get non-harmful input from the user, and then proceed with it. This will prevent cross-site scripting, but may sometimes lead to invalid HTML (if the user sends a mangled query, they may get broken HTML as a result). Other thoughts on validations and filtering: There are some input values (like $sort) that currently have escapeshellcmd() run on them, but are never used in a shell command. We could save a bit by only running escapeshellcmd() when it's actually needed, just before the shell call to rrdtool. For most of these, escapeshellarg() would be more appropriate, since they are arguments not actual commands. Putting all user-supplied input into some type of container, like an array called $user, would add clarity to the code. Wherever this input is used, it's obvious where the value originally came from. We'd need to decide if this stores the raw input, a filtered/validated value, or both. I'd favor doing the same thing for all values set in conf.php: put them in a $conf array. When reading through a script, this makes it much easier to identify the original source of whatever value is being used. alex ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/ _______________________________________________ Ganglia-developers mailing list Ganglia-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ganglia-developers
