Security issue: DB passwords possibly leaking to logs

[igabesz]

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.

[charmander]

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.

[igabesz]

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.

[charmander]

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.

[abenhamdine]

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.

[vitaly-t]

The simplest solution would be to replace this line:

node-postgres/lib/connection-parameters.js

Line 57 in 94d38f9

this.password = val('password', config)

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.

[charmander]

message and stack are showing up despite being non-enumerable (but those are explicit exceptions, maybe?)

[vitaly-t]

@charmander yes, those are accessed explicitly, that's why.

[abenhamdine]

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

Nice ! Given the criticity of that, perhaps it would be good to have a comment in code + setting explicitly enumerable to false, to show the intent and avoid a future infortunate change.

Anyone for the PR ?

[abenhamdine]

On second thought : don't we only need to attach client to error ?

I just had a look at the code, and if IIUC the client is attached to error in pg-pool :

https://github.com/brianc/node-pg-pool/blob/4d7734a71122cbbe1bb81bb466430e96f3405394/index.js#L178

I don't see err.client used anywhere in the code (neither in pg nor in pg-pool).

So perhaps a more efficient solution should be to remove this line from pg-pool.

Besides, client is passed to the error listener in https://github.com/brianc/node-pg-pool/blob/4d7734a71122cbbe1bb81bb466430e96f3405394/index.js#L186
so if anyone needs to access the client after error, he still can do it.

Any thoughts ?

[charmander]

That sounds good, but it’s also a breaking change. Cancellation can be removed about as easily.

[abenhamdine]

but it’s also a breaking change.

Ah yes you're right.

Looks like there are 3 important PR which will need to be merged in pg-pool :

• Update CI versions node-pg-pool#88 to make Travis pass
• idleListener no longer grabs references to things it doesn't need node-pg-pool#83 to get rid of memory leak in this very code
• then afterwards a future PR to remove err.client = client

@charmander I see that you dont' have collaborator access to pg-pool. Is it something you are willing to ask @brianc for ?

Or perhaps it's more simple in the short term to make password non enumerable in connectionParameters (but I'm not totally sure if it's enough, password is also assigned directly to client instance IIRC).

Cancellation can be removed about as easily.

I haven't followed the cancellation issue, but I don't see why it would be related to this leak.

[charmander]

I haven't followed the cancellation issue, but I don't see why it would be related with this leak.

Credentials are attached to clients to support (only) cancellation, but that hasn’t worked in any pg 7.x. The existing API for cancellation is a bit misleading too. It could be removed in 7.x, then return with a new interface for 8.x – at which point the client property could also be dropped from pool errors.

[vitaly-t]

then return with a new interface for 8.x

8.x - is it even on the map? I don't see any branch for it.

[charmander]

@vitaly-t Oh, it’s not necessarily planned for an 8.x – all I mean is that it would have to be some future major version.

[chris-hut]

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?

[vitaly-t]

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

[brianc]

Awesome! Good news! :)

…

On Tue, Mar 31, 2020 at 4:34 PM Vitaly Tomilov ***@***.***> wrote: @brianc <https://github.com/brianc> I have tested this issue in v8.0.0, and it was fixed there. This can be closed now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1568 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHILWIKCZDOUZXYSCU5TRKJOVLANCNFSM4EPYP36Q> .