Difference between revisions of "Codebase Security"

From OpenEMR Project Wiki
 
(108 intermediate revisions by 2 users not shown)
Line 1: Line 1:
__TOC__
__TOC__


= Codebase Security Vulnerability Assessment and Fixing =
= Codebase Security =
==Overview==
:This will always be a work in progress.
 
==Assessment==
==Assessment==
===Analysis by ViSolve===
:The security team at ViSolve has been evaluating the SQL injection issue in OpenEMR. For this purpose the “SQL Inject Me” tool was used. The tool generated reports documenting few major SQL vulnerabilities.
:Some of the form fields in the application were found to be at high risk, which can be rectified if the latest security mechanism (refer the Plan section below) of the OpenEMR is implemented in the pages where vulnerabilities were found. We also identified that few fields were not validated properly on the server side.
::*[https://docs.google.com/file/d/0B2PXx3MYPPTEaTlTMGR0TUhzRkE/edit?usp=sharing Consolidated Test Result]
::*[[Media:Oemr_sql_injection_test_results.zip|Oemr_sql_injection_test_results.zip]]
===Analysis by SANS Institute===
===Analysis by SANS Institute===
*A nice, preliminary report can be found here:
*A nice, preliminary report can be found here:
Line 9: Line 21:
::#Many cross-scripting vulnerabilities
::#Many cross-scripting vulnerabilities
::#Many sql-injection vulnerabilities
::#Many sql-injection vulnerabilities
::#Valid username extraction
::#Valid username extraction (via brute force)
::#Password storage hash not salted
::#Password pass the hash
::#Arbitrary file uploading
::#Arbitrary file uploading
===Analysis by OWASP team of the Technical Educational Institute of Larisa===
===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):
*Security analysis of OpenEMR by the OWASP team of the Technical Educational Institute of Larisa (contributed by Avantsys Informatics):
Line 47: Line 60:
:::Path Disclosure (1)
:::Path Disclosure (1)


