SQL Injection/Overwrite

[sunside]

Hi,

thanks to SugarCRM Inc.'s not employing basic programming rules, yesterday all custom fields (approx. 100 fields) in all of our opportunities got erased, leaving us with a backup from a week ago, losing some critically important data.

How?

Just try entering something like 12#3 into a custom field of type INT. But don't try this on a production system! (Unless you want to annoy your employer and get fired.)

Unfortunately, someone thought it was a good idea to enter such a number into a custom field of type int. Well, usually in an application employing sound programming practice, this is not a problem at all. But together with PHP's ignorance about variable types and Sugar's carelessness, this made up to a very serious problem for us.

What does it do?

When making up the SQL query to update the opportunities_cstm table, Sugar obviously doesn't sanitize the inputs. This is really easy to do, and there are ready-made libraries such as this to do the job. Because the # sign indicates a comment in MySQL, it ends the query right there. Instead of executing
UPDATE opportunities_cstm SET field1='bla', field2=2, field3=..., int_field=123 WHERE id_c='sugar_opp_id_here'
Sugar will only execute
UPDATE opportunities_cstm SET field1='bla', field2=2, field3=..., int_field=12 # Here goes the comment
which will overwrite ALL values in ALL opportunities with the values of the current opportunity!

We were extra lucky that this was the LAST field which I had added MOST RECENTLY, so really ALL values are lost. I would suppose that we had lost fewer data if this was somewhere to the middle of the SQL query. I didn't do any tests though since I am really sick of this piece of software at the moment. I haven't tested if this can be exploited by web users, e.g. by inserting 12#3 into a web form field that is then passed to sugar e.g. in a SOAP request and is then used to update some custom fields (can be in a contact, lead or anywhere else I guess). In this case I must add that you are responsible for sanitizing user input yourself before building this SOAP request and sending it off to Sugar (rule 7 of the coding practices cited below), but Sugar must also do some checking on what it gets in a SOAP request, because it cannot be regarded as safe.

This is generally known as "SQL Injection" and can be countered by using principle #1 of secure programming practice and rule 1 from web application programming 101:
Never trust user input.
See the CERT Top 10 Secure Coding Practices for example.

I am really annoyed to see 50+ people working on a software for years and not even consider these basic rules. It doesn't make me overly confident about the security of the rest of the code, if one of our users can inadvertently and by accident delete all our custom fields.

To the Sugar folks: I would really appreciate it if you could do your homework first before building in any more bells and whistles.

Users: beware!

[pentolino]

Does this really big bug as been fixed??

[sugarchris]

Thanks for bringing this to our attention. This bug is being addressed today and will be availble in the subsequent patch.

RE: the poll
Maybe that should be "Is any web application 100% secure?" Sugar addresses every security vulnerability we find and reported to us as quickly as possible. It's definitely not in our interest to ignore it.

[sugarchris]

In further invesitating this bug, I've found that the mechanism that escapes the comment, "#" char, is overridden when the field is of type "integer". This will definitely be addressed in the next patch.

[majed]

The first thing I would like to mention is that this had been previously blocked within the SugarCRM application, and we are working diligently to ensure that the application stays secure. Secondly here are two fixes you can apply to your system to ensure that this doesn't happen again. One on the javascript level and one on the back end level. Apply one or both if you would like.

In sugar_3,js

replace the unformatNumber function with this

// format and unformat numbers
function unformatNumber(n, num_grp_sep, dec_sep) {
	if(typeof num_grp_sep == 'undefined' || typeof dec_sep == 'undefined') return n;
	n = n.toString();
	if(n.length > 0) {
		n = n.replace(new RegExp(RegExp.escape(num_grp_sep), 'g'), '').replace(new RegExp(RegExp.escape(dec_sep)), '.');		
		return n;
	}
	return '';
}

For the backend fix in modules/Currencies/Currency.php replace function unformat_number($string) with

function unformat_number($string) {
	static $currency = null;
	if(!isset($currency)) {
		global $current_user;
		$currency = new Currency();
		if($current_user->getPreference('currency')) $currency->retrieve($current_user->getPreference('currency'));
		else $currency->retrieve('-99'); // use default if none set
	}
	
	$seps = get_number_seperators();
	// remove num_grp_sep and replace decimal seperater with decimal
	$string = trim(str_replace(array($seps[0], $seps[1], $currency->symbol), array('', '.', ''), $string));
	 preg_match('/[0-9\.]*/', $string, $string);
	return trim($string[0]);
}

As always try it on a dev or test environment before deploying, and if you have any issues with this let me know and I will address them as soon as possible.