Security issue: can change ?site=foo to ?site=bar and be logged into another site

system · October 16, 2013, 8:37pm

chriskuhar wrote on Wednesday, October 16, 2013:

I am not sure if this is the right way to fix the issue, but this is an issue.

This pertains to a server with more than one site, and a user is logged into one site, by changing the URL, they have access to another site.

In the browser navigator bar, there is the site URL which contains the site url, for example, this goes for any URL.

http://emr.mysite.com/interface/main/main_screen.php?auth=login&site=foo

Now change the URL to another site

http://emr.mysite.com/interface/main/main_screen.php?auth=login&site=bar

notice you are now viewing data from another site.

It would be nice to have this fixed, below is my fix, but maybe someone with a better understanding of the system has a better one?

The idea is to check the site value in the url, check it against the $_SESSION value, if it is different, clean the session out and send user to login page.

macky{ckuhar}2020: git diff globals.php
diff --git a/interface/globals.php b/interface/globals.php
index 9fa70ec…4270f17 100644
— a/interface/globals.php
+++ b/interface/globals.php
@@ -105,6 +105,14 @@ if (empty($_SESSION[‘site_id’]) || !empty($_GET[‘site’])) {
$tmp = $_SERVER[‘HTTP_HOST’];
if (!is_dir($GLOBALS[‘OE_SITES_BASE’] . “/$tmp”)) $tmp = “default”;
}

if (!empty($_SESSION[‘site_id’]) && !empty($tmp) &&

  (strcasecmp($_SESSION['site_id'], $tmp) !== 0) &&

  !empty($_SESSION['userauthorized'])) {

    session_unset();  // clear session, clean logout

    header('Location: /interface/login_screen.php');

```
    exit;
```
}
if (empty($tmp) || preg_match(’/[^A-Za-z0-9\-.]/’, $tmp))
die(“Site ID '”. htmlspecialchars($tmp,ENT_NOQUOTES) . “’ contains invalid characters.”);
if (!isset($_SESSION[‘site_id’]) || $_SESSION[‘site_id’] != $tmp) {

system · October 16, 2013, 8:50pm

yehster wrote on Wednesday, October 16, 2013:

Another option is to make the session cookie site specific.

github.com

openemr/openemr/blob/master/interface/globals.php#L89




// This is the directory that contains site-specific data.  Change this
// only if you have some reason to.
$GLOBALS['OE_SITES_BASE'] = "$webserver_root/sites";


// The session name names a cookie stored in the browser.
// Now that restore_session() is implemented in javaScript, session IDs are
// effectively saved in the top level browser window and there is no longer
// any need to change the session name for different OpenEMR instances.
// On 4/8/17, added cookie_path to improve security when using different
// OpenEMR instances on same server to prevent session conflicts; also
// modified interface/login/login.php and library/restoreSession.php to be
// consistent with this.
ini_set('session.cookie_path', $web_root ? $web_root : '/');
session_name("OpenEMR");


session_start();


// Set the site ID if required.  This must be done before any database
// access is attempted.
if (empty($_SESSION['site_id']) || !empty($_GET['site'])) {

by using a different session name for different sites.

The top.restoreSession() javascript function would also need to be updated to use the variable cookie name.

The advantage of this approach would be that one could be logged in to two sites at the same time.

system · October 16, 2013, 8:55pm

yehster wrote on Wednesday, October 16, 2013:

Also, of note, switching sites by changing the siteID in the query string only gets you into the other site if the same username exists on both sites.

system · October 16, 2013, 9:11pm

yehster wrote on Wednesday, October 16, 2013:

Chris,
Thinking about it a little more, the site id check you are proposing can coexist with a site-specific session name. Doing both might provide some additional security in case there’s something else that’s been missed as part of the multi-site module.

system · October 16, 2013, 9:12pm

chriskuhar wrote on Wednesday, October 16, 2013:

You are right Kevin, that is the proper solution, I’ll track that solution down.

user “admin” is a default value, but a security minded person would change that.

system · October 17, 2013, 12:57am

yehster wrote on Thursday, October 17, 2013:

It’s not a slam dunk though.

github.com

openemr/openemr/blob/master/interface/globals.php#L81


}


// The webserver_root and web_root are now automatically collected in
//  real time per above code. If above is not working, can uncomment and
//  set manually here:
//   $webserver_root = "/var/www/openemr";
//   $web_root =  "/openemr";
//


// This is the directory that contains site-specific data.  Change this
// only if you have some reason to.
$GLOBALS['OE_SITES_BASE'] = "$webserver_root/sites";


// The session name names a cookie stored in the browser.
// Now that restore_session() is implemented in javaScript, session IDs are
// effectively saved in the top level browser window and there is no longer
// any need to change the session name for different OpenEMR instances.
// On 4/8/17, added cookie_path to improve security when using different
// OpenEMR instances on same server to prevent session conflicts; also
// modified interface/login/login.php and library/restoreSession.php to be
// consistent with this.

It looks like Brady actually changed away from that paradigm for various reasons a long time ago.

system · October 17, 2013, 8:10am

bradymiller wrote on Thursday, October 17, 2013:

Hi,
Very interesting (and real) vulnerability. Chris’s initial strategy seems like enough (ie. if the GET site (note empty is default) is not equal to the SESSION site, then die; I’m not sure if killing the session is needed, though?).
-brady
OpenEMR

system · October 17, 2013, 8:19am

bradymiller wrote on Thursday, October 17, 2013:

actually, looking at code in interface/globals.php looks like an empty GET site is not equivalent to default (minor error in my post above).
At the end of the current code there can the conditional to $_SESSION[‘site_id’] != $tmp be used to instead die rather than changing the $_SESSION[‘site_id’] to $tmp .

system · October 18, 2013, 12:07am

cmswest wrote on Friday, October 18, 2013:

separate issue:

once you’re setup with admin, you have to go into database and hand edit users tables to change and then would have to fix password or else there’s a login issue?

system · October 18, 2013, 12:09am

cmswest wrote on Friday, October 18, 2013:

or create new admin with unique user name and then delete username admin?

system · October 20, 2013, 8:46pm

bradymiller wrote on Sunday, October 20, 2013:

Hi,

Tested this out some and here’s a hopeful robust fix for this:

Let me know your thoughts Chris, Kevin?

Rod, since you know this multisite module best, see any obvious issues here?

Also will need to ensure it doesn’t break the patient portal along with installation/upgrading of new multisite instances.

-brady
OpenEMR

system · October 21, 2013, 4:16am

sunsetsystems wrote on Monday, October 21, 2013:

A couple of comments.

First, it used to be that both username and password were checked (via authCheckSession() in library/auth.inc) on each web access. However when RSA key pairs where introduced on 5/28, it appears this was changed so that only the username is checked, not the password. I’m wondering if this could result in other vulnerabilities? Kevin, can you comment?

So anyway, before this change you had to have the same username and password in both sites in order to get this kind of access. But in that case, you had the access anyway. So it was more of an oddity than a vulnerability. Now it’s a vulnerability.

Brady, your fix seems OK if that’s the only relevant vulnerability, however I think the above-mentioned checkSession() function would be a more appropriate place for it.

Rod
http://www.sunsetsystems.com/

system · October 22, 2013, 3:59am

yehster wrote on Tuesday, October 22, 2013:

I think that explicitly checking that the siteID matches is more important than a password check, because it happens “earlier” in the sequence of events, and will also catch an inadvertent site switch if “ignoreAuth” is true e.g. when using the portal.

system · October 22, 2013, 6:16pm

sunsetsystems wrote on Tuesday, October 22, 2013:

I agree that site ID check is good. What I’m wondering is if password check is also desirable. For example if the admin changes my password while I’m logged in, seems I could just keep merrily using the system as long as I don’t log out. Are there other such vulnerabilities?

Rod
http://www.sunsetsystems.com/

system · October 22, 2013, 6:40pm

yehster wrote on Tuesday, October 22, 2013:

I agree that adding the password check is a good idea. Just wanted it clear that it needs to be in addition to the site ID check and not instead of.

system · October 23, 2013, 4:52am

bradymiller wrote on Wednesday, October 23, 2013:

That specific example does not seem like a vulnerability, though. Just because the admin changes the password does not mean the user should be logged out(or does it??). However, if a admin disables the user, then the user should be logged out; will that happen currently in OpenEMR if a user is inactivated?
-brady
OpenEMR

system · October 23, 2013, 6:08am

visolveemr wrote on Wednesday, October 23, 2013:

Brady
If a admin disables the user,then that user is not logging out. This is not happening currently in OpenEMR.

Thanks
OpenEMR Customization/Support provider,
ViSolve Inc
services@visolve.com

system · October 23, 2013, 2:05pm

yehster wrote on Wednesday, October 23, 2013:

github.com

openemr/openemr/blob/master/library/auth.inc#L120








//----------THINGS WE DO IF WE STILL LIKE YOU


function authCheckSession()
{
if (isset($_SESSION['authId'])) {
     // If active directory was used, check a different session variable (as there is no password in database).
    if ($GLOBALS['use_active_directory']) {
        if ($_SESSION['active_directory_auth']) {
            return true;
        } else {
            return false;
        }
    }


    $authDB = privQuery("select ".implode(",", array(TBL_USERS.".".COL_ID,
                                                    TBL_USERS.".".COL_UNM,
                                                    TBL_USERS_SECURE.".".COL_PWD,
                                                    TBL_USERS_SECURE.".".COL_ID))

Adding “AND active!=0” to the query here will kick a user out.

system · October 23, 2013, 2:49pm

sunsetsystems wrote on Wednesday, October 23, 2013:

Probably best if either changing the password or inactivating denies access right away, as that’s surely the intent.

Rod
http://www.sunsetsystems.com/

system · October 24, 2013, 5:57am

bradymiller wrote on Thursday, October 24, 2013:

Hi,

So, would place the hash in a session variable then? And ensure this matches, as with the user being active, on each authCheckSession() request? This in addition to the ensure site matches sounds like would be much more secure overall. I contributed the site fix, Kevin contributed the active user fix, I wonders who’s up to contribute the password check/fix…

-brady