In the current SQL wrappers, when an error condition happens, it assume that the request is being made in a “standard browser” request so it echos the error as HTML and returns “successful” headers.
So when an error condition happens during an AJAX request, it’s not as clear to developers or end users that something went wrong. It currently only shows up if you inspect the request in “web console” and know which request to inspect. (Since it returns a successful error code still).
The example I’m seeing is that the new execute_background_services call fails on my current install (understandable, since I haven’t run the upgrade scripts). However, it fails more or less silently.
Down the road, we should consider more sophisticated error handling options, so AJAX errors due to database calls can be more “gracefully” handled, or so that transactions can be rolled back.
Should we even be sending the actual error message to the screen? Wouldn’t it be best to just state there was a sql error and to look at the error log for more details (with the same security reasons for not outputting the php errors on a production server). If we do keep the error output in html, we should wrap both the statement and error output with the text() function (although not officially published, this issue has just been brought to attention by one of the black hats; it’s a global xss vulnerability).
Can you provide more details on what the black hat reported? I’d like to understand how significant of a threat it is. Somehow I suspect it’s very hard to get the SQL error message to actually do something malicious.
Reporting the errors directly in the HTML output makes it easier to troubleshoot things with novice users. Given that the error log on a new install reports ridiculous numbers of “undefined” errors and a new user may not even know of the error log’s existence in the first place, it’s going to be much harder to figure out what has gone wrong because of things like, forgetting to run the sql_upgrade scripts after a patch, or finding somewhere new we forgot to escape a query parameter that chokes with a single quote.
I don’t really like that we report sql errors in this manner, but it definitely has utility, but I think changing it will cause more harm than good.
I let this group know that all items except 2,4 were dealt with, however it appears there testing still shows 7 is an issue. And since this sql-injection is still an issue, then item 6 is still an issues (can create a bad sql query that then throws the variables to the screen without being escaped). So can do this wherever a sql-injection vulnerability exists. It’s not critical or anything, but it’s an easy thing to harden by just surrounding the sql error output variables with text() before sending them to the screen.
On the security front, OpenEMR is getting more and more security releases lately, which is likely related to it becoming more accepted. Here’s the wiki page attempting to follow these. It think the only published vulnerabilities not dealt with (except for issues noted in this post above) are items 2,4 of the link in this post above. Would be nice to clear them all and make is a long-term priority to remove all xss and sql-injection vulnerabilities from OpenEMR.
I get it now and realize that text() around the $statement in HelpfulDie does matter.
The concrete example is if the GET parameter for begin is
...BAD SQL<script>window.alert("yo!")</script>
Then HelpfulDie is going to return code]…ORDER BY BAD SQL <script>window.alert(“yo!”)</script> as html that will get executed in the browser.
So because of HelpfulDie any SQL injection vulnerability is currently also a non-persistent XSS vulnerability. (until we add text(), which we should…)
#7 looks like it should be fixed though because there are add_escape_custom() calls around each of the parameters in pnotes.inc
Do they have a more detailed description of how to exploit it? If add_escape_custom() isn’t sufficient then other places we’ve been assuming to be safe may not really be.
Hi,
Regarding number 7, I am a bit confused there. It appears to be taken care of and I even escaped all the other functions in library/pnote.inc . I was gonna look into it in more detail again (ie. be 100% confident that everything is escaped) before emailing secunia back.
-brady OpenEMR
(I’d be happy to commit the text fix to the sql error once the clinicalrules parsing fix is in the codebase; so I can test it)
btw,
I’ll cc you and Rod on the next email secunia since something of interest may come out of this (if we are missing something fundamental)
-brady OpenEMR
Hi,
Committed the security fix to sourceforge (escape the html output in sql.inc). Also sent an email to secunia regarding the sql-injection issue on item 7; on my testing the mysql special characters for those variables are definitely getting escaped, which prevents sql-injection.
-brady OpenEMR
Learned a great deal from the black hats emails. Turns out we need to have mechanisms for escaping non-parameters (things that are not surrounded by a single quote). Here’s a commit that allows “escaping” of Limits, Sort Order Keywords and Identifiers. This should fix item 7 (still need to officially test it). Check it out and let me know your thoughts on this: http://github.com/bradymiller/openemr/commit/31605cf51c252ba0ac1196026b855bb55818f5ec
The ticks don’t get escaped (ie. they don’t protect from sql-injection when wrap them around something). Additionally, in the cases where you place the table in the identifier (such as table.row), then can’t wrap the entire thing with backticks, instead can only wrap the row portion (table.`row`).
The main issue here, which I think you are also thinking about is how much more work it may take to know if identifiers are ok or not. For example, there may be some places in the custom and layouts engine and CDR enginewhere this could be tough. So, I have added two options to the escape_identifier() function to support whitelisting (when know what we are looking for) and sanitization (when not known what we are looking for). I placed more explanation in the function comments. Here is the new commit based on master (note I also updated the documentation in formdata.inc.php, reorganized it and had it included ubiquitously (interface/globals): http://github.com/bradymiller/openemr/commit/c8823452af10e128ab0bce138ccb2de2bbf78c4c
It is testing well and the following sql-injection vector does not work in any of the sql-parameter from item 7:
(case(select 1 and 1=sleep(5)) when 1 then pnotes.title end)
Note you can play around with the escape_identifier() function in non-whitelisting mode if want (left an uncommented call to this in pnotes.inc script).