Wallet Manager Security Issues
Posted: 2011-08-26 Filed under: industry | Tags: database, injection, security, sql 8 CommentsIf you have been following this blog for a while, you know that my corpmate James and I have been working on a Wallet Manager site to help manage our Eve ventures. Over time it has grown into our all-encompassing-project-management-thing which now has a trading, manufacturing, invention, and cost analysis sections.
I wanted to disclose why this darn thing is not open to the public as the majority of the feedback that I have been hearing has been, “awesome, now when can I use it!?”
We have not made the site public because of security issues, specifically due to the numerous SQL injection abilities in our code.
Here is a common function that we use that takes the typeID of an item and returns its name. We use this so when we display a Cap Recharger II for example, you can see the name of the item and not just the ‘2032’ number identifier that is easier to work with from a programmability standpoint.
This PHP function retrieves the item name from an input of its typeID.
public function getName($typeID)
{
$sql = ‘SELECT typeName FROM invTypes WHERE typeID = ‘.$typeID.’‘;
$connection=Yii::app()->db;
$command=$connection->createCommand($sql);//Run the query
$results = $command->query();
$itemName = $results->read();return $itemName[‘typeName’];
}
The database query is highlighted in green and the terrible part has been highlighted in red.
What you are seeing is a database query that is fed a non-sanitized input. Good programmers will take the $typeID variable and sanitize it before putting it into the SQL query. A common check is to limit the variable to only have characters such as A-Z and 1-3 characters. This check will not allow any special characters such as : ; ‘ ” $ that are used for SQL operations to be allowed in the query.
With our current function with the unsanitized input variable, you can plug in all sort of things into the query. You could inject code in place of the variable to read, drop, and modify the database, something we obviously don’t want happening.
Sadly around half of our function were written in this fashion in order to get the pages up and working. Because it has been an internal project, the focus has been on the aesthetic result and not the security of the code behind it. If we were to release it to the public we would have to go over each function and check to make sure that it is secure.
Let me quote CCP and say Soon(tm) for the release.
k162space 
As for why this is the case and to quell one avenue of comments…
A lot of this code was written expressly for us. Only more recently have we started to think on a more global level about the functional design and public readiness of the project. Many of the tedious aspect of the programming remain un-tended-to, such as the administrative section. Much of it is “it just works.”
For example, obviously the correct Yii way to do the above would be:
We are in a state of technical debt, having reached well in to the hundreds of thousands of lines of code. With only Blake and myself working on it, it will certainly be a fairly large undertaking to convert some of the late-night-paint-on-the-wall type stuff to proper code. I think it is possible, but whether or not I have the time and patience is probably another story.
-JB
1. Then just think about making the source public, i.e. convert it to be an open source project. I think at this stage of development odds are good that you will find a few developer willing to help.
2. If you don’t want to make that step you can also think about getting another few developers just helping with a few features or these security measures in particular.
3. A third solution might be to start publishing the application with limited features. Create a road map with releases sponsoring reasonable feature sets and review these code parts at that point. Releasing gradually is key with this one.
Also, php is type aware. You can cast the input to int in this very example (typeID).
Don’t make it public 😛
sadly if every one uses it well, less profit for every one
secondly theres a number of automated tools that check for sql injection , other know overflows
and even cross site scripting,
lastly sql perms to not allow modification of data, IE READONLY perms, can go along way to protecting hardedning anything you have forgotten.
di55y, Bede,
Methods and knowledge of proper practices regarding SQL permissions as well as binding params has not really been the issue so much as shotgunning quick ideas into the system and then not circling back around to refactor them properly.
Yii’s data model classes automatically bind params to SQL queries, so injection and typing problems do not arise if we stick with the approved way of doing things. It’s when we haven’t that we have our vulnerabilities. There are areas in the code where deleting rows is quite necessary to keep the cruft down, so read-only permissions cannot really be applied here.
di55y, I have thought fairly seriously about the third solution. I think if I did that I would like to start fresh and migrate back in some number of features per release, so each part of the code would be looked at again.
-JB
Oh how well I understand how you got to this place. All I can say is that I hope you find something that will make the effort worthwhile. For what it’s worth, I’ll confirm again that from what I’ve seen, this app would be worth a paid subscription.
Thats why i lub asp.net
And i use paramater based sql statements,
and ulitmitly hard coded types. ie no funny “‘” business when the system expects an integer.
Infact id check types on anything that touches databases these days..
I had pondered a carefully crafted eve char name & or contract…
http://www.eveonline.com/devblog.asp?a=blog&nbid=2357
FYI they just dumped 6 million lines of aggregated market data history.. I only just finished intergrating a number of data sources for my lil thing, now im adding eve data stuff too…
[…] are many, many non-sanitized input points that are major security issues as noted in this post. I would not use this as a public-facing […]