===Analysis by Project Insecurity===
:[[Media:Openemr insecurity.pdf]]
===Miscellaneous Analysis===
*Some sort of software convention ([http://www.eurostarconferences.com/ Eurostar 2009]) looking for OpenEMR bugs, still waiting for them to post the bugs. Here are all the links pertaining to this: [http://www.eurostarconferences.com/blog/2009/12/9/when-is-a-conference-more-than-a-conference.aspx link1] [http://www.workroom-productions.com/blog/2009/12/testlab-followup-not-yet.html Link2] [http://www.developsense.com/2009/12/eurostars-test-lab-bravo.html link3] [http://www.developsense.com/2009/12/best-bug-or-bugs.html link4] [http://thetesteye.com/blog/2009/12/notes-from-eurostar-2009/ link5]
*Some sort of software convention ([http://www.eurostarconferences.com/ Eurostar 2009]) looking for OpenEMR bugs, still waiting for them to post the bugs. Here are all the links pertaining to this: [http://www.eurostarconferences.com/blog/2009/12/9/when-is-a-conference-more-than-a-conference.aspx link1] [http://www.workroom-productions.com/blog/2009/12/testlab-followup-not-yet.html Link2] [http://www.developsense.com/2009/12/eurostars-test-lab-bravo.html link3] [http://www.developsense.com/2009/12/best-bug-or-bugs.html link4] [http://thetesteye.com/blog/2009/12/notes-from-eurostar-2009/ link5]
*[[Security Alert Fixes|List of published security exploits]]


==Plan==
==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.'''
===Maintain updated securing OpenEMR instructions for users===
:Step 1. Remove the mechanism to fake register globals.
:This is maintained at the [[Securing OpenEMR]] wiki page.
: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).


===CSRF===
:Cross-Site Request Forgery.
::First commit in main codebase with the main mechanism and example of use in adminacl scripts:
::*https://github.com/openemr/openemr/commit/1510ed42cf86fc76572e4d9c0875d953acfc5c93
::Subsequent commits:
::*https://github.com/openemr/openemr/commit/373ca9c7bc8bea464250785c68fb2bd2d147ffd9
::*https://github.com/openemr/openemr/commit/f0a642e437299c2a609ee6bb70e8e53294a919fa
::*https://github.com/openemr/openemr/commit/31eeaa3058a61db664aee0b9caf4873bc0c79cd4
::*https://github.com/openemr/openemr/commit/23f0cc3b6e439e724fb424588ca8cfa320e26930
::*https://github.com/openemr/openemr/commit/6fa02fdbb4cc3f6680d1260b3654491186b44ee3
::*and many more commits...
:Added samesite cookie use for when using PHP 7.3+ (for OpenEMR 5.0.2+)


*Below is how to practically complete the above steps:
===Escape Shell Commands===
:Walking through the codebase and use escapeshellcmd() and escapeshellarg() functions to prevent vulnerabilities that stem from using shell commands in php.


===CSV (Formula) Injection===
:Use the csvEscape() function.


::'''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:'''
===Log Injection===
:* https://www.owasp.org/index.php/Log_Injection
:* Use the errorLogEscape() function to sanitize the log output.
<pre>
<pre>
     $fake_register_globals=false;</pre>
     error_log('Error in following: ' . errorLogEscape($errorVariable));</pre>


===Header Sanitation===
:* https://www.netsparker.com/blog/web-security/crlf-http-header/
:* Currently researching best method to prevent this.
:* https://github.com/openemr/openemr/issues/2357


::'''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:'''
===SQL-Injection===
<pre>
    $sanitize_all_escapes=true;</pre>
 


::'''Step 3. Utilize binding/placeholders in sql calls (prevents sql-injection).'''
::'''Utilize binding/placeholders in sql calls (prevents sql-injection).'''
<pre>
<pre>
     sqlStatement=("DELETE FROM immunizations WHERE id =? LIMIT 1",array($_GET['id']);</pre>
     sqlStatement=("DELETE FROM immunizations WHERE id =? LIMIT 1",array($_GET['id']);</pre>
::::* '''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):'''
::::* '''Exception to step 1 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):'''
<pre>
<pre>
         sqlStatement=("UPDATE lists SET " .
         sqlStatement=("UPDATE lists SET " .
Line 82: Line 112:
         "comments = '". add_escape_custom($_POST['form_comments']) . "', " .
         "comments = '". add_escape_custom($_POST['form_comments']) . "', " .
         ...</pre>
         ...</pre>
 
::::* '''Exception to step 1 for when a variable represents a sql table:'''
::::* '''Exception to step 3 for when a variable represents a LIMIT amount(s):'''
<pre>
        sqlStatement=("DELETE FROM ".escape_table_name($table_name)." WHERE id =1");</pre>
::::* '''Exception to step 1 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):'''
<pre>
        sqlStatement=("SELECT ".escape_sql_column_name($column_name,array("users"),TRUE)." FROM users WHERE id =1");</pre>
::::* '''Exception to step 1 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):'''
<pre>
        sqlStatement=("SELECT ".escape_sql_column_name($column_name,array("users"))." FROM users WHERE id =1");</pre>
::::* '''Exception to step 1 for when a variable represents a LIMIT amount(s):'''
<pre>
<pre>
         sqlStatement=("SELECT * FROM users WHERE id =1 LIMIT ".escape_limit($limit_number));</pre>
         sqlStatement=("SELECT * FROM users WHERE id =1 LIMIT ".escape_limit($limit_number));</pre>
::::* '''Exception to step 3 for when a variable represents a sort ordering sql keyword(asc or desc):'''
::::* '''Exception to step 1 for when a variable represents a sort ordering sql keyword(asc or desc):'''
<pre>
<pre>
         sqlStatement=("SELECT * FROM users WHERE id =1 ORDER BY id ".escape_sort_order($sort_order_keyword));</pre>
         sqlStatement=("SELECT * FROM users WHERE id =1 ORDER BY id ".escape_sort_order($sort_order_keyword));</pre>
::::* '''Exception to step 3 for when a variable represents a sql identifier. Two possible methods here and the first one is the preferred method:'''
::::* '''Exception to step 1 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:
:::::* '''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):
<pre>
<pre>
         $options = array("users","patients","documents");
         $options = array("users","patients","documents");
         sqlStatement=("DELETE FROM ".escape_identifier($table_name,TRUE,$options)." WHERE id =1");</pre>
         sqlStatement=("DELETE FROM ".escape_identifier($identifier,$options)." WHERE id =1");</pre>
:::::* '''If all the possible options are not known beforehand, then use:
:::::* '''If all the possible options are not known beforehand, then use (this method is experimental only):
<pre>
<pre>
         sqlStatement=("DELETE FROM ".escape_identifier($table_name)." WHERE id =1");</pre>
         sqlStatement=("DELETE FROM ".escape_identifier($identifier)." WHERE id =1");</pre>


 
===Cross-Scripting Prevention===
::'''Step 4. Utilize htmlspecialchars function wrappers from library/htmlspecialchars.inc.php.  These functions have ample documentation within that file. (prevents cross-scripting attacks)'''
::'''Utilize htmlspecialchars function wrappers from library/htmlspecialchars.inc.php.  These functions have ample documentation within that file. (prevents cross-scripting attacks)'''
<pre>
<pre>
    require_once("$srcdir/htmlspecialchars.inc.php");
     $para_class = attr($db_data); //is escaped(including quotes)
     $para_class = attr($db_data); //is escaped(including quotes)
     $para_content = text($user_data); //is escaped(not including quotes)
     $para_content = text($user_data); //is escaped(not including quotes)
Line 107: Line 144:
     $header_text = xlt('My Header'); //is translated and escaped(not 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";</pre>
     echo "<h3>$header_text</h3><p class='$para_class' title='$title_text'>$para_content</p>\n";</pre>
::::* '''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'''
::::* '''Exception to step 2 for javascript literals; when using javascript literals, utilize the custom js_escape() function instead to prevent white screen of death, or for translation strings use the custom xlj() function; both these functions can be found in library/htmlspecialchars.inc.php'''
<pre>
        <script LANGUAGE="JavaScript">
        var example = <?php echo js_escape($variable); ?>;
        alert(<?php echo js_escape($db_data)); ?>);
        alert(<?php echo xlj('This software is the best'); ?>
        </script></pre>
::::* '''Exception to step 2 for when escaping in the javascript context, however then inject the variable into html; utilize the custom javascript jsText() and jsAttr() functions; both these functions can be found in library/js/utility.js'''
<pre>
<pre>
         <script LANGUAGE="JavaScript">
         <script LANGUAGE="JavaScript">
         alert('<?php echo addslashes($db_data)); ?>'); // escape potential quotations marks
         var example = <?php echo js_escape($variable); ?>;
         alert('<?php echo xls('Quotes and Apostrophes in translations may cause problems'); ?>
         alert(example);
          // If the return value for a translation contains quotes or apostrophes using just xl
        element.innerHTML = jsText(example);
          // would result in white screen of death. Prefer xls, but addslashes(xl('my string'))
          // will prevent problems.
         </script></pre>
         </script></pre>
::::* '''Another rare exception to step 4 when further nesting php variable within a javascript literal that is nested within html code.'''
::::* '''Another rare exception to step 2 when further nesting php variable within a javascript literal that is nested within html code.'''
<pre>
        "<a href='' onclick='return selcode(" . js_attr($drug_id) . ")'>";
</pre>
::::* '''Yet another rare exception to step 2 when further nesting php variable within a url html attribute for a get variable.'''
<pre>
<pre>
         "<a href='' onclick='return selcode(\"" . attr(addslashes($drug_id)) . "\")'>";
         echo "<td bgcolor='#CCFFCC' width='10%'><a class='link_submit' href='./forms_admin.php?id=" . attr_url($registry['id']) . "&method=disable&csrf_token_form=" . attr_url(collectCsrfToken()) . "'>" . xlt('enabled') . "</a>";
</pre>
</pre>


==Implementation==
===Sanitize file and directory names===
* Below are examples where scripts have been converted to the new security mechanism discussed above:
:Use the functions in the library/sanitize.inc.php script.
: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
===Secure method to upload files===
:Drug Dispensory Module: http://github.com/openemr/openemr/commit/67c82076f8af6ca08009a8948641e57cc0b8bd90
:Use the functions in the library/sanitize.inc.php script.
:Address Book module: http://github.com/openemr/openemr/commit/c3243ac074795657a35b04609b83dac7ed423af0
:Optional feature to only allow certain filetypes when uploading patient documents:
:add_edit_issue.php script: http://github.com/openemr/openemr/commit/34874e7e1e4e76ec44fb62a07136121462b306e1
::*https://www.open-emr.org/wiki/index.php/Administration_Globals#Secure_Upload_Files_with_White_List
:Messages and Pnotes (patient notes) module: http://github.com/openemr/openemr/commit/21e15cce4507d36c7ffd234f2c4f034b38d1087e
 
:Patient searching modules: http://github.com/openemr/openemr/commit/a9aa64513e4556aeb2f36b049e86aac47b3fef42
===Secure the Embedded Third Party Elements===
:Transactions module: http://github.com/openemr/openemr/commit/f56f469c9d2481f3d440c79db1917e0a38f076a9
:Ensure continue to secure/update the third party elements
:Patient history module: http://github.com/openemr/openemr/commit/a4817af442d569525b24129ed75afa915030a4dd
:*List of the [[OpenEMR_Wiki_Home_Page#Embedded_Components|Embedded Components is here]].
: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==
===Do not allow guessing of usernames===
* 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
:Avoid any code in the future that allows attackers to guess usernames.


==Other miscellaneous security fix methods===
===Sanitize filenames and directory names====
:Use the functions in the library/sanitize.inc.php script.


[[Category:Security]][[Category:Developer Guide]]
[[Category:Security]][[Category:Developer Guide]]

Latest revision as of 06:41, 15 August 2020

Codebase Security

Overview

This will always be a work in progress.

Assessment

Analysis by ViSolve

The security team at ViSolve has been evaluating the SQL injection issue in OpenEMR. For this purpose the “SQL Inject Me” tool was used. The tool generated reports documenting few major SQL vulnerabilities.
Some of the form fields in the application were found to be at high risk, which can be rectified if the latest security mechanism (refer the Plan section below) of the OpenEMR is implemented in the pages where vulnerabilities were found. We also identified that few fields were not validated properly on the server side.

Analysis by SANS Institute

  • A nice, preliminary report can be found here:
  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):
  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

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

Analysis by Project Insecurity

Media:Openemr insecurity.pdf

Miscellaneous Analysis

Plan

Maintain updated securing OpenEMR instructions for users

This is maintained at the Securing OpenEMR wiki page.

CSRF

Cross-Site Request Forgery.
First commit in main codebase with the main mechanism and example of use in adminacl scripts:
Subsequent commits:
Added samesite cookie use for when using PHP 7.3+ (for OpenEMR 5.0.2+)

Escape Shell Commands

Walking through the codebase and use escapeshellcmd() and escapeshellarg() functions to prevent vulnerabilities that stem from using shell commands in php.

CSV (Formula) Injection

Use the csvEscape() function.

Log Injection

    error_log('Error in following: ' . errorLogEscape($errorVariable));

Header Sanitation

SQL-Injection

Utilize binding/placeholders in sql calls (prevents sql-injection).
    sqlStatement=("DELETE FROM immunizations WHERE id =? LIMIT 1",array($_GET['id']);
  • Exception to step 1 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 1 for when a variable represents a sql table:
        sqlStatement=("DELETE FROM ".escape_table_name($table_name)." WHERE id =1");
  • Exception to step 1 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 1 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 1 for when a variable represents a LIMIT amount(s):
        sqlStatement=("SELECT * FROM users WHERE id =1 LIMIT ".escape_limit($limit_number));
  • Exception to step 1 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 1 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");

Cross-Scripting Prevention

Utilize htmlspecialchars function wrappers from library/htmlspecialchars.inc.php. These functions have ample documentation within that file. (prevents cross-scripting attacks)
    $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 2 for javascript literals; when using javascript literals, utilize the custom js_escape() function instead to prevent white screen of death, or for translation strings use the custom xlj() function; both these functions can be found in library/htmlspecialchars.inc.php
        <script LANGUAGE="JavaScript">
        var example = <?php echo js_escape($variable); ?>;
        alert(<?php echo js_escape($db_data)); ?>);
        alert(<?php echo xlj('This software is the best'); ?>
        </script>
  • Exception to step 2 for when escaping in the javascript context, however then inject the variable into html; utilize the custom javascript jsText() and jsAttr() functions; both these functions can be found in library/js/utility.js
        <script LANGUAGE="JavaScript">
        var example = <?php echo js_escape($variable); ?>;
        alert(example);
        element.innerHTML = jsText(example);
        </script>
  • Another rare exception to step 2 when further nesting php variable within a javascript literal that is nested within html code.
        "<a href='' onclick='return selcode(" . js_attr($drug_id) . ")'>";
  • Yet another rare exception to step 2 when further nesting php variable within a url html attribute for a get variable.
        echo "<td bgcolor='#CCFFCC' width='10%'><a class='link_submit' href='./forms_admin.php?id=" . attr_url($registry['id']) . "&method=disable&csrf_token_form=" . attr_url(collectCsrfToken()) . "'>" . xlt('enabled') . "</a>";

Sanitize file and directory names

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

Secure method to upload files

Use the functions in the library/sanitize.inc.php script.
Optional feature to only allow certain filetypes when uploading patient documents:

Secure the Embedded Third Party Elements

Ensure continue to secure/update the third party elements

Do not allow guessing of usernames

Avoid any code in the future that allows attackers to guess usernames.