Codebase Security

Codebase Security Vulnerability Assessment and Fixing

Assessment

• Nice independent security analysis by Realsearch group at NC State.

The Realsearch group at NC State has been working with OpenEMR in it's evaluation of the CCHIT security criteria. As a part of this research they've done automated testing of the application and have discovered a number of security vulnerabilities with the software. They have gone through and tried to manually verify each vulnerability. The list of actual vulnerabilities, more than 500 in total, can be found at the links below. The true vulnerabilities have a value of True in the 'Vulnerable' column.

• Automated Penetration Testing Test Results
• Static Analysis

As a summary, here are the types of issues they've found and their counts:

Fortify 360:

Cross-Site Scripting (215)

Nonexistent Access Control (129)

Dangerous Function (24)

Path Manipulation (20)

Error Information Leak (19)

Global Variable Manipulation (9)

Insecure Upload (8)

Improper Cookie Use (7)

HTTP Header Manipulation (4)

Rational AppScan:

Cross-Site Scripting (50)

Phishing Through Frames (25)

Cross-Site Request Forgery (22)

Error Message Information Leak (14)

SQL Injection (4)

JavaScript Cookie References (6)

Directory Listing (6)

Password Not Encrypted (2)

Path Disclosure (1)

• Some sort of software convention (Eurostar 2009) looking for OpenEMR bugs, still waiting for them to post the bugs. Here are all the links pertaining to this: link1 Link2 link3 link4 link5

• Published security advisories:

Arbitrary Database Creation/Database Enumeration - OpenEMR 4.0.0 (FIX included in 4.0.0 patch released on 4/15/2011)

Local File Inclusion - OpenEMR 4.0.0 (FIX included in 4.0.0 patch released on 4/15/2011)

Reflected Cross-site Scripting - OpenEMR 4.0.0

Plan

• Proposal/plan to fix. This is a specific proposal/plan which is currently underway in order to prevent sql-injection and xss attacks. This involves a per script walk-through of the code. For developers, new scripts or scripts that have been converted to this new system need to follow these steps. Not only does this secure the script from sql-injection and xss attack, but it also markedly simplifies coding since the developer does not need to deal/worry about database escaping of variables (escape for a rare case). The strategy includes the following steps; will first discuss the strategy, and then will discuss how to practically do it.

1. Remove the mechanism to fake register globals. This is discussed here: Clean up use of the extract... (HMM, seem to have lost this stuff; will dig it back up)

2. Automatically reverse magic quotes (if turned on).

3. Utilize binding/placeholders in sql calls (prevents sql-injection). If making a sql call that contains a variable in it, then use a placemaker.

• Exception in sql queries that contain numerous variables or when placing a variable to represent a sql tabl or colum name; in this case utilize the add_escape_custom() function from the library/formdata.inc.php script.

4. Utilize htmlspecialchars function (prevent xss attacks). This function should be wrapped around all text that can potentially be "touched" by the user.

• Exception to rule 4 for javascript literals; when using javascript literals, utilize the addslashes function instead to prevent white screen of death.

• Below is how to practically complete the above steps:

1. Remove the mechanism to fake register globals. Place the following code at top of script above the include statement for the globals.php file:

    $fake_register_globals=false;

2. Automatically reverse magic quotes (if turned on). Place the following code at top of script above the include statement for the globals.php file::

    $sanitize_all_escapes=true;

3. Utilize binding/placeholders in sql calls (prevents sql-injection).

    sqlStatement=("DELETE FROM immunizations WHERE id =? LIMIT 1",array($_GET['id']);

• Exception to rule 3 for when a variable represents a sql table or column:

    require_once("$srcdir/formdata.inc.php");
    sqlStatement=("DELETE FROM ".add_escape_custom($table_name)." WHERE id =1");

• Exception to rule 3 for when there are a large number of variable in the sql query:

    sqlStatement=("UPDATE lists SET " .
    "type = '" . add_escape_custom($text_type) . "', " .
    "title = '" . add_escape_custom($_POST['form_title']) . "', " .
    "comments = '". add_escape_custom($_POST['form_comments']) . "', " .
    "begdate = " . add_escape_custom(QuotedOrNull($form_begin)) . ", "  .
    ...</pre


::4. Utilize htmlspecialchars function wrappers from library/htmlspecialchars.inc.php.  These functions have ample documentation within that file.
<pre>
    require_once("$srcdir/htmlspecialchars.inc.php");
    $para_class = attr($db_data); //is escaped(including quotes)
    $para_content = text($user_data); //is escaped(not including quotes)
    $title_text = xla('My Title'); //is translated and escaped(including quotes)
    $header_text = xlt('My Header'); //is translated and escaped(not including quotes)
    echo "<h3>$header_text</h3><p class='$para_class' title='$title_text'>$para_content</p>\n";

• Exception to rule 4 for javascript literals; when using javascript literals, utilize the addslashes function instead to prevent white screen of death.

    <script LANGUAGE="JavaScript">
    alert('<?php echo addslashes(xl('Hello there')) ?>');
    </script>

Implementation

• Below are examples where scripts have been converted to the new security mechanism discussed above:

Messages and Pnotes (patient notes) module: http://github.com/openemr/openemr/commit/21e15cce4507d36c7ffd234f2c4f034b38d1087e

Patient searching modules: http://github.com/openemr/openemr/commit/a9aa64513e4556aeb2f36b049e86aac47b3fef42

Transactions module: http://github.com/openemr/openemr/commit/f56f469c9d2481f3d440c79db1917e0a38f076a9

Patient history module: http://github.com/openemr/openemr/commit/a4817af442d569525b24129ed75afa915030a4dd

Immunization module: http://github.com/openemr/openemr/commit/5d06c6f08d04405a80b036810a8523a7cb680a31

Authorization module: http://github.com/openemr/openemr/commit/e08e3327b83f36164db0177c9acb8b7a1c3f9ddb

demographics.php script: http://github.com/openemr/openemr/commit/c0bfa8a51106cd97842374d5ae719bb5b469b763

Language admin gui module: http://github.com/openemr/openemr/commit/28f02594d450ce1e1546557b4cee040b8bedc194

Ongoing Discussion

• The forum where this plan was developed/discussed along with updates on progress is here: http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3530656