Codebase Security

Codebase Security

Overview

At this point in time there are no known pre-authentication vulnerabilities in OpenEMR 4.1.1(with most recent patch installed) or the development version. Thus, the current vulnerabilities require login by an authorized user. These type of vulnerabilities are considered post-authentication. The long-term goal of OpenEMR is to have no post-authentication vulnerabilities, which is extremely desirable these days secondary to malware and social phishing methods. This page lists some security analysis done by third parties in addition to the current ongoing project and future roadmap in order to make the OpenEMR codebase free of vulnerabilties.

Assessment

Analysis by SANS Institute

• A nice, preliminary report can be found here:

• openemr_vuln_sans.pdf
• Quick summary:

1. Many cross-scripting vulnerabilities
2. Many sql-injection vulnerabilities
3. Valid username extraction (via brute force)
4. Password pass the hash
5. Arbitrary file uploading

Analysis by OWASP team of the Technical Educational Institute of Larisa

• Security analysis of OpenEMR by the OWASP team of the Technical Educational Institute of Larisa (contributed by Avantsys Informatics):

• openemr-security-report.pdf
• Quick Summary:

1. When uploading files, filenames and/or files are not being filtered/sanitized.
2. Many cross-scripting vulnerabilities
3. Many sql-injection vulnerabilities

Analysis by Realsearch group at NC State

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

Miscellaneous Analysis

• 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
• List of published security exploits

Plan

SQL-Injection and Cross-Scripting Prevention

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

Step 1. Remove the mechanism to fake register globals.

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

Step 3. Escape variables within sql calls (prevents sql-injection).

Step 4. Escape variables displayed in html (prevent xss attacks).

• Below is how to practically complete the above steps:

Step 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;

Step 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;

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

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

