Quote when appending username and password to the ODBC connection string #8307

NattyNarwhal · 2022-04-05T21:31:24Z

Because the UID= and PWD= values are appended to the SQLDriverConnect case when credentials are passed, we have to append them to the string in case users are relying on this behaviour. However, they must be quoted, or the arguments will be invalid (or possibly more injected). This means users had to quote arguments or append credentials to the raw connection string themselves.

It seems that ODBC quoting rules are consistent enough (and that Microsoft trusts them enough to encode into the .NET BCL) that we can actually check if the string is already quoted (in case a user is already quoting because of this not being fixed), and if not, apply the appropriate ODBC quoting rules.

This has been tested with the MariaDB and IBM i Db2 drivers.

There are still some questions:

Procedural ODBC, with code sharing?
Are we paying attention to all of the quoting rules? i.e. any other special characters? We don't need to care if ; are in the strings at least, since we always quote now.
Should we always quote? Could it break some drivers? If so, could it be an INI option?
Buffer length/truncation behaviour (since the quoted string is on stack)
How do we unit test this?

Fixes GH-8300

cmb69 · 2022-04-06T15:17:09Z

Thank you for the PR!

Some thoughts:

I think we should only quote if necessary; it is unclear, though, in which case it is necessary; the respective documentation contradicts itself:

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.

vs.

Because of connection string and initialization file grammar, keywords and attribute values that contain the characters []{}(),;?*=!@ not enclosed with braces should be avoided.

This documentation does not specify how to treat braces in quoted strings; we're likely on the safe side if we handle that according to .NET (i.e. double the braces)

we need to cater to truncation; at the very least raise a warning in that case; it might make sense to avoid truncation altogether (maybe just count the number of additional characters first, and then allocate a respective buffer)
we also need to fix this for ext/odbc; code sharing could be a bit of an issue, since that code would have to go into main/ (or some other place which is always available; consider phpize builds), although it might not be useful for other purposes
testing is also an issue; end-to-end testing would require to set up an account with a semicolon in password or username; not sure if that worth it; a unit test of the relevant functionality could be done by defining a userland function in ext/zend_tests, and testing this

NattyNarwhal · 2022-04-06T16:02:21Z

I think we should only quote if necessary; it is unclear, though, in which case it is necessary; the respective documentation contradicts itself:

Probably a good idea. I think. An odbc_should_quote can check for those characters (though I'm unaware if any outside of semicolons and curly braces are actually a problem?).

we also need to fix this for ext/odbc; code sharing could be a bit of an issue, since that code would have to go into main/ (or some other place which is always available; consider phpize builds), although it might not be useful for other purposes

Right. The code sharing potential is obvious, it just seems silly to have to put it in Zend/.

we need to cater to truncation; at the very least raise a warning in that case; it might make sense to avoid truncation altogether (maybe just count the number of additional characters first, and then allocate a respective buffer)

I had a prototype that allocated a buffer, but there'd have to be some untangling of the logic to deal with i.e. failed allocations and such.

ext/pdo_odbc/odbc_driver.c

cmb69 · 2022-04-07T10:34:32Z

ext/pdo_odbc/odbc_driver.c

 			}
 			snprintf(dsn, new_dsn_size, "%s;UID=%s;PWD=%s", dbh->data_source, uid, pwd);
 			pefree((char*)dbh->data_source, dbh->is_persistent);
 			dbh->data_source = dsn;
+			/* Convoluted label to handle freeing in this scope */
+connection_string_fail:


Yeah, that's how we usually deal with deallocation needs. It may make sense to use alloca() instead of emalloc() for performance reasons, but that would not avoid the manual cleanup, for portability reasons; see

php-src/Zend/zend_portability.h

Lines 348 to 365 in 6828f9d

#if (defined(HAVE_ALLOCA) || (defined (__GNUC__) && __GNUC__ >= 2)) && !(defined(ZTS) && defined(HPUX)) && !defined(DARWIN)

