node-postgres: Security issue: DB passwords possibly leaking to logs

Version: "pg": "7.1.2"

We’ve seen such lines in our logs (uncaught exception handler).

{"err":{
"message":"Connection terminated unexpectedly",
"stack":"Error: Connection terminated unexpectedly\n    at Connection.con.once (PATH/node_modules/pg/lib/client.js:182:21)\n    at Object.onceWrapper (events.js:313:30)\n    at emitNone (events.js:106:13)\n    at Connection.emit (events.js:208:7)\n    at Socket.<anonymous> (PATH/node_modules/pg/lib/connection.js:75:10)\n    at emitOne (events.js:116:13)\n    at Socket.emit (events.js:211:7)\n    at TCP._handle.close [as _onclose] (net.js:554:12)",
"client":{
  "domain":null,
  "_events":{},
  "_eventsCount":1,
  "connectionParameters":{
    "user":"USER",
    "database":"DATABASE",
    "port":PORT,
    "host":"HOST",
    "password":"PASSWORD!!!111!!one!!"
    ...

Well. I hope I don’t have to explain. Also it’s kinda urgent for us.

About this issue

Original URL
State: closed
Created 6 years ago
Reactions: 12
Comments: 19 (10 by maintainers)

Most upvoted comments

The simplest solution would be to replace this line:

https://github.com/brianc/node-postgres/blob/94d38f941e9039a2b09a560bdd1410676920edbd/lib/connection-parameters.js#L57

with this:

Object.defineProperty(this, 'password', {value: val('password', config)});

i.e. make property password non-enumerable, and then it won’t show up anywhere.

vitaly-t on Feb 19, 2018

Hmm I think it’s a pretty serious issue. Even if pool.on('error', ... is used in the code, @igabesz is right, you can never be sure if the error isn’t catched and logged somewhere.

Since we operate in a very security sensitive industry, I’m very concerned with that.

abenhamdine on Feb 20, 2018

I highly recommend to reconsider this error management because right now I would not recommend this package for any serious industrial use-cases. I explain it in details.

You can never know how your exceptions will be handled. The try-catch block is not necessarily near the database operations. See this actual example: something broke and the error was caught by the uncaught error handler. (Which logged the error and then killed the whole process.) In this very case we can (and will) check the exception in the uncaught error handler if it contains client.connnectionParameters.password property before logging. But shall we do this in each and every try-catch blocks just to make sure? What if a 3rd party lib has a try-catch and some logging as well? Shall I intercept the logged texts and search for password strings inside?

You don’t know how and where your exceptions will be handled. And vice versa: one should not be afraid in their catch to log the exceptions. Furthermore: One should not be required to know if logging an error is safe or not.

And about the sensitivity of this information: In many cases even the developers won’t know the actual configuration that is being used in the production server. Maybe the whole developer organization has no access right to the production environment. But hey, some DB passwords are leaking because we logged an error object. And the developer of the package does not even consider this a serious issue.

If you need to make the .cancel() work then I suggest using a closure to access the client.

igabesz on Feb 9, 2018

Has there been any updates on this? Or is the approved solution to just not log any errors?

https://node-postgres.com/features/pooling#checkout-use-and-return This pooling documentation has an example that would in fact log these credentials, something we followed and didn’t realize the repercussions of, I feel like that shouldn’t be the default example then without heavy warning?

chris-hut on May 27, 2019

@brianc I have tested this issue in v8.0.0, and it was fixed there. This can be closed now.

vitaly-t on Mar 31, 2020

Ditto @igabesz! This is concerning

Dwaynekj on Jan 30, 2020

Sorry, I missed that this was an uncaught exception handler. Don’t let it go uncaught; add an 'error' listener to the pool. The pool is the only part of pg that will attach a client object to an error. (Then switch away from password authentication to certificates.)

And the developer of the package does not even consider this a serious issue.

I’m not the developer. Just offering advice on how to use the current version of pg considering your problem.

Even error messages aren’t as safe to log in general as you’d like them to be, though.

charmander on Feb 9, 2018

The connectionParameters property is only necessary for .cancel(), and that doesn’t work right now (#1392). Maybe it can be removed. However…

Also it’s kinda urgent for us.

Configure your error logging not to include the password property.

charmander on Feb 8, 2018