5 ms·

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 payloa

by orf 10y ago

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 http://i.imgur.com/hC9KmWh.png

blowski 10y ago
This one: https://support.sugarcrm.com/Documentation/Sugar_Versions/7.7/Ent/Administration_Guide/System/Repair/index.html#Remove_XSS https://support.sugarcrm.com/Documentation/Sugar_Versions/7....
JumpCrisscross 10y ago
I'm a numpty. Can you explain why this is bad?
- code_duck 10y ago
  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.
  - mgkimsal 10y ago
    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.
    - code_duck 10y ago
      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.
  - koolba 10y ago
    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!
    - code_duck 10y ago
      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.
      - koolba 10y ago
        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.
  - dfox 10y ago
    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]
- lucideer 10y ago
  (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
  - geekamongus 10y ago
    Is the best practice in (c) not just escaping output, but also filtering input?
    - lucideer 10y ago
      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).
      - jerf 10y ago
        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.)
- stinky613 10y ago
  The sibling comments here give good details, but here's a 50,000ft view: https://xkcd.com/463/ 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."
arethuza 10y ago
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.
ReligiousFlames 10y ago
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.)

by orf 10y ago

blowski 10y ago

This one: https://support.sugarcrm.com/Documentation/Sugar_Versions/7.7/Ent/Administration_Guide/System/Repair/index.html#Remove_XSS https://support.sugarcrm.com/Documentation/Sugar_Versions/7....

JumpCrisscross 10y ago

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

code_duck 10y ago
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.
- mgkimsal 10y ago
  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.
  - code_duck 10y ago
    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.
- koolba 10y ago
  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!
  - code_duck 10y ago
    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.
    - koolba 10y ago
      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.
- dfox 10y ago
  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]
lucideer 10y ago
(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
- geekamongus 10y ago
  Is the best practice in (c) not just escaping output, but also filtering input?
  - lucideer 10y ago
    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).
    - jerf 10y ago
      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.)
stinky613 10y ago
The sibling comments here give good details, but here's a 50,000ft view: https://xkcd.com/463/ 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."

arethuza 10y ago

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.

ReligiousFlames 10y ago

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.)