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

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

Closed
NattyNarwhal opened this issue Apr 4, 2022 · 6 comments

Comments

@NattyNarwhal
Copy link
Member

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

NattyNarwhal commented Apr 4, 2022

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");

@NattyNarwhal NattyNarwhal changed the title Procedural and PDO ODBC don't escape user input when building Procedural and PDO ODBC don't escape user input when building connection string Apr 4, 2022
@cmb69
Copy link
Member

cmb69 commented Apr 5, 2022

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

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

cmb69 commented Apr 5, 2022

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

NattyNarwhal commented Apr 5, 2022

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

cmb69 commented May 27, 2022

Closed via 2920a26.

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

Successfully merging a pull request may close this issue.

2 participants