Security Vulnerabilities

OpenEMR Development
#21

bradymiller wrote on Friday, June 18, 2010:

hey,

I’ve completed a quick revision, and some thoughts below… Placed the commits on my github repository:
http://github.com/bradymiller/openemr/commits/security_example_2
Even cooler to look at them here (each dot is a commit in security example thread):
http://github.com/openemr/openemr/network

Left previous branch security_example on github for easy diff to see revisions.

To review the steps(commits):
1. Place new adodb bind sql.inc functionality
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())

Details revisions and thoughts:

1. Described above. Just removed a bug. Major issues is whether to combine this into original functions or to keep them separate. The only way we can combine is if we can figure out a way for them to output the same data structures (I’m still researching this, but likely not possible) in order to not break third party customized code. If keep separate, then Stephen has suggested changing the names to *_bind instead of *_safe. All I care about the name is it has a _* at end to make the walk through easier (ie easier to spot the old vs new functions).

2. Described above. Migrated the new mechanism to globals and swiped code from the online php manual (to work with php4 and php5), which seems to work well.

3. Described above. Stephen suggest this may not be possible and to keep the hooks/functions in place. The problem is that this walk through will take months, so can’t change these functions all at once. My suggestion is to completely yank them where no longer needed, however use them for when escaping of column names are needed (in the layout code). This still requires a walkthrough (can’t automate) to ensure place trim() where needed, and there are several of these escaping mechanisms throughout the codebase.

4. Described above. The sql function names will depend on outcome on number 1, but otherwise is working well.

5. Described above. This functionality already exist, so easy. But requires detailed look at each script to ensure EXTRACT is not needed.

6. Described above. Working well.

The main issues are in step 1, and will require some more research and discussion.

-brady

#22

bradymiller wrote on Friday, June 18, 2010:

hey,

Also need some guidance on the best magic quote reverser. These two examples can be found here:
http://docs.php.net/manual/en/security.magicquotes.disabling.php

Sounds like the first one may not deal with the keys of arrays. Any other better alternatives?

FIRST OPTION:

<?php
if (get_magic_quotes_gpc()) {
    $process = array(&$_GET, &$_POST, &$_COOKIE, &$_REQUEST);
    while (list($key, $val) = each($process)) {
        foreach ($val as $k => $v) {
            unset($process[$key][$k]);
            if (is_array($v)) {
                $process[$key][stripslashes($k)] = $v;
                $process[] = &$process[$key][stripslashes($k)];
            } else {
                $process[$key][stripslashes($k)] = stripslashes($v);
            }
        }
    }
    unset($process);
}
?>

SECOND OPTION:

if (get_magic_quotes_gpc()) {
    function undoMagicQuotes($array, $topLevel=true) {
        $newArray = array();
        foreach($array as $key => $value) {
            if (!$topLevel) {
                $key = stripslashes($key);
            }
            if (is_array($value)) {
                $newArray[$key] = undoMagicQuotes($value, false);
            }
            else {
                $newArray[$key] = stripslashes($value);
            }
        }
        return $newArray;
    }
    $_GET = undoMagicQuotes($_GET);
    $_POST = undoMagicQuotes($_POST);
    $_COOKIE = undoMagicQuotes($_COOKIE);
    $_REQUEST = undoMagicQuotes($_REQUEST);
}

-brady

#23

bradymiller wrote on Friday, June 18, 2010:

woohoo,

I think I was able to work sql.inc so will not need separate functions and is in my github branch security_example_3:
http://github.com/bradymiller/openemr/commits/security_example_3
http://github.com/openemr/openemr/network

Pertinent changes are in first commit.

-brady

#24

stephen-smith wrote on Friday, June 18, 2010:

1. I’m a bit uncomfortable with the functions returning different values (resource/recordset) based on if the array was empty or not.  I can imagine cases where I build up a query string and array dynamically and sometimes the array is empty.  Can we use something more special (like not an array at all, maybe NULL?) to indicate old behavior?  (It would still be the default.)

