On 08/13/2011 12:27 AM, Ansgar Burchardt wrote: > * No parameter binding: all SQL queries are build using string > manipulation; most parameters come directly from $_REQUEST, escaping > (via mysql_real_escape_string) is only done in some places.
No, you are totally discarding /usr/share/dtc/shared/drawlib/templates.php. Escaping with mysql_real_escape_string isn't needed when there's enough input checking. > * No scheme used to make sure only sanitized variables are ever used. > Together with the first point this makes SQL injections very likely. There is. Each input is checked against a regular expression. See how things are built in each forms in /usr/share/dtc/shared/inc/forms, using the dtcListItemsEdit() function, which is object-like manipulation. The issue is that in some few places, it's not using that, and that's what has been caught for the list.php since this code hasn't been re-factored. > * Checking variables and using them often happens at totally different > places (often different files). This makes it even harder to make > sure they are always safe to use. This avoids code duplication which you are criticizing just below. > * Related to the last point, control flow is a mess. Most seems to be > sphagetti code, there is a lot of duplication, many global variables, > etc. I love the "many global variable" one. That's typical from someone that has a quick look, but doesn't see further. Global variable are used in very specific cases. For table names and configuration variables (coming from the "config" table field names). These are there to force declaration of what a function uses. The only place where what you are saying is relevant are whats in shared/vars/global_vars.php, where you see 8 global variables (which is where you found one issue). > * No priviledge separation: everything -- including apache -- runs as > the user "dtc" which also owns config files for apache, bind and > others. This probably makes this user root-equivalent. But the latest Git version uses sbox to jail each customer in a chroot (running on a union filesystem using aufs), making it quite hard to be harmful. >> #611680 isn't relevant and has no impact, as written on the BTS. > > It has some impact as it makes the code more brittle: when a small > breach is found, it is easy to escalate it higher. Fixing it wouldn't change the issue. If you have access to the central MySQL database, it's basically finished anyway, since you have credentials to access remote dom0. If you have suggestions on how to solve that, let me know, but I don't think there is a solution. >> #566654 I believe, isn't more dangerous than the /etc/mysql/debian.cnf. >> So unless /etc/mysql/debian.cnf is removed from Debian, fixing #566654 >> is useless, IMHO. > > dtc is accessible remotely, the system account for mysql should only be > accessible locally. This means read access to files (or backups) gives > remote access; to make use of the mysql system account, you would > already need to have some kind of local access. No, there's phpmyadmin anyway. > [ About #637477 and #637487: ] >> I have already fixed what Ansgar reported, and I am currently working >> on other fixes. [...] > > These patches[1] look more like a bandaid than a proper solution. I hope > you do not see them as fixed :/ > > [1] > <http://git.gplhost.com/gitweb/?p=dtc.git;a=commitdiff;h=01c4eea40736b89cb396b0596f2808c8f64b8729> Right, I intend to fix it better, but that's a quick fix before I have time for a better solution. Thomas -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
