Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

odbcconnection Trace emits Password #9

Open
theory opened this issue Aug 10, 2018 · 7 comments
Open

odbcconnection Trace emits Password #9

theory opened this issue Aug 10, 2018 · 7 comments

Comments

@theory
Copy link

theory commented Aug 10, 2018

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
Copy link
Member

mjegh commented Aug 10, 2018

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

@mjegh
Copy link
Member

mjegh commented Aug 13, 2018

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
Copy link
Author

theory commented Jul 24, 2019

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

@theory
Copy link
Author

theory commented Jul 24, 2019

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

@mjegh
Copy link
Member

mjegh commented Jul 25, 2019

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
Copy link
Author

theory commented Jul 25, 2019

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
Copy link
Member

mjegh commented Jul 25, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants