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

Password's not encrypted for payment gateways #268

Closed
kalenjohnson opened this issue Dec 17, 2015 · 5 comments · Fixed by #1653
Closed

Password's not encrypted for payment gateways #268

kalenjohnson opened this issue Dec 17, 2015 · 5 comments · Fixed by #1653
Assignees

Comments

@kalenjohnson
Copy link
Contributor

From what I can tell, passwords for basically all gateways are saved in the database in clear text:

https://github.com/strangerstudios/paid-memberships-pro/blob/dev/classes/gateways/class.pmprogateway_paypal.php#L92-L101
https://github.com/strangerstudios/paid-memberships-pro/blob/dev/classes/gateways/class.pmprogateway_paypalexpress.php#L107-L116
https://github.com/strangerstudios/paid-memberships-pro/blob/dev/classes/gateways/class.pmprogateway_paypalstandard.php#L91-L100
https://github.com/strangerstudios/paid-memberships-pro/blob/dev/classes/gateways/class.pmprogateway_payflowpro.php#L74-L83

I was surprised when I looked in wp_options for another issue and found my username and password in cleartext for Payflow Pro, a service I have never set up. Not sure how it got in there, but in any case, if there is a password, it should really be hashed: https://codex.wordpress.org/Function_Reference/wp_hash_password

@kalenjohnson
Copy link
Contributor Author

As I pressed enter, I realized hashing would not allow you to retrieve the password, only compare against it. At least some form of encryption would still be better than clear text though.

@kalenjohnson kalenjohnson changed the title Password's not hashed for payment gateways Password's not encrypted for payment gateways Dec 17, 2015
@ajdexter
Copy link

ajdexter commented Jan 2, 2016

Confirmed on my end too. This is kinda a big deal to me.

@kalenjohnson
Copy link
Contributor Author

@strangerstudios is this just not considered a big deal?

@strangerstudios
Copy link
Collaborator

This is a complicated issue, and I'd like to write up more, but let me chime in quickly.

When looking into this and similar "should we encrypt this data" type questions, it seems to me that encrypting here would really only stop a small subset of security breaches in a "security through obscurity" kind of way.

As you've figured out, the API usernames and passwords/keys would need to be unencryptable to communicate with the API. Any security breach that got the database to download API information is likely to also have access to the decryption algorithm and keys in the wp-config.php file. And so they would be able to decrypt the keys anyway.

Still we have been thinking through ways of doing the encryption/decryption and are prepared to do the work if/when it seems like a standard practice. I don't believe other plugins that interact with APIs this way encrypt the keys. Let me know if you find out otherwise.

It should also be noted that we like to do things to make the plugin more secure, even if it's just for a subset of attacks... if there are no drawbacks. It may seem like there are no drawbacks to encrypting the API creds, but it does make things slightly more confusing for users when entering the configuration and when copying creds from one install to another or sharing them with developers/etc.

Thanks for your understanding. I'm open to discussion on this, POCs, and pull requests. Let's keep talking.

@sc0ttkclark
Copy link
Contributor

The problem outlined here with encrypting/encoding the password-like values of API keys and signatures for the variety of gateways is still valid. I haven't seen any gateways for WooCommerce or other similar commerce systems encrypting/encoding this information, only obscuring it by not outputting it or outputting it within an input that obscures the text.

As part of the work on #1653, we are nailing down a solution that would obscure the text across the board for these kinds of values in the Payment settings area.

We are always open to further ideas/discussion here to improve things but at the moment these values remain accessible to those with admin access to control the settings. You can always restrict access to roles so they have more granular access to certain settings pages without giving them access to the Payments settings using this handy list of capabilities that are supported by Paid Memberships Pro: https://www.paidmembershipspro.com/documentation/advanced/capabilities/

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

Successfully merging a pull request may close this issue.

3 participants