yehster wrote on Wednesday, January 11, 2012:
http://www.exploit-db.com/exploits/17998/
https://github.com/yehster/openemr/commit/b91c1cd4dd48febfc7b6f93eced08f555187e824
Committed this to sourceforge after confirming that the described attack URL no longer works.
The specific query identified in this audit was corrected, but there are actually a few other potential attack modalities in the same add_edit_issue.php file.
bradymiller wrote on Wednesday, January 11, 2012:
Hi,
Some good points are brought to light here regarding sql-injection, magic quotes and security. The script interface/patient_file/summary/add_edit_issue.php basically does not deal with either magic quotes or sql-injection. The ideal way to deal with this stuff in order to ensure compliance with both magic quotes off/on and prevent sql-injection is to:
1. Remove request variable escaping if magic quotes is on
2. Apply sql escaping in the sql query
In the above commit, half of this issue has been dealt with (number 2), but now this script is theoretically not compatible with the magic quotes on setting (ie. you will double-escape single/double quotes). So, it’s best to deal with both 1 and 2 to ensure compliance with both maghic quotes setting.
Ideally this script will be converted to the new security model, which will then make it safe to also add to the patient portal:
http://www.open-emr.org/wiki/index.php/Codebase_Security#Plan
(note that both number 1 and number 2 above are dealt with)
(also for the long sql-queries with multiple variables, rather than use binding, can use the add_escape_custom() function from library/formdata.inc.php script instead)
-brady
bradymiller wrote on Wednesday, January 11, 2012:
Also,
Forgot to mention. For this patch, if want to to the focused fix only, then to deal with number 1, would change:
$issue = $_REQUEST;
to:
$issue = strip_escape_custom($_REQUEST);
(and include the /library/formdata.inc.php script)
-brady
bradymiller wrote on Wednesday, January 11, 2012:
But then with post 3, note there will then be potential inconsistency issues with magic quotes, since there are several other queries use the $issues variable. Will be much better to convert this script to the new security model rather than make additional changes (ok to keep the one you’ve already committed); I’ll add this to my queue unless you want to take a stab at it.
-brady
yehster wrote on Wednesday, January 11, 2012:
Brady,
I have to admit that I wasn’t fully aware of the magic quotes issue before I committed. I thought it was indeed a trivial change.
However, is the magic quotes concept really relevant for $issue? It’s not valid for single quotes, double quotes or backslashes to appear in that variable in the first place since it needs to represent an integer value as a key into the list table.
In other words, $_REQUEST shouldn’t contain ",’ or \ in the first place, so if it did that likely means someone is messing with the get request manually (at their own peril…)
(also for the long sql-queries with multiple variables, rather than use binding, can use the add_escape_custom() function from library/formdata.inc.php script instead)
You’re saying that for the below current code, all of the values should be wrapped with add_escape_custom(), (easier than re-writing with binding, whew)!
if ($issue) {
$query = "UPDATE lists SET " .
"type = '" . $text_type . "', " .
"title = '" . $_POST['form_title'] . "', " .
"comments = '" . $_POST['form_comments'] . "', " .
"begdate = " . QuotedOrNull($form_begin) . ", " .
"enddate = " . QuotedOrNull($form_end) . ", " .
"returndate = " . QuotedOrNull($form_return) . ", " .
"diagnosis = '" . $_POST['form_diagnosis'] . "', " .
"occurrence = '" . $_POST['form_occur'] . "', " .
"classification = '" . $_POST['form_classification'] . "', " .
"reinjury_id = '" . $_POST['form_reinjury_id'] . "', " .
"referredby = '" . $_POST['form_referredby'] . "', " .
"injury_grade = '" . $_POST['form_injury_grade'] . "', " .
"injury_part = '" . $form_injury_part . "', " .
"injury_type = '" . $form_injury_type . "', " .
"outcome = '" . $_POST['form_outcome'] . "', " .
"destination = '" . $_POST['form_destination'] . "', " .
"reaction ='" . $_POST['form_reaction'] . "', " .
"erx_uploaded = '0' " .
"WHERE id = '$issue'";
sqlStatement($query);
Anyway, I’ll keep this on my queue so we can talk about the security model for my benefit and for any other interested parties. I will commit to a personal branch and solicit feedback next time before updating sourceforge.
bradymiller wrote on Wednesday, January 11, 2012:
Hi,
You are correct on the $issue assessment that it would require significant effort to elucidate any magic quote issues, so practically your quick mod is fine. Just saw a good reason to bring up the magic quotes issue because it’s something that is usually not thought about and can become a real pain; as of a couple years ago, there were places in the code that would break with magic quotes on and other places that would break with magic quotes off (when placing single/double quotes into forms). Took a significant effort to remove those issues, so I’m just being hypersensitive to reintroducing these issues (anything that might break compliance with magic quotes on setting). For example, this script will break if place single/double quotes is used in form and magic quotes is set to off, but will work if magic quotes is set to on. And if $issue was a form entry (which, it is not; just illustrating a point), then would be a situation where single/double quotes would break in both magic quotes off (for other form entries) and magic quotes on (if $issue had a form entry). Hope this makes sense.
Since this is a rather central script, makes sense to just convert it to the new security model to ensure it works with magic quotes off setting also.
Magic quotes are simply evil 
-brady
yehster wrote on Wednesday, January 11, 2012:
I run my system with magic quotes off (the default with php >= 5.3.0) and can verify that escapable characters in the form did indeed cause insert and update to behave strangely.
Anyway, I changed this for just the title field here, and want to see if you think it’s done right.
https://github.com/yehster/openemr/commit/5ee13b9ab3507403a937cc23fd5e87bc78c9c209
bradymiller wrote on Wednesday, January 11, 2012:
Hi,
Here’s a quick conversion of this script to the new security model (still need to test it):
http://github.com/bradymiller/openemr/tree/new_security_model_example
This script should now be safe from sql-injection and cross-scripting attacks. Security model is described here(note use of new functions in library/htmlspecialchars.inc.php to avoid the length htmlspecialchars and xl function calls):
http://www.open-emr.org/wiki/index.php/Codebase_Security#Plan
-brady
bradymiller wrote on Wednesday, January 11, 2012:
Hi,
Realized I made a mistake with the new htmlspecialchars functions. I need to replace the out() function calls with text() function instead.
-brady
bradymiller wrote on Wednesday, January 11, 2012:
Hi,
Next revision (testing ok, but plan to do a bit more testing) of the new security model implemented in the add_edit_issue.php script:
http://github.com/bradymiller/openemr/tree/new_security_model_example_2
This script should be now compatible with magic quotes off and on and protected from sql-injection and cross scripting attacks.
I also updated the following wiki section, so it is hopefully more clear now:
http://www.open-emr.org/wiki/index.php/Codebase_Security#Plan
-brady
bradymiller wrote on Saturday, January 14, 2012:
Hi,
Finished incorporating security model into the add_edit_issue.php script and is testing well:
http://github.com/bradymiller/openemr/commits/new_security_model_example_3
Plan to commit this unless any issues. The text(), xlt(), attr(), and xla() seem pretty intuitive and were much quicker to intitute than the htmlspecialchars() functions. For an explanation of these functions, see here (Step 4):
http://www.open-emr.org/wiki/index.php/Codebase_Security#Plan
Do not plan to include these types of changes in the 4.1 patches, since the mods are rather extensive and will require a period of time to ensure no bugs were introduced.
Next module plan to clean up is the address book module.
-brady
bradymiller wrote on Saturday, January 14, 2012:
Hi,
And here’s the incorporation of the security model into the address book module (testing well). Plan to commit this unless any issues brought up:
http://github.com/bradymiller/openemr/commits/security-address-book_1
-brady