# define ZEND_ALLOCA_MAX_SIZE (32 * 1024)

# define ALLOCA_FLAG(name) \

bool name;

# define SET_ALLOCA_FLAG(name) \

name = 1

# define do_alloca_ex(size, limit, use_heap) \

((use_heap = (UNEXPECTED((size) > (limit)))) ? emalloc(size) : alloca(size))

# define do_alloca(size, use_heap) \

do_alloca_ex(size, ZEND_ALLOCA_MAX_SIZE, use_heap)

# define free_alloca(p, use_heap) \

do { if (UNEXPECTED(use_heap)) efree(p); } while (0)

#else

# define ALLOCA_FLAG(name)

# define SET_ALLOCA_FLAG(name)

# define do_alloca(p, use_heap) emalloc(p)

# define free_alloca(p, use_heap) efree(p)

#endif

I've always been hesitant to use alloca for the reasons people usually are (i.e. failure when not enough stack?). If you still have to free, it clobbers a lot of the ergonomics of it too...

Yeah, VLAs would be a nice solution, if they we're available "everywhere".

I think technically, VLAs are worse in the "failure is hard to deal with" department than alloca. It's really unfortunate C doesn't have similar facilities to C++ in terms of scoped (de)allocations/RAII.

NattyNarwhal · 2022-04-08T17:57:27Z