2. Yes, we need to handle keys.  We also might need to handle “magic_quotes_gpc = Off” and “magic_quotes_sybase = On” at the same time.  I also wonder if ADOdb handles “magic_quotes_runtime = On”.  We should document that OpenEMR prefers all those to be off; and probably turn off magic_quotes_runtime when we can.

(Still 2.) Both options have stylistic issues to me.  The second one looks a bit better, since it properly recurs into array values.

2 and 3. It would be nice to test/set a global flag that indicated whether we had globally de-magic-ed things.  That way, strip_slashes_custom could still work *if* the global fix hadn’t been run.  add_escape_custom should always just still work; it would only be used in code that hadn’t been converted to ADOdb, yet.  I still think there’s little reason to “fix” code that is not broken.

4. Again, I don’t like "fix"ing things that aren’t broken.  The vast majority of these calls are already safe.  They don’t need to be converted to new functions unless they are already going to show up as changed/added lines in one of our diffs/patches.

5.  Love the change!

6. No real problems.

(Still 6, but off-topic) I noticed a couple of places where you are doing something like xl(“stuff”) . $and . xl(“more stuff”).  In general, this is sort of frowned-on for translation.  It marks “stuff” and “more stuff” as separate strings to be translated, when really they are part of the same phrase, which could influence the translation.  Also, the ordering and punctuation may need to change depending on the language.  See the Qt 4.5 i18n document (particularly the linked section) and the GNU gettext manual (particularly section 4.3, the last example in section 4.4, and section 4.6).  PHP’s printf does support the “%n$” form of argument conversion, so it’s probably better than leaning on string concatenation.

#25

bradymiller wrote on Saturday, June 19, 2010:

hey,

Have next revision of commits on my github branch security_example_4:
http://github.com/bradymiller/openemr/commits/security_example_4
http://github.com/openemr/openemr/network

Same commits as above (reviewed quickly below):
1. Place new adodb bind sql.inc functionality and document/reorganize function
2. Global per-script magic quote reverser
3. Remove custom escape and database preparation functions
4. Convert sql functions to adodb binding
5. Stop faking register globals
6. Avoid xss attacks (via htmlspecialchars())

1. To avoid myself going insane, I’ve documented and re-organized all the functions in the script. Stephen, read through the functions and you’ll see the beauty of it. Basically any function that pitches out two possible data structure should will always be caught by a function that can handle both data structures. If sql queries are done per the current/previous openemr standards, it will work automatically (this is important to support third party software that breaks the no native mysql call rules). The painful thing was getting it to work with the audit engine (if not done correctly, this has potential to screw up the insert last id stuff which can basically kill the database and explode openemr); I think it’s working on my testing, but I’ll ask Visolve to take a look at it. I think this is getting close to completion (dependent on Visolve input and feedback).

2. I used option 2 from above and seems to be working fine. If anybody is using sybase or runtime magic quotes… well, that’s just natural selection at its best (if needed, can add to this function and can also turn all off in the future .htaccess file).  Shouldn’t turn this on/off globally for now since the walk through will take months. Better to just turn it on on a per script basis. Remember, we also have smarty code to contend with and that will likely take a modified technique, also going against a global switch. When it’s all done, would be easy to make a global switch if wanted and swipe the per script switches.  I think this part is pretty much completed (dependent on feedback).

3. No changes here. I’m not gonna touch the functions in options.inc.php, since they will always be useful in certain situations. But there’s no reason to wrap variables anymore with these functions if not needed (ie. magic quotes are auto removed or the database inserts are auto-added in binded commands). Pretty much completed (depending on feedback)

4. Per number one; function names do not change, so only changes to calls that include a bind array are needed. Pretty much completed (depending on feedback)

5. No change. Pretty much completed (depending on feedback)

6. No change. Pretty much completed (depending on feedback)

Miscellaneous; regarding parameterized translation functionality, read this thread (I’m surprised Pimm hasn’t responded to this yet; Pimm, if you do respond, please do it in the below link):
http://sourceforge.net/projects/openemr/forums/forum/202506/topic/3509701

-brady

#26

bradymiller wrote on Saturday, June 19, 2010:

error in above:
number 3, meant to say library/formdata.inc.php

-brady

#27

bradymiller wrote on Saturday, June 19, 2010:

Stephen,
I think I see you point on using NULL instead of empty in the first commit. Would likely be useful to have NULL, then if you place an empty array with a placeholder then adodb will recognize the error appropriately (it seems to have mechanism to ensure placeholders=numbers of binds in array). This should be straighforward with a couple line mods on commit number 1, so will plan this for next revision.
-brady

#28

bradymiller wrote on Sunday, June 20, 2010:

Next revision of commits on my github branch security_example_5:
http://github.com/bradymiller/openemr/commits/security_example_5
http://github.com/openemr/openemr/network

This code seems to be stable on my testing. Main change is in commit 1 to make compatible with Visolve’s auditing engine:
http://github.com/bradymiller/openemr/commit/c12ac124a98756db9cc3d511b4227a86173e2efa

Visolve, I think it’s doing well, but can to look t the following stuff to ensure I’m not doing anything seriously wrong:

1. library/adodb/adodb.inc.php script - Your original audit calls were misplaced. Basically giving a false if use a binding array and true if not using a binding array?? So moved them. your thoughts?

2. sql.inc SqlQuery function - Incorporated your funky status parameter(for the lone log.inc call). Please ensure I did it correctly; I’m a bit confused why you have it skip false audit calls, but give true audit calls?

3. sql.inc SqlInsert function - Just want to ensure you don’t see any serious issues with this code. Your thoughts?

4. sql.inc added a getSqlLastID method - Used in above SqlInsert function. This should make it easier for developers to grab the last id (via global if audit engine is turned on). Your thoughts?

Otherwise, pending further discussion and input, this stuff appears to working (so ready to commit whenever).

thanks,
brady

#29

bradymiller wrote on Monday, June 21, 2010:

hey,

Added some new commits to this git code branch to secure the interface/patient_file/summary/demographics.php script and associated scripts/functions:
http://github.com/bradymiller/openemr/commits/security_example_5
http://github.com/openemr/openemr/network

Note the last commit is two bug fixes for the openemr/library/sql.inc functions.
The other commits secure the scripts and functions associated with the demographics.php script. Required extensive hardening of the options.php.inc library.

This mechanism is testing very well. Would be nice for other to test it out, considering the extensive changes (especially in the sql.inc functions).

-brady

#30

bradymiller wrote on Tuesday, June 22, 2010:

hey,

I think this stuff is ready to be committed, so can then begin the code walk trhough. I posted a tracker item with patches for those whom aren’t using git:
http://sourceforge.net/tracker/?func=detail&aid=3019490&group_id=60081&atid=1245239#

For git users, most recent branch is here:
http://github.com/bradymiller/openemr/commits/security_example_6

Note the code in branch is same as above security_example_5 branch, except just reorganized using interactive rebase to make it more clear.

Here are following commit/patch descriptions:
0001: Conversion to of sql.inc functions to adodb to allow binding (to avoid sql-injection).

0002: Added option for global magic quote removal in globals.php script, which is controlled by a per script flag. (to simplify escaping)

0003: Secured language gui scripts:
1) Turned off the fake register globals flag
2) Turned on the global magic quote removal flag
3) Removed custom escaping
4) Incorporated binding into pertinent sql calls (prevent sql-injection)
5) Incorporated htmlspecialchars (prevent xss attacks)

0004: Secured demographics.php script along with related scripts and pertinent functions.
The related scripts are patient_picture.php and stats.php and pnotes_fragment.php.
The related functions are in options.inc.php and patient.inc and pnotes.inc.

This stuff is testing well with and without magic quotes. I’d like to commit this and get the security walk through underway (pretty much following the above steps listing in commit 3 above in all scripts)

-brady

#31

bradymiller wrote on Wednesday, June 23, 2010:

hey,

