Password displayed in cleartext on Result instance

[richiethomas]

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]

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]

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]

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]

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]

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.

[flavorjones]

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