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

Possible credentials leak #105

Closed
colshrapnel opened this issue Mar 12, 2019 · 4 comments
Closed

Possible credentials leak #105

colshrapnel opened this issue Mar 12, 2019 · 4 comments

Comments

@colshrapnel
Copy link
Contributor

colshrapnel commented Mar 12, 2019

I was reached by a user concerned of the possible leak of the database credentials when an error occurs during Factory:create() call:

PHP Fatal error: Uncaught ParagonIE\EasyDB\Exception\ConstructorFailed: Could not create a PDO connection. Please check your username and password. in vendor/paragonie/easydb/src/Factory.php:77
Stack trace:
#0 0.php(10): ParagonIE\EasyDB\Factory::create('mysql:host=loca...', 'username', 'putastrongpassw...')
#1 {main}
thrown in vendor/paragonie/easydb/src/Factory.php on line 77

The problem is coming from the fact that Factory::create()'s parameters are listed in the stack trace.

I offered a user a quick and dirty solution of wrapping the call into a try catch and then re-throwing a generic exception that contains the error message from the caught exception.

But that's only a workaround and I think it would be better to change the Factory::create() method's signature. the simplest solution would be to make the method to accept an array of parameters instead of an explicit list of variables. This is against the best practices but here I would think it would be a good tradeoff between good practices and security.

I could send a pull request if you agree for this change.
Or we can try to find some other solution.

@paragonie-scott
Copy link
Member

This sounds like a duplicate of #102.

But that's only a workaround and I think it would be better to change the Factory::create() method's signature.

This is sensible, but would require a major version bump, due to a BC break.

Would adding a fromArray() method and releasing a new minor version suffice?

@colshrapnel
Copy link
Contributor Author

I am not very good in maintaining libraries, but technically - yeah, it would do.

@colshrapnel
Copy link
Contributor Author

Added #106
Not sure if it's the best way but at least it makes the transition most simple - just square braces have to be added around the list of parameters, ie

$db = Factory::create($dsn,$username,$password); => $db = Factory::fromArray([$dsn,$username,$password]);

@paragonie-scott
Copy link
Member

Your strategy is precisely what I was going to do. :)

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

No branches or pull requests

2 participants