Tales of SugarCRM Security Horrors | Hacker News Reader

Tales of SugarCRM Security Horrors

karmainsecurity.com

235 pointspeter_tonoli78 comments

The SugarCRM administration panel has a button labeled "remove XSS". We have a picture of it up in our office.

Yes. A button that attempts to remove XSS payloads from the database that admins can click. That's the level of security competence we are talking about here.

Edit: Here is the button: http://i.imgur.com/hC9KmWh.png

This one:

https://support.sugarcrm.com/Documentation/Sugar_Versions/7....

SugarCRM is an example of projects that give PHP a bad rep.

Maybe programming languages should have a minimum engineering and architectural design competence test such that required knowledge is assured before releasing craptacular poo upon the world.

(If you're write a SugarCRM sploit, obviously patch the Remove XSS to skip yourself.)

I remember working with another CRM product that had a setting in its configuration file called "secure" that was set to false by default!

Basically this controlled whether authentication was always applied or not - by default it was kind of optional.

I'm a numpty. Can you explain why this is bad?

(a) It's clearly such a persistent problem that they explicitly added such a button.

(b) If* the button really performs as advertised, why on earth should it be a manual step? Do it automatically.

(c) Best practice here is to escape-on-output, negating the need to remove in-DB XSS, so the fact that this button exists strongly implies they aren't doing that

* I would guess that the button is very unlikely to perform comprehensively as advertised

From their website:

>Select a module to remove potential XSS strings

My reading of this is SugarCRM, which ships barebones like Drupal, has a whole ecosystem of poorly coded third-party modules you need even for basic CRM functionality. The core Sugar devs wrote this little script to run through module code and its db entries to find potential XSS issues. I imagine that this would break things so its a 'admin beware' type of thing they shouldn't automate. No one wants to upgrade to Sugar 5.2 from 5.1 to find half their modules broken, so its manual. Maybe in the future it won't be and its a shot across the bow to third-party developers to take security seriously.

That's the problem with these popular bare-bones FOSS frameworks. You get a decent core product like Drupal or even Wordpress, but you need a dozen or so modules to make it do anything useful. The code quality on random modules, is of course, random as well, so the core devs try to work around it. Its ugly, but more an indictment against module authors than the core SugarCRM team. That team is just trying to fix the shit-code in the modules and protect the reputation of their product. They don't want to find themselves in the situation Microsoft often found itself in, where Flash and Java exploits were blamed on Windows as non-tech savvy people saw their Windows hacked and didn't know it was a third-party program responsible for it.

This is also why we don't often implement systems like these at work. While I may have trust in the Drupal or Sugar team, I'm ultimately left to be forced to trust sole module authors like 'webdev420hacker' and 'boutiquewebsites1997' because they're the only game in town for these modules. I think bare-bone frameworks are falling out of style a bit. Go with a manged cloud product or go with a more featureful product that isn't dependent on 3rd party modules so much, or go with a commercial solution with everything instead. There's too much variability with third-party modules and not remotely enough security conscious eyes on this code.

I think we could address all of your issues if only we could design it such that something clicked the button as often as possible.

cron, maybe?

Perhaps SugarCRM should create a new "Secure Enterprise Edition" of the product. The fees would be substantially higher, of course, with the proceeds used to hire overseas IT shops whose 24/7 staff would remained logged in, clicking the button repeatedly. To mitigate the privacy concerns, SugarCRM could create a special version of that administrative page with nothing except the "Remove XSS" button.

or process any data in that way while it is added or read. that avoids scanning the whole table, too.

Is the best practice in (c) not just escaping output, but also filtering input?

Nope.

Input validation is good practice (checking that the input is what's expected), but input filtering is problematic for two reasons:

1. Since input filtering lacks context, the usual option is to filter very broadly (e.g. to attempt to filter for SQL injection and various forms of XSS simultaneously). This leads to much more complicated filtering strategies, adding maintenance overhead, risking data corruption, and generally increasing technical debt.

2. Data loss. This is less of an immediate security issue, but full data retention can be useful for debugging, for future migrations or data transformations, &c. It also ensures you don't get data corruption (e.g. where filtering for one context breaks your data in a different context).

There's another less appreciated one, I think, which is that input filtering often has the problem that the "sensitive" values that you might be trying to filter out are also very often perfectly valid values as well.

For instance, the apostrophe character is a very potent character in a number of injection scenarios and one you might be tempted to "filter", but it is also a perfectly legal character in things as common as "names". It is merely one example of a very large and constantly growing set.

You can't help but correctly encode things on the way out if you want things to work properly.

There is also the question of "filtering" vs. "rejecting". I personally recommend that one way or another anything with the first 32 ASCII characters that you don't expect not end up in your database, because they are full of magical behaviors in all kinds of places, but I also tend to recommend outright rejection on the grounds that these things don't innocently come in. Nobody accidentally types the Negative ACK character into their name. But at the very least, filter it out early. You can also outright "filter" on Unicode character classes you don't expect. But this really ought to be seen more as mere day-to-day business "data validation" than a security measure because of the aforementioned fact that some of the Characters of Interest are still valid, and you can't afford to just filter them all out.

(You basically end up with "English letters and numbers". If you're trying to "filter" away all the "bad" characters in advance, without really knowing where they're going, you can't even have things like "space" (very active shell character), and UTF8 can actually be dangerous if stuff isn't expecting it, etc. And when push really comes to shove, even strings of nothing but English letters and numbers can become dangerous if they are too long, in certain pathological contexts, i.e., "seriously, don't write network software in C". Because the safety of a string is not an intrinsic property of a string but has everything to do with interpretation by further bits of code, there isn't a way to generically "cleanse" a string.)

I've also come across scenarios where allowing the user to enter valid HTML was a requirement. Especially in cases where users will be entering HTML that renders as part of a site, it was much easier to treat all user input as potentially unsafe and escape output in all cases, with the exception of the one or two places where user-created HTML was supposed to be rendered and/or sent to an external API.

I run into web applications all the time where lazy and incompetent developers have blocked or filtered out the '<' and '>' characters in a naive attempt to prevent content injection attacks.

And another: Fixes/Changes.

If you rely on input filtering but you miss something, there's a bug and filtering doesn't work, or there's a new type of text that needs to be filtered (eg: a new tag added that didn't exist at the time), you have no recourse -- that text is already in the database.

A software update can fix/change the output filtering, and since that runs at display time (when the vulnerability is actually activated), it can address it.

I think context sensitive filtering is the logical choice at all stages of the operation. Input coming in from a web form should pass through two filters. One context sensitive filter for the form data (Were the required fields filled out, and ideally do they look like the expected data in those fields?) and then again when inserting to the SQL database, to protect against SQL injection.

Then, when the data itself is read, it should pass through context-sensitive output filters, one for the template engine, once again if it's going to be embedded in html, or javascript, or a stylesheet, or into a URL. Output filtering needs to happen, but it critically should not be attempted at the time of input, as the developer of the form should not be expected to predict any possible usage for that data once gathered.

The broad-strokes filtering you describe in step 1 is an anti-pattern through and through. :) I'd avoid personally any codebase or framework attempting this strategy, as I've seen it fail too many times.

If hackernews filtered input, I wouldn't be able to say that the <script> tag can be used in XSS attacks.

Can you think of a case where it would be helpful to store XSS in the database until removed by clicking this button?

What happens if we view the admin panel and trigger one of these malicious scripts, because I haven't clicked this button recently enough? Oh, maybe someone should click it very frequently!

Or... Hmm, maybe rather than storing data that may be mass deleted by a script, with no review, after possibly compromising our security, we should just reject it in the first place and log a security incident for that user.

In many cases XSS and similar vulnerabilities (SQLi and even some buffer overflows) are also and primarily functional problem as some classes of valid input cause some kind of breakage. Correctly escaping on output solves both the security and functional problem, filtering on input usually aggravates the functional issue even more.

On the other hand in context of CRMs, helpdesks and other things that have to process received emails and then show them in web interface this gets hairy because escaping on output while correctly handling variously weird or outright broken input is non-trivial. For example every helpdesk solution I've seen gets confused by some combination of nested MIME (ie. email with attachments forwarded with additional attachments) and notionally non-text email bodies (ie. bounces from Postfix).

[Edit: and in this case storing whatever was on input without any transformations and sanitisations enables you to recover the content WHEN (not if) the breakage occurs, by either manually examining the input or, in the email case, importing it into desktop MUA. I even had once to do this with message that was unreadable in otherwise surprisingly well-behaving gmail's web interface]

my view would be 'escape on output' is always better, regardless. there may be reasons I'd want to keep it in there - for analysis, to prove someone was trying to be malicious, or perhaps to demonstrate there's actually a real problem getting past some input mechanisms.

However, the 'defender' in me can think of a situation where this would be good:

Input filtering mechanisms have improved to deal with new attacks/problems, but old data is not 'new input', so you'd apply the updated algorithms to existing data.

I guess my thinking is that should be part of the upgrade or update process - whenever antiXSS rules are changed, they are applied at that time. It's having this button there as if it's something you'd run for periodic maintenance that seems insane.

I'd definitely save input that breaks the rules when received, too - but maybe in a separate table.

XSS is a malicious version of the more general problem of not escaping user inputs. its much easier to escape on output rather than input as you can't really tell what's "acceptable". It's also more robust as out of band updates (say direct database updates of comments) get outputted correctly too.

If I have a comment forum with input filtering (rather than output escapong) I can't restrict it to just letters numbers and punctuation or the user's can't discuss HTML tags!

That makes sense. The objection I feel here is the inconsistency - why accept the input in the first place, only to delete it with this button? An above comment suggests rules may be updated over time, which could explain their thinking.

Because they don't properly escape things everywhere (anywhere?) and the DB update button is a kludge to deal with existing messed up data breaking pages they don't know about.

It was probably less than an hours work to add that button vs. hundreds of hours to track down all the unescaped outputs.

The sibling comments here give good details, but here's a 50,000ft view: https://xkcd.com/463/

The comic compares running McAfee Antivirus on a voting machine to a teacher wearing a condom to work every day; "Strictly speaking, it's better than the alternative--yet someone clearing is doing their job horribly wrong."

> there are still chances for both authenticated (CVE-2012-0694) and unauthenticated (KIS-2016-07) attackers to exploit PHP internal memory corruption vulnerabilities which do not require objects declarations, like this ten years old vulnerability which requires just an array definition or this one which relies on references and arrays declarations

The two bugs linked were both fixed around 10 years ago. If you're running PHP v4.4, your problems are basically infinite. It would be nice to make a clearer distinction between PHP problems and SugarCRM problems.

I said "like this 10 years old vulnerability", which means there could be other 0-day vulnerabilities affecting even the latest PHP versions and that could be exploited without relying on objects declarations within the serialized string.

Using the unserialize() PHP function itself is not security issue, unless you use it with user-supplied input, and that's the reason why this is a SugarCRM problem and not a PHP problem.

It's a great post, by the way, no criticisms of that. But I interpreted that specific bit as "this core PHP bug was reported 10 years ago and still hasn't been fixed", so makes it sound like a huge and ongoing PHP vulnerability.

How timely; I recently evaluated sugarcrm/suitecrm but similar to author was dismayed by their code quality.

Does anyone have any recommendations of other open source CRMs?

I won't consider SuiteCRM to be part of this. Understand that even though SuiteCRM was a fork of SugarCRM CE, the community has made so many changes that I will consider it at this point a completely different product. All the vulnerabilities listed above do not apply to SuiteCRM in its current release and if you look at the blog of SuiteCRM, it is heavily advertised that SugarCRM CE is full of security issues and everyone should migrate out of it[1]. Here is the quote: “SugarCRM Community Edition users need to migrate to an alternative platform as soon as possible. The number of current vulnerabilities in Community Edition is worrying. There will be no more support for Community Edition after April 2017 and the vulnerabilities will increase as the software ages. Simply put, if you're running SugarCRM Community Edition, you're becoming a soft target.”

https://suitecrm.com/index.php?option=com_easyblog&view=entr...

FWIW SuiteCRM has the exact same "remove XSS" button in the admin panel that Sugar has - doesn't exactly scream "we're confident in our security"

So suitecrm want people to migrate to their product and claim to have superior security? That's hardly surprising.

Maybe you should have a look at OroCRM and OroPlatform (https://www.orocrm.com, https://www.orocrm.com/oro-platform). It is built on Symfony and seems rather new, maybe not feature complete.

Based on OroCRM's comparison chart their community edition is effectively a non-feature complete version of their commercial product, i.e. same scenario that happened to Sugar?

https://www.orocrm.com/orocrm-enterprise-and-community

Odoo (formely OpenERM) is rather omnipresent in this field.

For non-profits/associations, check out CiviCRM.org.

Code on Github: https://github.com/odoo/odoo

>(formely OpenERM)

You meant "OpenERP"

indeed, thanks :)

Wow, this kept getting better and better, I didn't expect to make it through the entire text. Some parts are shocking.

Whoa! That's horrifying. Not only don't they update their open source version when fixing security bugs (Great argument against choosing open core solutions btw), they don't even fix most bugs!

This isn't a great argument against chosing open core solutions, it's a great argument against chosing SugarCRM.

Many open core solutions (I'm thinking of gitlab, but there are probably many other) fixes bugs in both versions.

It is. If there are two versions, it's possible (it even "adds value") to fix bugs in one version and not the other. If there is only one, you can't do that. Simple as that.

It makes the company (look at sugarcrm) look bad when they do that.

Open core can be done well, just like proprietary software can be done wrong.

A few years ago, my team and I tried building a small CRM solution based on SugarCRM; we figured, "hey, it's basically a simple CRUD app with some reports and somewhat-dynamic objects, right"?

We gave up after a week (ended up building the thing in Django). vTiger/SugarCRM is most likely the worst PHP codebase still in active development/production.

The core issue is that sugar crm uses PHP built in `unserialize` on user controlled input, and they don't want to switch to json ostensibly because of performance issues.

Why don't they hmac the payloads (with a timestamp and something tied to the user (an ID, the username, whatever)) and verify the hmac before deserializing?Verifying an hmac prevents undetected tampering, is fast, and there are libraries for it in ~every language.

The performance part doesn't make sense to me either. What are they serializing in a CRM that is so time-critical? I found a benchmark [0] that shows differences of like .2 milliseconds for a 904 byte array.

[0] http://techblog.procurios.nl/k/n618/news/view/34972/14863/ca...

I wonder what's the use case for serializing and unserializing objects using php built-in functions. Is this some kind of "I'm too lazy to json encode a subset of properties" or are there some edge cases where one would use this extremely sharp knife?

He mentions that they think it might have a negative effect on performance. But more probably they just don't want to refactor their spaghetti code to properly handle serialized data, so instead they go for quick fixes.

Sharp knives are the least dangerous if you're cutting something.

A sharp knife will just cut the thing.

A dull/less sharp knife will not cut, causing you to add more pressure, causing the knife to slip, which is when you're more likely to cause yourself harm.

Good point.

There are cases in which one might use it, but no cases in which one should.

If you have completely sanitized data what is the problem with object serialization? Thank you.

I think the problem lies somewhere inside "completely sanitized data"."If you have them completely sanitized it usually is not what many would call object serialization (php unserialize) but rather a data format (JSON).

The only reliable way to sanitize PHP-serialized data is to unserialize it, scrub it, and reserialize it. This poses a nigh intractable chicken-egg problem, and makes a switch to JSON by far a more economical option.

json_encode didn't exist when sugarcrm was initially developed. whether they should have changed it later or not is a different question, but there was nothing else to go with at the start.

I've seen other projects like Doctrine2 ORM serialize arrays using the built-in serialize function too. I guess/hope they sanitize input beforehand.

You need to run a WAF like modsecurity in front of any PHP application these days.

The good thing is that Sugar is slowly but steadily replacing the old codebase but they should be more transparent on addressing these serious issues.

Yes. For example, SugarCRM is adding prepared statement support which mitigates any potential SQL injection problems. I mention this since it was posted just this last week. These changes are very large and far reaching so it does take time.

https://developer.sugarcrm.com/2017/04/17/use-of-prepared-st...

Prepared statements were considered the best practice for over a decade now. I know these things take time but the fact that this is just happening now is actually more cause for alarm, not less.

Great stuff again Matthew :), your blogs are always easy to read and helpful! Personally I think that string concatenation in query building throughout the Sugar code base (campaigns, workflows) is very problematic and could also be exposed in a couple of scenarios. But like I said, this seems to be the work in progress currently at Sugar.

Hopefully Sugar will come forward with a response to these allegations because these are serious security risks.

Thanks. Yes, I am hopeful that a response will be forthcoming.

> mitigates any potential SQL injection problems

Not quite, string concat for defining queries is still plenty vulnerable regardless of PDO.

it mitigates quite some SQL injection possibilities, but yes, string concat while building queries still remains an issue.

Other CRM providers would probably deserve a closer look too.

Marketcircle for example has never been able to offer reliable SSL support for its CalDAV / CardDAV server. And they are switching to a cloud solution too – closed source and proprietary …

SugarCRM stopped public development long ago. Most people use Sugar non-CE or SuiteCRM (a maintained fork) which probably has similar/same vulns.

response to the blog: https://blog.sugarcrm.com/2017/04/24/important-security-upda...

Their latest update (4/25/17):

Based on impact and reach, none of the vulnerabilities in in the second section scored higher than 'medium'... It’s important to note that updates based on issues scored as ‘medium’ are no longer provided to our last-generation open source Community Edition (CE), so the bloggers post no longer aligns with our current commercial products and solutions.

So, I just replied to their blog post with this comment (awaiting moderation, so will probably never be published):

Hi Rich, I'd like to know more about your latest update, the one with regards to the second section of my post: you're saying you rated all of the vulnerabilities I reported with a "medium" CVSS score (which version btw? v3.0?). However, I reported two SQL injection vulnerabilities and according to your security advisories (https://www.sugarcrm.com/security/sugarcrm-sa-2016-003 and https://www.sugarcrm.com/security/sugarcrm-sa-2017-001), in the past you rated SQL injection vulnerabilities in SugarCRM with 'High' or 'Important' risk level... May I know why now they're considered of 'Medium' risk level? They same applies to the remaining vulnerabilities, which might allow a malicious user to execute arbitrary PHP code, and so far in your security advisories this kind of issues has been rated with 'Critical' risk level (like this: https://www.sugarcrm.com/security/sugarcrm-sa-2016-001)... The numbers don't add up!

Can someone recommend a CRM? Preferably open source and free?

Currently we are considering odoo but any advice appreciated. https://comparisons.financesonline.com/sugar-crm-vs-odoo

The page is unreadable on mobile :(

Looks very good in Reader mode on Firefox for Android but the bare layout is indeed not mobile friendly.

Android Firefox readermode is fantastic. I absolutely love it. I wish it was the default view for most pages with a toggle to see the original!

Reader mode is by far my favourite Firefox feature!

It is overwhelming how sweet is to read focused on the content instead on closing the next-to-scam popups that decides to open up on a random part of the article.

One interesting side effect of reader mode is that as all content has the same format, you get used very fast to compare content not based on external artifacts (font, colors, page layout), but in the actual message.

I had never used reader mode on FF, but just gave it a whirl for this atricle and really like it. Thanks for the tip!

It was fine for me on an iPhone SE, after switching to landscape orientation.

It's fine in Safari reader mode.

you'll hit hi res native and no mobile version sites from time to time as others have said go landscape and pinch out to zoom to text block.

Reply on news.ycombinator.com