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 is exposed via CloneWith #2725

Closed
bgrainger opened this issue Nov 5, 2019 · 2 comments · Fixed by #2729
Labels
bug
Milestone

Comments

@bgrainger
Copy link
Contributor

@bgrainger bgrainger commented Nov 5, 2019

Calling CloneWith("Persist Security Info=true") overrides the Persist Security Info setting on an existing connection, allowing the password to be retrieved.

It doesn't seem right that an arbitrary caller can disable that setting on an existing connection.

This doesn't seem like a security vulnerability, because any code that can call that method can probably also retrieve the password via reflection. (There may be the possibility of inadvertent disclosure via logs if some component happened to be logging connection strings?)

Steps to reproduce

var connection = new NpgsqlConnection("server=localhost;user id=root;password=test");
connection.Open();

// prints Host=localhost;Username=root
Console.WriteLine(connection.ConnectionString);

var connection2 = connection.CloneWith("PersistSecurityInfo=true;");

// prints Persist Security Info=True;Password=test
Console.WriteLine(connection2.ConnectionString);

Further technical details

Npgsql version: 4.1.1
Operating system: Windows 10

@YohDeadfall YohDeadfall added the bug label Nov 5, 2019
@roji

This comment has been minimized.

Copy link
Member

@roji roji commented Nov 6, 2019

I'm not sure exactly what I think about this... As you wrote, Persist Security Info isn't actually meant to prevent anyone from programmatically retrieving the password (since they can always do that anyway via reflection). Connection strings are also never, ever meant to be simple user inputs, so it's hard to imagine where this is actually an issue.

Do you (or @YohDeadfall) have an actual scenario in mind?

@YohDeadfall

This comment has been minimized.

Copy link
Member

@YohDeadfall YohDeadfall commented Nov 6, 2019

No actual scenario, but this looks like a bad design/minor bug which should be fixed some day.

For security critical things there are environment variables, a pass file, callbacks, Kerberos, etc.

@roji roji added this to the 4.1.2 milestone Nov 6, 2019
roji added a commit that referenced this issue Nov 6, 2019
roji added a commit that referenced this issue Nov 6, 2019
@roji roji closed this in #2729 Nov 6, 2019
roji added a commit that referenced this issue Nov 6, 2019
roji added a commit that referenced this issue Nov 6, 2019
Fixes #2725

(cherry picked from commit d8ec22b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.