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

visolveemr wrote on Thursday, October 24, 2013:

Hi Brady

Here is the fix for password check:

Adding the following changes instead of this line https://github.com/openemr/openemr/blob/master/library/auth.inc#L121

$getUserSecureSQL= " SELECT " . implode(",",array(COL_ID,COL_PWD,COL_SALT))
                   ." FROM ".TBL_USERS_SECURE
                   ." WHERE BINARY ".COL_UNM."=?";
$userSecure=privQuery($getUserSecureSQL,array($_SESSION['authUser']));

if (($_SESSION['authUser'] == $authDB['username'])&& ($userSecure[COL_PWD] == $_SESSION['authPass']))

And also adding the following changes before this line https://github.com/openemr/openemr/blob/master/library/authentication/login_operations.php#L106

$_SESSION['authPass']=$phash;

will kick out the user when their password is changed by a admin.

Please do review the same Brady.

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

yehster wrote on Thursday, October 24, 2013:

ViSolve,
Thanks taking a look at the code.
With just those changes, a user migrating from the old password storage mechanism to the new will get kicked out without knowing why as will a user changing their password for themselves as the SESSION[‘authPass’] won’t match. So we either need to update the session variable properly in those cases too, or provide a warning.

Additionally I think it’s cleaner to turn the existing query in authCheckSession into a join of users and users_secure rather than doing two queries.

yehster wrote on Thursday, October 24, 2013:

Code to be reviewed for active user check and password hash match.

Testcases I tried:
A. User with password in old format.
Executed this query through MySqlWorkbench

update users set password=sha1(“pass”) where username=‘admin’ and id=‘1’;
delete from users_secure where username=‘admin’;

Then logged in successfully.

B. Update user’s own password with Misc>password

C. Update user’s own password with Administration > Users
Not kicked out in either case (correctly)

(Prior to addition modification ins password_change, was getting kicked out as expected.)

D. Update other user’s password with Administration > Users
Admin not kicked out, but other user kicked out on next operation

E. Set other user as inactive with Administration > Users
Other user kicked out on next operation.

The parent commit in this branch is the oemr_ prefixing of the password_hash and password_salt function for PHP 5.5. compatibility if anybody wants to do a quick sanity check.

Brady,
After we’ve heard from you and if there are no other objections I can push all of these changes to the master repository. (your site ID fix, plus these two commits.)

P.S. I lettered the testcases here rather than numbering, because SF seems to mess with them through some kind of autonumbering scheme.

bradymiller wrote on Friday, October 25, 2013:

Hi Kevin,

I have faith in your password/user stuff. I’ll add the site id check stuff after some testing to ensure not breaking patient portal. (Something that will be cool to test is to also ensure the across site access vulnerability is shut down with your above password check, which I plan to try out; then with 2 layers of protection against this via the hash check and the site check).

Will plan to get all this stuff in the next 4.1.2 patch, so the earlier it gets into the master branch for testing the better.

-brady
OpenEMR

yehster wrote on Friday, October 25, 2013:

Commits pushed to master (both oemr_ prefix and session password hash verification).

bradymiller wrote on Sunday, October 27, 2013:

Hi,

Here’s the updated site checking code.

Had to make a couple changes to not break the onsite patient portal:

Is testing well and will commit it over next day or two unless any issues noted.

thanks,
-brady
OpenEMR

bradymiller wrote on Tuesday, October 29, 2013:

Hi,

Committed above multisite check commit to codebase. After some testing time in master, will plan to place these security/php5.5 related commits into a 4.1.2 patch.

-brady
OpenEMR

bradymiller wrote on Saturday, November 02, 2013:

Hi,

This stuff was included in the 4.1.2 patch 3 that was just released.

-brady
OpenEMR