perl5-dbi / DBD-ODBC

DBD module interfacing the ODBC databases
4 stars 14 forks source link

odbcconnection Trace emits Password #9

Open theory opened 6 years ago

theory commented 6 years ago

While running diagnostics as requested for #8, I noticed that odbcconnection tracing emits the password in plain text. Example:

use DBD::ODBC;
DBI->trace(DBD::ODBC->parse_trace_flags('odbcconnection'));
my $dsn = 'dbi:ODBC:Server=example.snowflakecomputing.com;Driver=Snowflake';
my $dbh = DBI->connect($dsn, 'sqitch', 'HIDE ME!', {
    PrintError        => 0,
    RaiseError        => 1,
    AutoCommit        => 1,
    odbc_utf8_on      => 1,
});

The output:

non-Unicode login6_sv
dbd_db_login6
    SQLDriverConnect 'Server=example.snowflakecomputing.com;Driver=Snowflake;UID=sqitch;PWD=HIDE ME!', 'sqitch', 'xxxx'

So it does the right thing for the password argument to connect, but its inclusion in the ODBC DSN is unfortunate.

mjegh commented 6 years ago

Yup, well spotted. I'll take a look.

mjegh commented 6 years ago

The ODBC out connection string being dumped with the password. Also, when the UID and PWD are added to the DSN it is visible when SQLDriverConnect args are traced.

DBI_TRACE=DBD perl -MDBI -Iblib/lib -Iblib/arch -le 'my $h = DBI->connect("dbi:ODBC:DSN=SFSQLSrv2017","test","test");'
non-Unicode login6_sv
dbd_db_login6
    SQLDriverConnect 'DSN=SFSQLSrv2017;UID=test;PWD=test', 'test', 'xxxx'
Out connection string: DSN=SFSQLSrv2017;UID=test;PWD=test;SERVER=172.20.6.18;DATABASE=test;ServerName=Microsoft SQL Server;DPrec=6;FPrec=6;
    !!dbd_error2(err_rc=1, what=db_login6/SQLConnect, handles=(17b7130,17b7720,0)
    !SQLError(17b7130,17b7720,0) = (01000, 5701, [unixODBC][Easysoft][SQL Server Driver][SQL Server]Changed database context to 'test'.)
    !SQLError(17b7130,17b7720,0) = (01000, 5703, [unixODBC][Easysoft][SQL Server Driver][SQL Server]Changed language setting to us_english.)
Turning autocommit on
DRIVER_ODBC_VER = 03.81
DRIVER_NAME = libessqlsrv.so, type=0
DRIVER_VERSION = 01.09.0009
MAX_COLUMN_NAME_LEN = 128
SQL_CATALOG_NAME = 1
SQL_SCHEMA_USAGE = 31
DBD::ODBC is unicode built : NO
SQLMoreResults supported: 1
SQLDescribeParam supported: 1
    !!DBD::ODBC unsupported attribute passed (PrintError)
    setting AutoCommit
    !!DBD::ODBC unsupported attribute passed (Username)
    !!DBD::ODBC unsupported attribute passed (dbi_connect_closure)
SQLDisconnect=0
    DBD::ODBC Disconnected!
theory commented 5 years ago

Looks like this bug is still in the latest release, yes?

theory commented 5 years ago

Bah, I misread my own output, never mind. Fixed in 1.59? Close this issue? Would you prefer issues here or in RT?

mjegh commented 5 years ago

Stange, I don't remember fixing it so are you sure? I don't really mind RT or here but everyone seems to be using git now. I'll try and find a few moments to checek if it is fixed and if not sort it out. I shouldn't really have sat on this for so long as security is important. Appologies for that.

theory commented 5 years ago

Sorry, you're right, it's not fixed. It is properly redacted from the list of DBI connect params, but not where it gets appended to the DSN. From what @TallTed tells me in sqitchers/docker-sqitch#16, perhaps UID and PWD don't need to be appended to the ODBC DSN, since they're passes as arguments, as well.

mjegh commented 5 years ago

Unfortunately, not quite as simple as that as you'll have seen in my responses on squitchers. If someone uses DRIVER= in the DBI->connect string I'm forced to use SQLDriverConnect and /people/ seem to expect their username/password also passed to DBI->connect to get into SQLDriverConnect so the code appends UID=username_passed_to_dbi_connect;PWD=password_passed_to_dbi_connect to the connection string.

I think the only way to go with this is to stop tracing/logging the out connection string and log the DBI->connect(arg1) only before appending the UID/PWD. Not a difficult issue and I'll fix in the next few days. BUT, I am notoriously forgetful, so by all means hassle me over this (I really mean it). As you've already seen your original report was Aug 2018 and it is July 2019.