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 displayed in cleartext on Result instance #1049

Open
richiethomas opened this issue May 22, 2019 · 6 comments · May be fixed by #1334
Open

Password displayed in cleartext on Result instance #1049

richiethomas opened this issue May 22, 2019 · 6 comments · May be fixed by #1334
Milestone

Comments

@richiethomas
Copy link

richiethomas commented May 22, 2019

Hey there, question for the maintainers. I recently used the Mysql2::Client class to fetch some results from my app's database, and I discovered that the database password is displayed in cleartext in the Mysql2::Result instance's query_options instance variable. I reproduced the issue with a throwaway Rails app below:

irb(main):004:0> client = Mysql2::Client.new(:host => "localhost", :username => "foobar", password: 'xyz')

=> #<Mysql2::Client:0x00007f9ca0aac398 @read_timeout=nil, @query_options={:as=>:hash, :async=>false, :cast_booleans=>false, :symbolize_keys=>false, :database_timezone=>:local, :application_timezone=>nil, :cache_rows=>true, :connect_flags=>2148540933, :cast=>true, :default_file=>nil, :default_group=>nil, :host=>"localhost", :username=>"foobar", :password=>"xyz"}>

irb(main):005:0> client.select_db('tempapp_development')

=> "tempapp_development"

irb(main):006:0> results = client.query("SELECT * FROM foobars WHERE name='foo'")                         
=> #<Mysql2::Result:0x00007f9ca0a761f8 @query_options={:as=>:hash, :async=>false, :cast_booleans=>false, :symbolize_keys=>false, :database_timezone=>:local, :application_timezone=>nil, :cache_rows=>true, :connect_flags=>2148540933, :cast=>true, :default_file=>nil, :default_group=>nil, :host=>"localhost", :username=>"foobar", :password=>"xyz"}, @server_flags={:no_good_index_used=>false, :no_index_used=>true, :query_was_slow=>false}>

irb(main):007:0> pp _

#<Mysql2::Result:0x00007f9ca0a761f8
 @query_options=
  {:as=>:hash,
   :async=>false,
   :cast_booleans=>false,
   :symbolize_keys=>false,
   :database_timezone=>:local,
   :application_timezone=>nil,
   :cache_rows=>true,
   :connect_flags=>2148540933,
   :cast=>true,
   :default_file=>nil,
   :default_group=>nil,
   :host=>"localhost",
   :username=>"foobar",
   :password=>"xyz"},            # <= cleartext password here
 @server_flags=
  {:no_good_index_used=>false, :no_index_used=>true, :query_was_slow=>false}>

=> #<Mysql2::Result:0x00007f9ca0a761f8 @query_options={:as=>:hash, :async=>false, :cast_booleans=>false, :symbolize_keys=>false, :database_timezone=>:local, :application_timezone=>nil, :cache_rows=>true, :connect_flags=>2148540933, :cast=>true, :default_file=>nil, :default_group=>nil, :host=>"localhost", :username=>"foobar", :password=>"xyz"}, @server_flags={:no_good_index_used=>false, :no_index_used=>true, :query_was_slow=>false}>

I'm curious as to why this parameter is displayed in cleartext, as opposed to obfuscated when output to the Rails console.

Related question- it makes sense why it's listed among the @query_options of the Client class, but I'm curious why it's also listed on the Result class.

I trust there's a valid reason, and as a journeyman engineer I'm seeking to learn what that is, to better understand how this gem works. Thanks for building and maintaining this gem, I can only imagine how work it is to manage an open-source project of this scale.

@richiethomas
Copy link
Author

richiethomas commented May 23, 2019

Follow-up question- would y'all accept a PR which overrides the inspect methods for the Client and Result class to something like the following:

<Mysql2::Result:0x00007f9ca0a761f8
 @query_options=
  {:as=>:hash,
   :async=>false,
   :cast_booleans=>false,
   :symbolize_keys=>false,
   :database_timezone=>:local,
   :application_timezone=>nil,
   :cache_rows=>true,
   :connect_flags=>2148540933,
   :cast=>true,
   :default_file=>nil,
   :default_group=>nil,
   :host=>"localhost",
   :username=>"foobar",
   :password=>[FILTERED]},      # <= 'query_options' itself is unaffected, but redacted when Result is stringified
 @server_flags=
  {:no_good_index_used=>false, :no_index_used=>true, :query_was_slow=>false}>

This issue actually came up in our production codebase recently, where an engineer copy/pasted a Result object from the prod database into a GitHub comment without realizing that it contained the cleartext password. This in turn triggered emails which included the comment (and therefore the cleartext prod DB password) to be sent to multiple people watching the GH repo.

I could also see this issue coming up if someone's repo is outputting these two classes to a logfile or 3rd-party logging service somewhere.

Assuming there are no dependencies on the string-ified classes containing the password (and I can't imagine there would be), it seems like a PR to address this would be low-hanging fruit.

@sodabrew
Copy link
Collaborator

Aha, great explanation of the problem. Changing a password is not useful after the initial connection. I think the sensible thing to do is have the C code internalize the password, or even remove it from memory entirely, once it's part of the MySQL connection structure.

Rather than overriding #inspect, I'm thinking to redact or even remove the password field from the struct entirely.

@richiethomas
Copy link
Author

I think the sensible thing to do is have the C code internalize the password, or even remove it from memory entirely, once it's part of the MySQL connection structure.

I'm not a C engineer so I don't have context on the time it would take to accomplish this. Is this issue time sensitive, given its security implications? And if so, would overriding inspect be a viable short-term workaround until the solution you mentioned is implemented? I'm not married to the solution I suggested by any means, just wondering if a two-part, "immediate hotfix" + "long-term solution" would be useful in this case.

@richiethomas
Copy link
Author

richiethomas commented May 23, 2019

Also, in retrospect- I feel I should have submitted this issue as a private message or otherwise followed Responsible Disclosure guidelines. My apologies if that's the preferred path for issues like this. Would emailing you directly on the email on your GH profile have been appropriate in this case?

@sodabrew
Copy link
Collaborator

I'm not a C engineer so I don't have context on the time it would take to accomplish this.

Not hard.

Is this issue time sensitive, given its security implications.

No.

I feel I should have submitted this issue as a private message or otherwise followed Responsible Disclosure guidelines.

In this case no. The password is not leaked externally, and not based on input from an external user. This is in the realm of user error.

That said, I consider this user error to be due to a "sharp edge" on this tool, and will be glad to prioritize filing off this sharp edge.

@sodabrew sodabrew added this to the 0.6.0 milestone Nov 27, 2019
flavorjones added a commit to flavorjones/mysql2 that referenced this issue Sep 21, 2023
to avoid leaking credentials in exceptions via #inspect

Closes brianmario#1049
@flavorjones flavorjones linked a pull request Sep 21, 2023 that will close this issue
@flavorjones
Copy link
Contributor

@sodabrew I've opened #1334 following your guidance above to remove the credentials from the data structure.

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 a pull request may close this issue.

3 participants