User secret needs to be encrypted

[elliot-sawyer]

My initial work with this module used phpseclib's AES to encrypt the user secret, but it was removed for the demo. Future releases should reinstate this functionality

[chillu]

What would you use as the encryption key? Presumably you don't have the user's password in the same PHP session (previous HTTP request). Would this require a permanent key that's autogenerated by the framework? That's something which could come in handy in other contexts as well, but we don't do yet. CWP actually has a few stable random salts in the environment config (generated on instance creation?) which aren't documented, but could be used in this particular context.

/cc @Firesphere

[elliot-sawyer]

I wouldn't generate it on the server at all. During my initial work I generated 32 random hexadecimal characters with random_bytes. You could also use half of a SHA256 hash generated with /dev/urandom. I stored this output as a constant in .env during development and referenced it with Environment::getEnv()

Naturally using a .env is not suitable for a production environment... even as an environment variable, you would need to restrict the authenticator to a single AES key for encryption and decryption. If the key is compromised, you'll have to reset all of the user secrets.

A better approach is a key-derivation algorithm like PBKDF2. The key is derived from the user's password and used to decrypt the secret we've stored in the database. phpseclib supports this, and you could decrypt the secret specifically for the one-time use of checking the TOTP token.

[elliot-sawyer]

Also worth mentioning, phpseclib is not a dependency for this project. At the time I preferred it for it's reasonably well-documented API and pure PHP implementation. If there's a better library to use, I'm open to suggestions. Defuse might be an option

[ScopeyNZ]

libsodium is bundled with PHP7.2, has an extension on PECL for earlier versions and a compat library written in PHP: https://github.com/paragonie/sodium_compat

Sodium provides key derivation operations (that aren't specifically PBKDF2): https://download.libsodium.org/doc/key_derivation

[chillu]

A better approach is a key-derivation algorithm like PBKDF2. The key is derived from the user's password and used to decrypt the secret we've stored in the database

Same comment as before ;) Presumably you don't have the user's password in the same PHP request (only previous HTTP request). And we're not looking at storing the user's password in the PHP session, right? I'm assuming that the user flow is "first submit password, if that passes you get to the MFA screen". Maybe we can do that in the same HTTP request, but that feels like a fairly large architecture and UX change at this point @Firesphere?

[chillu]

I guess you could derive the key from the user's password during the password login request, store that in a PHP session, and then use it in a potential subsequent MFA request? Since PHP session data is stored unencrypted on the local filesystem by default (with a direct mapping to the user identifier), that's not ideal - but better than storing their password.

I want to make this module part of the default installer eventually, to provide secure options by default. Which would mean it can't really break semver in the 4.x release line, by either requiring new PHP modules, or a new PHP version (e.g. 7.2+).

[elliot-sawyer]

There's a polyfill for libsodium if you aren't running 7.2: https://github.com/paragonie/sodium_compat

Re: storing the key in the session, it's no less secure than a user sending their password over HTTPS to do the authentication... it's only used to recompute the blowfish hash stored in the database. A derived key would work the same way as a hash - as long as it can't be reversed back to the original password, there's no danger in storing it temporarily. At any rate, you should unset or zerofill the variable when finished to destroy it from memory.

[elliot-sawyer]

Just noticed that @ScopeyNZ mentioned the polyfill above!

[ScopeyNZ]

The only consideration would be the disclaimer that comes with that polyfill:

https://github.com/paragonie/sodium_compat#important

[elliot-sawyer]

Is CWP keen to fund an audit?

[Firesphere]

And we're not looking at storing the user's password in the PHP session, right? I'm assuming that the user flow is "first submit password, if that passes you get to the MFA screen".

We do at the moment, but not for any specific reason other than that it's storing the first submission entirely. But I've considered changing this to just the SecurityID and BackURL (the latter being the reason of storing it in session for the duration of the login flow)

Maybe we can do that in the same HTTP request, but that feels like a fairly large architecture and UX change at this point @Firesphere?

Unless you want to keep the password in the session, that's close to not doable. Putting it in a single request basically undermines the concept of MFA

[elliot-sawyer]

Something like this might work:

1. User authenticates with email + password
2. If authentication is successful, derive the key from the user's password and store it in the session.
3. Redirect to the "middle"/second-factor screen
4. Decrypt the secret with the user's derived key.
5. Generate TOTP token with it
6. Verify the token
7. No matter the outcome, delete the derived key from memory.