• Exception to step 3 for when there are a large number of variable in the sql query(if do this, need to treat all variables this way; meaning do not combine the two methods in one statement to avoid the '?' character within datafields breaking things)(also ensure surround the variable with the single quotes):

        sqlStatement=("UPDATE lists SET " .
        "type = '" . add_escape_custom($text_type) . "', " .
        "title = '" . add_escape_custom($_POST['form_title']) . "', " .
        "comments = '". add_escape_custom($_POST['form_comments']) . "', " .
        ...

• Exception to step 3 for when a variable represents a sql table:

        sqlStatement=("DELETE FROM ".escape_table_name($table_name)." WHERE id =1");

• Exception to step 3 for when a variable represents a sql column (long form; ie. $column_name = users.fname )(Note that the second parameters is the table the column is from and you can send multiple tables):

        sqlStatement=("SELECT ".escape_sql_column_name($column_name,array("users"),TRUE)." FROM users WHERE id =1");

• Exception to step 3 for when a variable represents a sql column (short form; ie. $column_name = fname )(Note that the second parameters is the table the column is from and you can send multiple tables):

        sqlStatement=("SELECT ".escape_sql_column_name($column_name,array("users"))." FROM users WHERE id =1");

• Exception to step 3 for when a variable represents a LIMIT amount(s):

        sqlStatement=("SELECT * FROM users WHERE id =1 LIMIT ".escape_limit($limit_number));

• Exception to step 3 for when a variable represents a sort ordering sql keyword(asc or desc):

        sqlStatement=("SELECT * FROM users WHERE id =1 ORDER BY id ".escape_sort_order($sort_order_keyword));

• Exception to step 3 for when a variable represents a another sql identifier not covered yet(if there are any). Two possible methods exist and the first one is the preferred method:

• If all the possible options are known beforehand, then use(note that if you send TRUE for the third parameter, then it will die() and throw an error if there is no match):

        $options = array("users","patients","documents");
        sqlStatement=("DELETE FROM ".escape_identifier($identifier,$options)." WHERE id =1");

• If all the possible options are not known beforehand, then use (this method is experimental only):

        sqlStatement=("DELETE FROM ".escape_identifier($identifier)." WHERE id =1");

Step 4. Utilize htmlspecialchars function wrappers from library/htmlspecialchars.inc.php. These functions have ample documentation within that file. (prevents cross-scripting attacks)

    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 step 4 for javascript literals; when using javascript literals, utilize the addslashes function instead to prevent white screen of death, or for translation strings use the new (as of 12/21/2012) xls() shorthand function included in library/htmlspecialchars.inc.php

        <script LANGUAGE="JavaScript">
        alert('<?php echo addslashes($db_data)); ?>'); // escape potential quotations marks
        alert('<?php echo xls('Quotes and Apostrophes in translations may cause problems'); ?>
           // If the return value for a translation contains quotes or apostrophes using just xl
           // would result in white screen of death.  Prefer xls, but addslashes(xl('my string')) 
           // will prevent problems.
        </script>

• Another rare exception to step 4 when further nesting php variable within a javascript literal that is nested within html code.

        "<a href='' onclick='return selcode(\"" . attr(addslashes($drug_id)) . "\")'>";

Implementation

• Below are examples where scripts have been converted to the new security mechanism discussed above (to prevent sql-injection and cross-scripting attacks):

Login Module: http://github.com/openemr/openemr/commit/66823e8a56f64952010cfe3f270183b4c0b92f59

Office Notes Module: http://github.com/openemr/openemr/commit/8a8a4607ba5ae2b9eb6b6a3b1b8ed7c6ea7e03b1

Chart Tracker Module: http://github.com/openemr/openemr/commit/8a8a4607ba5ae2b9eb6b6a3b1b8ed7c6ea7e03b1

Encounter Form: http://github.com/openemr/openemr/commit/e1b0edb3d8067d42376d65435bd97ff5cde373c5

Dictation Form and Work/School Note Form: http://github.com/openemr/openemr/commit/d56d58fcfbe04214f9df419901e766341a1b95d1

Layout Based Fields forms: http://github.com/openemr/openemr/commit/61c95e62f79ad2afb68a2e8b1f81e3cca20b99ea

Part of Billing Module and calendar add_edit_event.php script: http://github.com/openemr/openemr/commit/f6310936b9dbc74e591ad56a75dae80b7791639d

Drug Dispensory Module: http://github.com/openemr/openemr/commit/67c82076f8af6ca08009a8948641e57cc0b8bd90

Address Book module: http://github.com/openemr/openemr/commit/c3243ac074795657a35b04609b83dac7ed423af0

add_edit_issue.php script: http://github.com/openemr/openemr/commit/34874e7e1e4e76ec44fb62a07136121462b306e1

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

Sanitize file and directory names

Use the functions in the library/sanitize.inc.php script.

Secure method to upload files

Currently under discussion.

Do not allow guessing of usernames

Fix has been committed to the development branch of OpenEMR:

• http://github.com/openemr/openemr/commit/c9a5aa413928a6666fce4d3f48e0285b5455b1ae

Optimize password security

Currently, OpenEMR 4.1.1 stores the user's password as a SHA1 hash. The password is hashed on the client end in javascript and then passed to the server for authentication. This flow opens OpenEMR up to a pass the hash vulnerability if the hashes are obtained from the OpenEMR database.

• A solution and fix has been committed to the development branch of OpenEMR:

• http://github.com/openemr/openemr/commit/c9a5aa413928a6666fce4d3f48e0285b5455b1ae

Secure the Embedded Third Party Elements

Need to secure/update the third party elements (such as phpgacl and phpmyadmin atc.)

• List of the Embedded Components is here.

Stay updated with published vulnerabilties and fixes

This is maintained at the Security Alert Fixes wiki page.

Maintain updated securing OpenEMR instructions for users

This is maintained at the Securing OpenEMR wiki page.

Ongoing Discussion

• The forum where this plan is developed/discussed along with updates on progress is here: http://sourceforge.net/p/openemr/discussion/202506/thread/566ef9c0