Procedural and PDO ODBC don't escape user input when building connection string

[NattyNarwhal]

Description

The following code:

<?php
$connection = odbc_pconnect('Driver=MariaDB;Database=test', 'foobar', 'pass;word')

Resulted in this output:

Warning: odbc_pconnect(): SQL error: [ma-3.1.13]Error while parsing DSN, SQL state S1000 in SQLConnect in /home/calvin/src/test-connection-string.php on line 3

But I expected this output instead:

<successful connection>

PDO is also affected.

I dealt with a PHP user's issue where they had a password with a ; in it. Unfortunately, the code that handles the ctor(connection string, uid, pwd) case is naive (both for procedural and PDO) and appends without any special processing. That means the connection string will be mangled (or worse, injectable) if the user application doesn't do what PHP does behind the scenes, but better themselves (do ctor("connection string;uid={uid};pwd={pwd}", null, null), whereas PHP only does ctor("connection string;uid=uid;pwd=pwd", null, null)).

Unfortunately, after reading unixODBC code, it seems clear that the responsibility of parsing these connection strings is at the driver level, so in theory every driver could be handling how to escape strings themselves. It seems the usual standard is {wrap in curly braces}, and some drivers support single quotes. There may be more escaping/quoting rules I'm not aware of (possibly covered by the ODBC standard?). The IBM i Db2 driver seems to support both, but the MariaDB driver only support curly braces.

It could also be that it's not PHP's responsibility to deal with this, but it is unfortunate to have to explain SQLConnect vs. SQLDriverConnect behaviour to people writing application code. If PHP can't escape, perhaps it could warn/error out if user input is problematic.

PHP Version

PHP 8.2.0-dev

Operating System

Fedora 35

[NattyNarwhal]

You can also use curly braces or single quotes wrapping the password in the third arg as a weak attempt to mitigate. It also gives you other fun opportunities for injection, like:

$connection = odbc_pconnect('Driver=MariaDB;Database=test', 'foobar', "{pass;word};Port=1337");

[cmb69]

Ugh, that's ugly (I don't think we should discuss details publicly, but we need to keep these in mind).

Anyhow, I'm not sure what to do, though. If we would automatically escape (if that's even possible in a portable way), that could easily break code which does the escaping manually. Adding an INI setting is undesireable (same code, different behavior). For PDO_ODBC, we could add a connection attribute, though. At the very least we should properly document the current behavior, and maybe we should introduce an additional function for escaping.

[…] perhaps it could warn/error out if user input is problematic.

So warn/error on unescaped ;? That might be the best immediate solution.

[NattyNarwhal]

So the documentation for SQLBrowseConnect and SQLDriverConnect seem to imply that braces are strongly recommended to be handled by drivers? If that's the case, I wonder if always wrapping in curly braces is OK. If so, PHP can do that, and if the user is already wrapping the value (just check first and last char if they're braces), don't touch it.

Do agree checking for ; is OK for an interim solution, but do we need to check for others?

[cmb69]

From the SQLDriverConnect docs:

Applications do not have to add braces around the attribute value after the DRIVER keyword unless the attribute contains a semicolon (;), in which case the braces are required.

So it looks like only semicolons are an issue.

[NattyNarwhal]

Looking at other ODBC bindings for other languages (Node, .NET, Erlang/OTP), I notice none of them actually use SQLConnect and instead use SQLDriverConnect (edit: I mixed these up initially). They all just take the connection string directly (and also probably assume you have to use DSN=whatever for the simple case). I wonder if deprecating the case where PHP appends a connection string could be palatable. It'd definitely be a big change for users though, and maybe be semantically confusing for PDO.

For another comparison, .NET has some facilities for building connection strings (generically and for ODBC), and actually seems to note some of the specific rules for quoting/escaping too.

[cmb69]

Closed via 2920a26.