For added protection on step 2 you could encrypt the derived key with a separate AES key stored in an environment variable specifically for encrypting and decrypting the derived keys used for authentication.

[ScopeyNZ]

Is CWP keen to fund an audit?

Here's the issue that was raised at one point for this. https://github.com/paragonie/sodium_compat/issues/8

The third comment indicates the expected cost (in USD) - 30k to 50k.

I can't speak for CWP but there were a bunch of big names that weren't keen to fund this despite apparently using it themselves.

Something like this might work:

...

For added protection on step 2 you could encrypt the derived key with a separate AES key stored in an environment variable specifically for encrypting and decrypting the derived keys used for authentication.

I think this sounds good. The extra encryption seems pointless given you have an appropriate key derivation method.

[elliot-sawyer]

I agree, if an attacker has access to the session data, they would probably have access to the AES key too which makes the extra encryption pointless. I think the best approach is to minimise the time that the derived key exists in memory. Generate it once, verify, then immediately remove it from the session.

The module will need to account for the user resetting or changing their password. If the user changes it with the original password, we can re-encrypt the secret. What should we do about lost password resets? Force users to use a backup token, or contact a CMS administrator?

[chillu]

I'm starting to think this might be overkill, given the constraints discussed above. So the worst case scenario here is that TOTP secrets are leaked via reflected SQL injection, which then simplifies a potential attack to "only" guessing the user's password - which is still strongly hashed+salted even if it's leaked. So in effect, it makes attacks just as hard as in our current login system, just negates the second factor (TOTP).

Can anyone think of another reason we might need lib_sodium in core? Or can we use encryption that's built-into PHP 5.x, rather than relying on key derivation?

[elliot-sawyer]

Security is hard to get right, but I don't think the effort is overkill. Encrypting the database secret is essential, and we should make the extra effort to make sure it's done. My comment above applies only to encryption of the derived key in the session data.

I don't think we should support 5.x at all since its EOL at the end of the year. There's no native encryption support for PHP 5.x as far as I'm aware, so you'd need to use the mcrypt or openssl extensions. Since mcrypt is abandoned and deprecated as of 7.1, OpenSSL would be my preferred choice. It offers openssl_pbkdf2 and openssl_encrypt / openssl_decrypt to do AES.

hybridsessions uses AES in a similar manner, so it would be good to use that approach for consistency.

[chillu]

Same comment as on https://github.com/silverstripe/cwp-core/issues/51: hash_pbkdf2() is available with sha512 on PHP 5.6, so I don't see a reason to add an ext-openssl dependency to any hosting environments which want to use this module. Note that the hybridsessions module is not very common, it's mostly used in CWP under Active DR modes. Unless I'm missing something, and openss_pbkdf2 is considered a more secure/solid implementation compared to hash_pbkdf2?

I'm concerned about deriving the TOTP secret from the user's password, because my understanding is that any password change would change the TOTP secret, and require the user to set up TOTP again. That's bad UX, and a side effect of the implementation rather than a security design feature.

Talked with @Firesphere here's the options we see. We both grudgingly prefer Option C, although it'll be a pain for developers.

Option A: Derive key from user password

• Pro: Provides protection against SQL injection
• Con: Set up TOTP again on both forgot password and change password
• Con: Reduces forgot password flow to single factor (can’t use TOTP to authenticate)

Option B: Encrypt key with user password

• Pro: Can re-encrypt with new user password on “change password”
• Pro: Provides protection against SQL injection
• Con: Set up TOTP again on forgot password
• Con: Reduces forgot password flow to single factor (can’t use TOTP to authenticate)

Option C: Encrypt key with base secret in environment

• Pro: Independant from forgot or change password flows
• Pro: Provides protection against SQL injection (unless coupled with directory traversal or code execution)
• Con: If TOTP is integrated into core, it requires the composer create command to generate base secrets
• Con: Makes environments less portable (e.g. prod to uat will require sync of base secrets)
• Con: More work in infrastructure: Sync and expose base secrets in UI
• Con: More documentation work for platform environments
• Con: Harder to migrate from/to platforms
• Note: For platforms under our control, Ingo tends towards syncing of secrets between environments (one secret per stack rather than per environment), but there's probably security guidance preventing secret sharing in that fashion.
• Note: MFA auth should not be required in development mode, developers can use system-wide admin account in environment config (not recommended in UAT or prod)