So regarding code sharing, where would be the best place to put it? I assume since ODBC extensions are bundled, it can be safe to put it in i.e. main/ because there's no standalone builds? The other procedural vs. PDO extensions don't really seem to do much code sharing, so I'm not sure what's the best strategy for it, or if it should be done at al. (In the case of i.e. pg, the procedural one doesn't have to worry about sanitizing credentials, whereas PDO does due to the PDO interface, doing a similar trick as PDO_ODBC)

NattyNarwhal · 2022-04-18T16:29:26Z

Sorry for the bump, still a bit unclear on what's the best place to put it still, especially for a kind of obscure piece of functionality. I guess main/ works, since I see other weird sundry functionality there, I'm just a little hesitant myself.

WRT:

testing is also an issue; end-to-end testing would require to set up an account with a semicolon in password or username; not sure if that worth it; a unit test of the relevant functionality could be done by defining a userland function in ext/zend_tests, and testing this

Exposing an API like function odbc_quote_connection_string(string $to_quote): string in procedural ODBC would be one way to test it from userspace. It might even be useful for i.e. database libraries in frameworks, even for non-ODBC databases (i.e. DB2?).

cmb69 · 2022-04-20T15:27:51Z

[…] still a bit unclear on what's the best place to put it still, especially for a kind of obscure piece of functionality. I guess main/ works, […]

Indeed, this looks like the most suitable place. The filename php_odbc_utils.c looks like a good choice either; maybe more stuff can be put in there in the future. I think the php_odbc_utils.h file needs to be exported (PHP_INSTALL_HEADERS); not sure where(i.e. in which config file) that macro should be used, though.

[…] because there's no standalone builds?

It should be possible to do phpize builds of ext/odbc and ext/pdo_odbc, and I think that might be standard for several Linux distros. Although, that shouldn't be a problem, if the functions are exported and the headers available.

Exposing an API like function odbc_quote_connection_string(string $to_quote): string in procedural ODBC would be one way to test it from userspace.

Oh, right. It might make sense to expose that function to userland.

NattyNarwhal · 2022-04-20T15:44:50Z

It seems at least in configure.ac and config.w32, all of main/*.h seems to be exported with PHP_INSTALL_HEADERS([Zend/ TSRM/ include/ main/ main/streams/]) and PHP_INSTALL_HEADERS("", "Zend/ TSRM/ main/ main/streams/ win32/");, respectively.

The thing with exposing it to userspace is the semantics. It might make sense to expose should and is quoted too, so userspace can determine it themselves. Of course, if the code's in main, should odbc or standard/etc be the ones exporting it?

NattyNarwhal · 2022-04-22T15:02:40Z

Two more thoughts:

Should there be notices in UPGRADING about this change? This could be a breaking change if you were relying on older buggy/insecure behaviour. I have some notes written in it that I'll commit.
Should there be an INI option to control the behaviour - i.e. force it back to the old behaviour or such?

cmb69 · 2022-04-22T15:35:36Z

Should there be notices in UPGRADING about this change?

Yes, definitely. Usually, these notes are not part of the PR (mostly because that easily can get stale), but feel free to add a respective commit (or just post the upgrading notes in a comment, so the "merger" can finally add these to UPGRADING).

Should there be an INI option to control the behaviour

I tend to say no, because that would cause different code behavior based on an INI setting. And if we target "master", the need for backward compatibility isn't that strong (besides that the changes likely only affect few users).

NattyNarwhal · 2022-04-26T14:26:55Z

I think I'm confident where this PR stands now; it should be ready for review now.

cmb69

Thank you! I think I'm mostly (I left some comments) fine with this, but would like to get more input from others, especially since we're introducing new userland functions and the module in main.

ext/odbc/php_odbc.c

main/php_odbc_utils.c

NattyNarwhal · 2022-04-26T16:43:41Z

I've removed the defensive checks as you sugest.

I can understand concerns about new functionality in main/ and ext/standard/ - the former seemed unavoidable, the latter made sense to me for the sake of the unit test. I'm not married to the idea of a public API surface, so it could be rethought/omitted.

Girgias · 2022-04-27T18:06:26Z

I'm really not a fan of having this in ext/standard, ext/odbc maybe.

I don't really like the fact this resides in main either, but don't have a good solution against that.

NattyNarwhal · 2022-04-27T18:14:37Z

I'm really not a fan of having this in ext/standard, ext/odbc maybe.

I can see that - I just put it there because it lives in main/ (so it could work without any ODBC exts, really) and would be useful in a PDO_ODBC-but-no-ODBC world. I think the closest to that is where Zend Server ships an ODBC built against the IBM SQL/CLI library, but PDO_ODBC built against unixODBC, but in that case ODBC would be available, so it's probably a reasonable thing to do.

I don't really like the fact this resides in main either, but don't have a good solution against that.

TBH, me neither, since it's quite ODBC specific, but there's not many places where extensions can share code other than that.

NattyNarwhal · 2022-05-05T14:18:15Z

Sorry for bumping, but it's been a week. I've moved the functions out of standard. I'd appreciate any reviews and additional concerns for review; I believe I had addressed mostly everything so far that I can, but some stuff might be better answered by someone with more knowledge of internals than me.

Because the UID= and PWD= values are appended to the SQLDriverConnect case when credentials are passed, we have to append them to the string in case users are relying on this behaviour. However, they must be quoted, or the arguments will be invalid (or possibly more injected). This means users had to quote arguments or append credentials to the raw connection string themselves. It seems that ODBC quoting rules are consistent enough (and that Microsoft trusts them enough to encode into the .NET BCL) that we can actually check if the string is already quoted (in case a user is already quoting because of this not being fixed), and if not, apply the appropriate ODBC quoting rules. This has been tested with the MariaDB and IBM i Db2 drivers. There are still some questions: - Procedural ODBC, with code sharing? - Are we paying attention to all of the quoting rules? i.e. any other special characters? We don't need to care if ; are in the strings at least, since we always quote now. - Should we always quote? Could it break some drivers? If so, could it be an INI option? - Buffer length/truncation behaviour (since the quoted string is on stack) - How do we unit test this?

We might not have to do the work if we don't have to. cmb points out that the rules are a little contradictory, and to me, seem unnecessary for some special characters, but it might be worth a shot.

Also handle freeing. No RAII in C, so there's some convolution in how we free the variables in this scope. Let me know if there's a better way.

I think the intent with the switch was special handling for the quotes, but then I realize you didn't need it if it's already quoted. Makes the suggested change from cmb.

Handles non-null UID with null PWD crash. Also is a bit more strict with the strstr to find an existing UID/PWD set in the connection string.

Right now, it just blithely duplicates the functions backing it, and the logic has been adapted to fit the procedural ODBC one. Putting this in `main/` or something would be a good idea to avoid having to keep this function in multiple places. I see some potential for more sharing and alteration of the rules in both though.

As of right now, only the connection string quoting rules, but perhaps other functionality can be moved in as needed. I'm not sure if main/ is the best place, but it seems other functionality only really used by extensions is here, so it makes sense.

This is because the code exists in main/, and are shared between both ODBC extensions, so it doesn't make sense for it to only exist in one or the other. There may be a better spot for it.

The primary reason for these functions oother than for i.e. database frameworks and libraries. It's much easier to test them outside of the context of needing a full ODBC driver.

Per GPB's suggestion. It does make sense, even if the functionality is common to both extensions.

NattyNarwhal · 2022-05-24T15:22:41Z

Sorry for the bump again, but I've rebased. I've also heard about the 8.2 feature freeze in a few months, so I would like to try to get this in before that, particularly if this has security repercussions.

cmb69

Thanks for your work, and the pinging! :)

Except for the minor issue pointed out below, this looks good to me. I'm not too happy to have the functions in ext/odbc (since they may be useful for PDO_ODBC users as well), but we discussed that already, and it appears to be the best compromise (for now).

If there are no objections, I'll merge this on Friday.

ext/odbc/odbc_utils.c

This PR was merged into the 1.28-dev branch. Discussion ---------- Add odbc_connection_string_*() functions This PR adds ODBC functions added in php/php-src#8307. Commits ------- 1ec3314 Add odbc_connection_string_*() functions

NattyNarwhal marked this pull request as draft April 5, 2022 21:34

cmb69 reviewed Apr 7, 2022

View reviewed changes

NattyNarwhal mentioned this pull request Apr 11, 2022

ODBC's heuristic for guessing if DSN or connection string could be better #8345

Closed

NattyNarwhal force-pushed the quote-odbc-uid-pwd branch from f5337d3 to c542668 Compare April 20, 2022 18:38

NattyNarwhal marked this pull request as ready for review April 22, 2022 15:07

NattyNarwhal changed the title ~~WIP: Quote when appending username and password to the ODBC connection string~~ \Quote when appending username and password to the ODBC connection string Apr 22, 2022

NattyNarwhal changed the title ~~\Quote when appending username and password to the ODBC connection string~~ Quote when appending username and password to the ODBC connection string Apr 22, 2022

cmb69 reviewed Apr 26, 2022

View reviewed changes

ext/odbc/php_odbc.c Outdated Show resolved Hide resolved

ext/odbc/php_odbc.c Outdated Show resolved Hide resolved

main/php_odbc_utils.c Outdated Show resolved Hide resolved

main/php_odbc_utils.c Show resolved Hide resolved

NattyNarwhal added 9 commits May 18, 2022 15:37

Determine if a string should be ODBC quoted

d338f35

We might not have to do the work if we don't have to. cmb points out that the rules are a little contradictory, and to me, seem unnecessary for some special characters, but it might be worth a shot.

Allocate a buffer for each quoted string

7b337d4

Also handle freeing. No RAII in C, so there's some convolution in how we free the variables in this scope. Let me know if there's a better way.

Use strpbrk to check for problem characters

542cbb1

I think the intent with the switch was special handling for the quotes, but then I realize you didn't need it if it's already quoted. Makes the suggested change from cmb.

Check null password when append connection string

f0102b2

Handles non-null UID with null PWD crash. Also is a bit more strict with the strstr to find an existing UID/PWD set in the connection string.

Check for the correct variable being null

2fce9e7

Expose ODBC connection string funcs in standard

a4c133e

This is because the code exists in main/, and are shared between both ODBC extensions, so it doesn't make sense for it to only exist in one or the other. There may be a better spot for it.

NattyNarwhal added 6 commits May 18, 2022 15:37

Userland tests for ODBC connection string funcs

9e832ac

The primary reason for these functions oother than for i.e. database frameworks and libraries. It's much easier to test them outside of the context of needing a full ODBC driver.

Notes on behaviour changes in ODBC for 8.2

9cf7edc

Get rid of debugging cruft

9211bf7

ZendMM can't fail, omit checks

7826a4c

remove defensive check if callers will never pass us NULL

0c4c018

Move ODBC string userland API into ext/odbc

311b1c1

Per GPB's suggestion. It does make sense, even if the functionality is common to both extensions.

NattyNarwhal force-pushed the quote-odbc-uid-pwd branch from 74291c4 to 311b1c1 Compare May 18, 2022 18:39

cmb69 approved these changes May 24, 2022

View reviewed changes

ext/odbc/odbc_utils.c Outdated Show resolved Hide resolved

Update comment about location and reasoning

3e76962

ramsey added Extension: odbc Extension: pdo_odbc labels May 24, 2022

ramsey added this to the PHP 8.2 milestone May 24, 2022

cmb69 closed this in 2920a26 May 27, 2022

derrabus mentioned this pull request Jun 29, 2023

Add odbc_connection_string_*() functions symfony/polyfill#433

Merged

kevinreniers mentioned this pull request Sep 20, 2023

odbc_pconnect fails with DSN and username/password with special characters #12251

Closed

	#if (defined(HAVE_ALLOCA) \|\| (defined (__GNUC__) && __GNUC__ >= 2)) && !(defined(ZTS) && defined(HPUX)) && !defined(DARWIN)
	# define ZEND_ALLOCA_MAX_SIZE (32 * 1024)
	# define ALLOCA_FLAG(name) \
	bool name;
	# define SET_ALLOCA_FLAG(name) \
	name = 1
	# define do_alloca_ex(size, limit, use_heap) \
	((use_heap = (UNEXPECTED((size) > (limit)))) ? emalloc(size) : alloca(size))
	# define do_alloca(size, use_heap) \
	do_alloca_ex(size, ZEND_ALLOCA_MAX_SIZE, use_heap)
	# define free_alloca(p, use_heap) \
	do { if (UNEXPECTED(use_heap)) efree(p); } while (0)
	#else
	# define ALLOCA_FLAG(name)
	# define SET_ALLOCA_FLAG(name)
	# define do_alloca(p, use_heap) emalloc(p)
	# define free_alloca(p, use_heap) efree(p)
	#endif

Quote when appending username and password to the ODBC connection string #8307

Quote when appending username and password to the ODBC connection string #8307

Uh oh!

Conversation

NattyNarwhal commented Apr 5, 2022

Uh oh!

cmb69 commented Apr 6, 2022

Uh oh!

NattyNarwhal commented Apr 6, 2022

Uh oh!

Uh oh!

cmb69 Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

cmb69 Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal Apr 7, 2022

Choose a reason for hiding this comment

Uh oh!

NattyNarwhal commented Apr 8, 2022

Uh oh!

NattyNarwhal commented Apr 18, 2022

Uh oh!

cmb69 commented Apr 20, 2022

Uh oh!

NattyNarwhal commented Apr 20, 2022

Uh oh!

NattyNarwhal commented Apr 22, 2022

Uh oh!

cmb69 commented Apr 22, 2022

Uh oh!

NattyNarwhal commented Apr 26, 2022

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NattyNarwhal commented Apr 26, 2022

Uh oh!

Girgias commented Apr 27, 2022

Uh oh!

NattyNarwhal commented Apr 27, 2022

Uh oh!

NattyNarwhal commented May 5, 2022

Uh oh!

NattyNarwhal commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NattyNarwhal commented May 24, 2022 •

edited

Loading