fix(security): avoid password leaks on query logs

[castarco]

Motivation:

We detected that under certain conditions, passwords could be leaked through logs, when executing code such as:

const query = knex('myTable').select('*');
logger.debug({ query }) // passwords are leaked here

In our case, the logger was Pino, this problem doesn't necessarily happen with other loggers (as they might use different
serialisation methods).

Fixes:

• password fields are not leaked anymore. Fixes DB passwords can be accidentally leaked through logs #5560 .
• accepts configuration objects where the password field exists but it is non-enumerable (this might happen when lib users are trying to prevent the same previous problem, but at a higher level).

Changes:

• marks the password field as non-enumerable when possible, to avoid potential leaks through logs.
• adapts tests
• add new regression test

[castarco]

Hi @tgriesser & @kibertoad , I'm sorry for adding direct mentions, but I have the feeling that this PR fixes a serious issue, and it can be easy that it gets lost among the dozens of automated PRs produced by bots.

[coveralls]

Coverage: 92.847% (+0.01%) from 92.836% when pulling 6196921 on castarco:fix-leakable-passwords into b6d04f7 on knex:master.

[OlivierCavadenti]

@castarco Thanks for the PR by the way, the error is on the "master" branch and not in your PR.

[castarco]

Thanks @OlivierCavadenti 😃 , I was investigating now what I might have broken. It will save me some time!

[castarco]

Any news on whether this will be approved/rejected/merged? From our side it's not urgent, because we are using yarn patch in our project, but this PR fixes a vulnerability that can affect other people...

[Eomm]

@OlivierCavadenti any update on this?

[OlivierCavadenti]

@Eomm sorry for the delay.
I will release a new version as soon as possible (this week), but I need time to also check documentation.
Stay tuned :D

[daniellockyer]

Is it possible that this has broken auth in 2.5.0? We've started to get auth issues for the 2.4.0 -> 2.5.0 Knex bump PR in Ghost: https://github.com/TryGhost/Ghost/actions/runs/5505546016/jobs/10033054604?pr=17254#step:11:283

  Error: Access denied for user 'root'@'172.17.0.1' (using password: NO)
      at /home/runner/work/Ghost/Ghost/node_modules/knex-migrator/lib/database.js:176:39
      at Packet.asError (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/packets/packet.js:728:17)
      at ClientHandshake.execute (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/commands/command.js:29:26)
      at Connection.handlePacket (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/connection.js:489:32)
      at PacketParser.onPacket (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/connection.js:94:12)
      at PacketParser.executeStart (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/packet_parser.js:75:16)
      at Socket.<anonymous> (/home/runner/work/Ghost/Ghost/node_modules/mysql2/lib/connection.js:101:25)
      at Socket.emit (node:events:390:28)
      at addChunk (node:internal/streams/readable:315:12)
      at readableAddChunk (node:internal/streams/readable:289:9)
      at Socket.Readable.push (node:internal/streams/readable:228:10)
      at TCP.onStreamRead (node:internal/stream_base_commons:199:23)

[OlivierCavadenti]

Hi, can you check this @castarco ? #5629

[castarco]

I'll check. It could be related.

[OlivierCavadenti]

I'll check. It could be related.

Thanks !

[elszczepano]

Is it possible that this has broken auth in 2.5.0? We've started to get auth issues for the 2.4.0 -> 2.5.0

Yes, it looks exactly the same as in my case 😄

[castarco]

I didn't had time this past night to fix it as I was low on energy, but I was walking through the code and I already have some idea of how to fix it. Hopefully today I'll be able to post a patch.

Again, sorry for the disturbance.

[OlivierCavadenti]

I didn't had time this past night to fix it as I was low on energy, but I was walking through the code and I already have some idea of how to fix it. Hopefully today I'll be able to post a patch.

Again, sorry for the disturbance.

No problem, we all deal with our personal life, do as you can

[idandagan1]

Hi,
I used to pass the db connection password using the package "config" with custom variables from outside the app.

But after upgrading to v2.5.0, it fails with the error:

Cannot redefine property: password
TypeError: Cannot redefine property: password
    at Function.defineProperty (<anonymous>)
    at setHiddenProperty 

So what's your suggestion for injecting the password?

This is my knexfile.ts :

import * as process from 'process';

require('ts-node/register');

import config from 'config';

export interface KnexConfig {
    // knex fields
}

const originalConfig: KnexConfig = config.get('postgres');

// Create a new object with the modified password value
const postgres: KnexConfig = {
    ...originalConfig,
    connection: {
        ...originalConfig.connection,
        password: process.env.PG_PASSWORD,
    },
};

export default postgres;

Does it makes sense?

[Fryuni]

Just as a note, this broke our integrations tests since we now cannot reuse the same object with the configurations between scenarios.

We have to clone the object for each instantiation of knex otherwise we get SCRAM-SERVER-FIRST-MESSAGE: client password must be a string from all but the first instantiation.

Easy fix on our side, but just registering here so other might know.