Option D: Encrypt key with base secret in database

• Pro: Portable databases without environment constants
• Con: Does not add protection on SQL injection

Option E: Don’t encrypt secret

• Pro: Portable
• Con: SQL injection compromises TOTP, leaving password protections in place (down to single factor)

[elliot-sawyer]

Sorry but hash_pbkdf2 will only derive a key by generating a salted hash of the password. It doesn't do encryption and decryption... you still need AES to do that. That's going to require OpenSSL or MCrypt for PHP 5.6.

We don't actually need to use PBKDF2 to derive the password - any hashing function could do the job just as well. A SHA256 hash of the user password is irreversible, and provides a 64-character key. This key is what you use to encrypt and decrypt the ciphertext

[elliot-sawyer]

because my understanding is that any password change would change the TOTP secret

That's not quite accurate. A changed password would require you to decrypt the TOTP secret with the old key, and re-encrypt it with the new key. You can do this, because the CMS requires the old password before you can change it; the original secret is not lost. A lost password would, out of necessity, cause you to lose the secret. If an attacker can gain access to your email address and successfully reset the password, they can bypass 2FA.

I can understand the reasoning, but @Firesphere 's bootstrap module has already included backup codes for authenticating in the event that the second factor is inaccessible. This would include password resets: a user that has lost their password can reset it and use one of the backup codes generated for them when they first activated 2FA. If they provide a valid backup token, you can generate a new secret and encrypt it with a key derived from their new password.

If they don't have a backup token or a second factor, why would you authenticate them? It defeats the purpose of using 2FA in the first place. The authenticator should rightly deny access and force the user to contact a CMS administrator to set up 2FA again. It's bad user experience, but so is a compromised account.

[elliot-sawyer]

I would rule out options D and E immediately because they compromise security of the secret.

Options A and B seem to be doing the same thing - encrypting/decrypting derived from something the user has typed in. Any hashing function can create an encryption key from the user's password. If the password is lost and reset, the backup codes can reset the secret, or a CMS user can do it for them. This is my preference.

if A/B are still in the too hard basket, Option C is what I originally implemented for the module. Set a randomly generated key as an environment variable, and use that to encrypt and decrypt every secret.

Options A, B, and C all have a dependency on an extension or library that will perform encryption with AES, and I cannot see any way around this.

[clarkepaul]

If they don't have a backup token or a second factor, why would you authenticate them? It defeats the purpose of using 2FA in the first place.

We wouldn't authenticate them, well its not part of the UX/UI anyway.

For me option A,B are not good options, this is not standard practice from what I've experienced and an inconvenience for users. Our audience is not your typical developer who is experienced with the reasoning behind these practices and would fall back to password changes a fair bit.

With options D&E I can't really comment on because I don't really understand the security implications totally but sounds un-secure and probably wouldn't pass proper scrutiny? leaving C as per @elliot-sawyer .

With option C how much more effort is it, is this not achievable?

[elliot-sawyer]

Aside from doing nothing, option C is the lowest amount of effort. We just need to agree on a library or extension to handle the encryption.

[elliot-sawyer]

Suggestions for AES encryption:

• libsodium extension
  • not an option for PHP 5.6. A polyfill is available for < PHP7.1
• PHP Defuse
  • recommended by ParagonIE. Not formally audited
• phpseclib
  • pure-PHP, no dependencies on extensions.
• OpenSSL extension
  • runs on PHP5.6
• MCrypt extension
  • runs on PHP5.6. Abandoned, and deprecated in PHP 7.1

[elliot-sawyer]

Apologies, I cannot find an audit of DefusePHP - the previous link was to a security audit of someone else's project that was performed by the lead dev

[elliot-sawyer]

@chillu / @Firesphere / @ScopeyNZ : to avoid dependencies, what do you think about optional encryption?

1. If libsodium is loaded, encrypt/decrypt with AES
2. If openssl is loaded, encrypt/decrypt with AES
3. If mcrypt is loaded, encrypt/decrypt with AES
4. If none of these methods is available, we store and read the secret as plaintext

The key is still kept in the environment variables, but if you can't or won't support the ability to encrypt the secret the module won't force you to load extra extensions.

[ScopeyNZ]

