Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to store securely hashed sessionkeys #47

Open
wants to merge 13 commits into
base: FREESIDE_4_BRANCH
Choose a base branch
from
Open

Add ability to store securely hashed sessionkeys #47

wants to merge 13 commits into from

Conversation

wwhipple
Copy link

Freeside currently stores sessionkeys on the server in plaintext. Storing them as salted hashes would help be more PCI-compliant.

This pull request implements the ability to optionally store sessionkeys as salted SHA256 hashes.

It is implemented as follows:

  • A new hashsalt key is added to FS::Conf. A zero-length hashsalt value causes freeside to behave unchanged--no hashing takes place.
  • If the hashsalt is present as a string of length > 0, hashing is enabled.
  • Cookies continue to be stored on the browser in plaintext.
  • If the field name is 'sessionkey' (specified in the "our" variable @hashed_fields in access_user_session.pm and visible in FS::Record), qsearch will search the server's database for the salted/hashed version of the sessionkey (but only when configured on FS::Conf ...)
  • Similarly, when adding new sessionkeys to the database (FS::Record) will hash the value as it is inserted into the database.
  • One other non-related change: In Encrypt before the database, I rearranged the tests to test $conf_encryption before checking for the definition of ...::encrypted_fields.

First-time Transition to using the hashed sessionkey:
On an installation that includes this patch:

  • The administrator specifies a hashsalt value.
  • The next time s/he sends a cookie to the server, it will say "Bad Cookie" and force a new login. (This also applies to any users that might be logged in to Freeside.)

FS/t/Record.t test case:

  • We modified the test case for verification on our installation.
  • On an unattended run of the test suite, the result should be the same as the existing test, verifying that FS::Record loads.
  • If run as the freeside user and a valid adminuser is specified (as prompted in the USAGE note), 10 additional tests will test the insertion and retrieval of the sessionkey with and without the salted hash.

Weldon Whipple and others added 5 commits July 30, 2015 14:57
Add configurable option for storing the session key (in the
access_user_session table) as a salted SHA256 hash) based on setting
in the configuration.

If the 'hashsalt' configuration setting is a string of positive
length, use the salt string as input to the hash stored as the
sessionkey.

After setting the hashsalt, you will need to restart Apache,
freeside-queued, etc. You will also need to close and reopen your
browser and flush your cookies.
@freeside
Copy link
Owner

freeside commented Sep 9, 2015

If session keys stored as salted hashes is required for PCI compliance (do you have a specific citation?), then it should be the default, not an option, and it should be enabled automatically on upgrade, without administrator intervention. (Since 4.0 hasn't released yet and the 3.x -> 4.x upgrade is a major version upgrade where we're not trying to preserve web interface sessions, we're not worried about existing sessions.)

With regard to the change itself:

  • The FS::Conf description should note that changing the salt will force a new login for all sessions in progress
  • Use sha512_hex, not sha256_hex.
  • For testing: you shouldn't add a new way to test that requires site-specific configuration to use. Instead, see http://www.freeside.biz/mediawiki/index.php/Test_Suite

@oaxlin
Copy link
Contributor

oaxlin commented Sep 10, 2015

https://www.pcisecuritystandards.org/documents/PCI_DSS_v3-1.pdf
8.2.1 Using strong cryptography, render all authentication credentials (such as passwords/phrases) unreadable during transmission and storage on all system components.

While not specifically named, a session is a type of "authentication credential".

@ivanfreeside
Copy link
Contributor

Hello folks,

We're doing 4.0 release preparations, and as a big change, if this doesn't make 4.0 it won't be in until 5.x. Willl you be able to look into the feedback shortly? If you want us to do this work instead, please let us know.

@wwhipple
Copy link
Author

Ivan,

I'll start working on the securely hashed session keys today.

I apologize for the delay.

Later ...

Weldon Whipple

On Wed, Dec 9, 2015 at 8:12 PM, Ivan Kohler [email protected]
wrote:

Hello folks,

We're doing 4.0 release preparations, and as a big change, if this doesn't
make 4.0 it won't be in until 5.x. Willl you be able to look into the
feedback shortly? If you want us to do this work instead, please let us
know.


Reply to this email directly or view it on GitHub
#47 (comment).

@wwhipple
Copy link
Author

Just pushed modifications to ssh://[email protected]/wwhipple/Freeside.git,
branch feature/hash-sessionkey, per your comments of Sep 8.

Reverted t/Record.t back to its original state.

Studying http://www.freeside.biz/mediawiki/index.php/Test_Suite ...

On Wed, Dec 9, 2015 at 8:12 PM, Ivan Kohler [email protected]
wrote:

Hello folks,

We're doing 4.0 release preparations, and as a big change, if this doesn't
make 4.0 it won't be in until 5.x. Willl you be able to look into the
feedback shortly? If you want us to do this work instead, please let us
know.


Reply to this email directly or view it on GitHub
#47 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants