Leaking database credentials

[mkenney]

Hi, I have an issue with this package leaking database credentials. There's this line:

goracle/drv.go

Line 535 in ea928ad

User: url.UserPassword(P.Username, P.Password),

which seems like a really, really bad idea. Once you put the password into the package it should be impossible to get it back out (within reason, it should at least be explicitly intentional). It's pretty common in go do do something like

if nil != err {
    log.Error(err)
}

which has consequences in this package. Could we replace that with something like this instead?

User:   url.UserPassword(P.Username, "******"),

[mkenney]

or rather, handling it at this line since string() is likely used for other functions:

goracle/drv.go

Line 410 in ea928ad

return nil, errors.Wrapf(d.getError(), "acquireConnection[%s]", P)

[veqryn]

In addition to leaking this, Password also probably shouldn't be public/exported to begin with. That change would be a breaking change and require a constructor/new function to initialize ConnectionParams.

[tgulacsi]

Sorry, I don't really understand this.
The password is provided to goracle by You, the programmer.
The password is in the same process, so with unsafe, even unexported fields can be printed.

From who do you want to protect the password?

[mkenney]

@tgulacsi the issue is that the world is much larger than my dev box or the feature I wrote. This software runs in production systems. In cloud or PaaS infrastructure. In 3rd party systems.

Scaylr is my example here, but this applies to hundreds of situations. The logs for the service are sent to a 3rd party log aggregator as part of our standard infrastructure. An error in the Oracle database, completely unrelated to the code implementing this package, occurred which caused an issue with Ping() and BeginTx() being unable to acquire a connection and instead return an error.

In the code, and in golang in general, and in software in general, it's common to log non-fatal errors. In this case, the recovery routine contains

if nil != err {
    log.Error(err)
}

The string() method includes the password in its output. Which means the password is included in the error string. Which has caused a production oracle password, username, and in fact the full connection string, to show up in logs. In a 3rd party system. This kind of accidental data leakage is how compromises and data breaches happen in this modern world.

I want to protect the password from the entire universe, including myself, once the software is up and running. This should be everyone's philosophy for any kind of sensitive data in real world applications.

[mkenney]

Also, just for some color on the consequences for companies that run into issues like this, changing the Oracle password unfortunately is kind of a big pain in the butt for us right now.

We're in a containerized world using deployed secret stores to provide access to passwords and haven't quite implemented a manager to make this kind of thing easier (looking into Vault), so we still have to bounce each process that uses that account once the credentials are updated. Which means we needed to have a planned maintenance and downtime window for those products. It's a pretty big interruption and required the focus of half a dozen people for around 3-4 hours :(