I think this is a problem we should aim to solve for core tbh. We should offer a simple API encryption that just works - a little like what Laravel offers: https://laravel.com/docs/5.7/encryption

Security::encrypt?

I like the idea of progressively falling back but I think it's reasonable to throw an exception if we can't even use mcrypt. With a core solution we can recommend sodium_compat in the composer json and here we can re-enforce the suggestion in the readme?

Maybe I'm a little too optimistic here but it seems like if we're rolling some crypto API it makes sense to do it for the masses (in core)

[elliot-sawyer]

What's the ETA for getting a feature like that loaded into core? It would be a great feature to have with SilverStripe but I don't think we should hold this up to wait for it. If it is introduced into the framework, we can migrate to use that feature in a later release.

OpenSSL seems to be the lowest common denominator here. I'm happy to raise a PR using openssl_encrypt and openssl_decrypt if everyone is happy with that.

[ScopeyNZ]

I think it's reasonable to try for it if we're looking to have this as a headline feature for 4.4 - and we'll be doing some work on this anyway.

[elliot-sawyer]

Is there a branch for Security::encrypt/decrypt yet? I could focus efforts on there if needed

[ScopeyNZ]

No at the moment I'm just floating the idea. Seems sane considering if we're going to be implementing some sort of encryption API here, we could just target framework@4.4 for it instead?

I think it would be good to get @chillu's input on this idea - but I know he's got a very busy week this week.

[chillu]

Aside from doing nothing, option C is the lowest amount of effort

Lowest effort for the module dev ;) But we have to factor in the effort on our platforms (CWP and SSP) to either maintain the same base secrets across environments, or create databases which aren't portable between environments. And every other hosting context in the community will have to consider this as well.

what do you think about optional encryption?

This really depends if we're bundling TOTP with the core recipe, which would increase the core system requirements. In an SSP and CWP context, we'll mandate PHP 7.1+ by the time this module is considered supported.

I think this is a problem we should aim to solve for core tbh. We should offer a simple API encryption that just works - a little like what Laravel offers

It's getting a bit OT, so created a core issue for that discussion: https://github.com/silverstripe/silverstripe-framework/issues/8578. In general, I don't see core support (4.4+) as a blocker for this module for the goals we have (release around April 2019), but in the meantime we might want to consider doing a new major release line for the module in order to keep the status quo operational without relying on unstable core dependencies.

I've created an internal ticket for our platform teams to investigate creating a base secret shared between environments

[chillu]

@elliot-sawyer said:

Options A and B seem to be doing the same thing - encrypting/decrypting derived from something the user has typed in. Any hashing function can create an encryption key from the user's password.

Option A:

• Setup: derived_key = derive_key(password); encrypted_totp_secret = encrypt(totp_secret, derived_key)
• Password change: totp_secret = decrypt(encrypted_totp_secret, old_password); new_derived_key = derive_key(new_password); encrypted_totp_secret = encrypt(totp_secret, new_derived_key)
• Password reset: new_derived_key = derive_key(new_password); encrypted_new_totp_secret = encrypt(totp_secret, new_derived_key)

So no way to make derived_key == new_derived_key. But then again, my thinking is flawed here anyway, since we would store the derived key as plaintext, which defeats its use as an encryption input (SQL injection)

Option B:

• Setup: encrypted_totp_secret = encrypt(totp_secret, password)
• Password change: totp_secret = decrypt(encrypted_totp_secret, old_password); encrypted_totp_secret = encrypt(totp_secret, new_password)
• Password reset: encrypted_new_totp_secret = encrypt(new_totp_secret, new_password)

[ScopeyNZ]

Marking as impact/high because it's really important for security.

[ScopeyNZ]

My vote's still on option C. I think that deriving the key from the password is actually not much more (or arguably any more) secure than having a key provided through environment config.

If someone manages to compromise a password - which they have probably done due to getting this far, deriving the key for user secret encryption is a relatively easy next step. On the other hand, having the key set by environment configuration means an attacker has to compromise the database (to get the encrypted secret), the users password and the environment config - perhaps through filesystem access or some sort of configuration information leak.

I'll try to flesh out the core RFC @chillu has raised this week because it would be good to get that running into 4.4 soon (if it's going to happen). In the worst case there are some other modules that already "homebrew" encryption - https://github.com/silverstripe/silverstripe-hybridsessions .