Mad Irish . net

Objects Gone Wild (in a bad way)

31 March 2010

Recently while performing an application security audit I had the misfortune of discovering probably one of the worst uses of OO I've ever seen. Object oriented (OO) programming is a powerful tool that can be used to abstract data conceptually into modular, easy to utilize (and maintain) components. There are a few general tenants of OO that lend use to object oriented design. One is that objects should be self contained. Objects should represent something, rather than serving as a container for functions. Another tenant of OO is that objects should be self contained, that is they should be sufficient on their own. You don't want to have an object that depends on all sorts of other functionality outside of itself. Object oriented languages generally provide inheritance so that you can create child, or derivative objects (such as a cat object inheriting from the animal object), but this should be the main dependency.

Unfortunately many function based programmers transition to object oriented programming without understanding the real intents and purposes of OO. Often times these programmers simply use objects as portable containers for functions, recoding their applications to use object methods instead of functions or subroutines.

To demonstrate how horribly wrong this could go I propose the following snippet of code that processes form posts to set up configuration variables for a web application: 

$obj = new CConfig();

foreach ($_POST['w2Pcfg'] as $name => $value) {
	$obj->config_name = $name;
	$obj->config_value = $value;
	$msg = $obj->store();
}

Examining this code it becomes clear that first a new instance of the CConfig object is created, then the code loops over the POST form array, assigning name value pairs to attributes in the instance, and calling a method store(). Looking at the CConfig class it becomes clear how horribly wrong this all is: 

class CConfig extends CW2pObject {
	public function CConfig() {
    		parent::__construct('config', 'config_id');
	}
}

OK, so the CConfig class extends the CW2pObject class, and does what? Well, it creates an instance of it's parent and then... That's it actually. The store() method is in the parent object as: 

public function store($updateNulls = false) {
	$k = $this->_tbl_key;
	if ($this->$k) {
		$store_type = 'update';
		$q = new DBQuery;
		$ret = $q->updateObject($this->_tbl, $this, $this->_tbl_key, $updateNulls);
		$q->clear();
	} else {
		$store_type = 'add';
		$q = new DBQuery;
		$ret = $q->insertObject($this->_tbl, $this, $this->_tbl_key);
		$q->clear();
	}
}

Without dwelling too much on the specifics, what this method does is that it performs a database insert based on its _tbl attribute value.

So, to summarize, we're instantiating an object from a class, which is then instantiating an instance of its parent. Next we're assigning undocumented, dynamic attributes to this class, and finally saving them to a database. The ONLY dynamic thing about this entire operation - the only thing that all this wretched code supports, is a dynamic INSERT statement! Why in the name of all that is holy would anyone think this was a good idea?!? Anyone going to look at any of the objects will quickly discover that none of the attributes being set in any of these classes are defined within the classes themselves. Furthermore, these classes are a total waste. There's absolutely no need for them. Sure, having these classes makes the code object oriented, but at no gain (unless perhaps the only purpose was marketing).

Looking at this code it's not even clear if refactoring it is worth the effort. Basically the code would be better served by rewriting it. This is a horrible discovery to make, especially because the only reason I discovered the code is that it fails to do any input sanitization. If a developer wanted to add some code to protect against injection there isn't even a clear place to do that. No variable is defined, and no type is enforced. The code basically DEGRADES the functionality of PHP and MySQL, making them more insecure and difficult to manage.

I hate to name names, but this code comes out of Web2Project, which inherited it from dotProject. I'm tempted to try and fix it, but the code base is such a morass that it really begs an entire rewrite. While I'd like to make the effort and contribute to the project, there comes a time when writing the whole system from scratch actually seems more appealing, and I think I'm at that point.

Tags