I just committed this stuff. Will show up in the cvs demo in the am (you should notice no differences, except that some sql-injection and xss attacks will no longer work). Time to commence with the walk-through; I’ll fully document the process of securing/cleaning up a script in the wiki when have time, but for now just basically follow what is done in the patch/commit 0003 or patch/commit 0004 above (0003 is what happens in a new script that is already converted to the library/formdata.inc.php functions, while 0004 is what happens in the older scripts that hasn’t been fully converted to the library/formdata.inc.php functions). This requires going through a script line by line fixing it and fixing also fixing the scripts and functions it calls. Stay away from the smarty stuff for now.  I’m thinking would be best to simply hit the most commonly used scripts first.  Andy, if you plan to help out, just let me know what you’re doing so we don’t waste time doing the same thing (goes for anybody else that wants to help out).

-brady

#32

visolveemr wrote on Wednesday, June 23, 2010:

Hi Brady,

We’ll look into the changes to the files" adodb.inc" and “sql.inc” and provide our views soon.

Thanks
ViCarePlus Team

#33

atlanticcan wrote on Thursday, June 24, 2010:

Hey Brady, will version 4.0’s release be delayed until these changes are made? If so can you provide a ballpark estimate of when it will be ready? I am trying to determine whether I should focus on 3.2 or if version 4.0 will be ready soon enough.

Thanks,
Marc

#34

bradymiller wrote on Friday, June 25, 2010:

hey,
This won’t delay the next release. We’re considering releasing 4.0 early September. 4.0 is still buggy, and the upgrade from 3.2 to 4.0 will be clean, so unless you really need a feature in 4.0, I’d rec going with 3.2.
-brady

#35

bradymiller wrote on Monday, June 28, 2010:

Patch to secure OpenEMR 3.2 from David Shaw’s reported cross-scripting security vulnerability is included in
the patch here:
http://www.openmedsoftware.org/wiki/OpenEMR_Patches

I am including this message here, because this thread is referred to in their public security vulnerability report.
( http://archives.neohapsis.com/archives/fulldisclosure/2010-06/0524.html )
( http://secunia.com/advisories/40264 )

-brady

#36

bradymiller wrote on Saturday, July 03, 2010:

hey,

In order to keep the momentum going on this project, just checked in an extensive commit to secure the patient history module and the encounter listing screen. This involved a systematic optimization of the library/options.inc.php script, which is now pretty much unbreakable from a sql-injection,xss standpoint (of course, still many more scripts left). Note I also removed the encounters_full.php since this script was deprecated, not used, and no longer was functional:
http://github.com/openemr/openemr/commit/a4817af442d569525b24129ed75afa915030a4dd

Other previous commits are as follows:
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

Again, to repeat, this stuff involves the following steps:
1) Turn off the fake register globals flag
2) Turn on the global magic quote removal flag
3) Remove all custom escaping
4) Incorporated binding into pertinent sql calls (prevent sql-injection)
5) Incorporated htmlspecialchars (prevent xss attacks)

-brady

#37

bradymiller wrote on Sunday, July 04, 2010:

hey,
Transaction module has been “cleansed”:
http://github.com/openemr/openemr/commit/f56f469c9d2481f3d440c79db1917e0a38f076a9

Also, here’s an academic paper on OpenEMR’s security vulnerabilities. Nicely written:
ftp://ftp.csc.ncsu.edu/pub/tech/2010/TR-2010-1.pdf

-brady

#38

bradymiller wrote on Tuesday, July 06, 2010:

Immunization module has been “cleansed”:
http://github.com/openemr/openemr/commit/5d06c6f08d04405a80b036810a8523a7cb680a31

-brady

#39

anonymous wrote on Wednesday, July 07, 2010:

Hi Brady,

There is a lot of information in this discussion thread. Can you post a summary of what needs to be done for codes that we are working on changing? Thanks.

#40

bradymiller wrote on Wednesday, July 07, 2010:

Thomas,

Placed on wiki here:
http://www.openmedsoftware.org/wiki/Active_Projects#PLAN

It’s pretty basic, so your developers should be able to catch on quickly. I also converted one of your recent code submissions to make it easier for them to learn.

-brady