Quote when appending username and password to the ODBC connection string

UPGRADING

@@ -25,6 +25,19 @@ PHP 8.2 UPGRADE NOTES
   . DateTimeImmutable::createFromMutable() now has a tentative return type of static,
     previously it was DateTimeImmutable
 
+- ODBC:
+  . The ODBC extension now escapes the username and password for the case when
+    both a connection string and username/password are passed, and the string
+    must be appended to. Before, user values containing values needing escaping
+    could have created a malformed connection string, or injected values from
+    user-provided data. The escaping rules should be identical to the .NET BCL
+    DbConnectionOptions behaviour.
+
+- PDO_ODBC:
+  . The PDO_ODBC extension also escapes the username and password when a
+    connection string is passed. See the change to the ODBC extension for
+    further details.
+
 - Standard:
   . strtolower() and strtoupper() are no longer locale-sensitive. They now
     perform ASCII case conversion, as if the locale were "C". Use
@@ -67,6 +80,13 @@ PHP 8.2 UPGRADE NOTES
     round-trips between PHP and Oracle Database when fetching LOBS. This is
     usable with Oracle Database 12.2 or later.
 
+- ODBC:
+  . Added odbc_connection_string_is_quoted, odbc_connection_string_should_quote,
+    and odbc_connection_string_quote. These are primarily used behind the scenes
+    in the ODBC and PDO_ODBC extensions, but is exposed to userland for easier
+    unit testing, and for user applications and libraries to perform quoting
+    themselves.
+
 - PCRE:
   . Added support for the "n" (NO_AUTO_CAPTURE) modifier, which makes simple
     `(xyz)` groups non-capturing. Only named groups like `(?<name>xyz)` are

configure.ac

@@ -1614,7 +1614,7 @@ PHP_ADD_SOURCES(main, main.c snprintf.c spprintf.c \
        php_ini_builder.c \
        php_ini.c SAPI.c rfc1867.c php_content_types.c strlcpy.c \
        strlcat.c explicit_bzero.c reentrancy.c php_variables.c php_ticks.c \
-       network.c php_open_temporary_file.c \
+       network.c php_open_temporary_file.c php_odbc_utils.c \
        output.c getopt.c php_syslog.c, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1)
 
 PHP_ADD_SOURCES_X(main, fastcgi.c, -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1, PHP_FASTCGI_OBJS, no)

ext/odbc/config.m4

@@ -461,7 +461,7 @@ if test -n "$ODBC_TYPE"; then
   PHP_SUBST_OLD(ODBC_LFLAGS)
   PHP_SUBST_OLD(ODBC_TYPE)
 
-  PHP_NEW_EXTENSION(odbc, php_odbc.c, $ext_shared,, [$ODBC_CFLAGS -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1])
+  PHP_NEW_EXTENSION(odbc, php_odbc.c odbc_utils.c, $ext_shared,, [$ODBC_CFLAGS -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1])
 else
   AC_MSG_CHECKING([for any ODBC driver support])
   AC_MSG_RESULT(no)

ext/odbc/config.w32

@@ -7,7 +7,7 @@ if (PHP_ODBC == "yes") {
 	if (CHECK_LIB("odbc32.lib", "odbc") && CHECK_LIB("odbccp32.lib", "odbc")
 	&& CHECK_HEADER_ADD_INCLUDE("sql.h", "CFLAGS_ODBC")
 	&& CHECK_HEADER_ADD_INCLUDE("sqlext.h", "CFLAGS_ODBC")) {
-		EXTENSION("odbc", "php_odbc.c", PHP_ODBC_SHARED, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
+		EXTENSION("odbc", "php_odbc.c odbc_utils.c", PHP_ODBC_SHARED, "/DZEND_ENABLE_STATIC_TSRMLS_CACHE=1");
 		AC_DEFINE("HAVE_UODBC", 1, "ODBC support");
 		if ("no" == PHP_ODBCVER) {
 			AC_DEFINE("ODBCVER", "0x0350", "The highest supported ODBC version", false);

ext/odbc/odbc.stub.php

@@ -197,3 +197,11 @@ function odbc_tableprivileges($odbc, ?string $catalog, string $schema, string $t
  */
 function odbc_columnprivileges($odbc, ?string $catalog, string $schema, string $table, string $column) {}
 #endif
+
+/* odbc_utils.c */
+
+function odbc_connection_string_is_quoted(string $str): bool {}
+
+function odbc_connection_string_should_quote(string $str): bool {}
+
+function odbc_connection_string_quote(string $str): string {}

ext/odbc/odbc_arginfo.h

@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 27a50ba79ed632721ee458527ef543e4b44ee897 */
+ * Stub hash: 298e48377c2d18c532d91a9ed97886b49a64c096 */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_odbc_close_all, 0, 0, IS_VOID, 0)
 ZEND_END_ARG_INFO()
@@ -245,6 +245,16 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_odbc_columnprivileges, 0, 0, 5)
 ZEND_END_ARG_INFO()
 #endif
 
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_odbc_connection_string_is_quoted, 0, 1, _IS_BOOL, 0)
+	ZEND_ARG_TYPE_INFO(0, str, IS_STRING, 0)
+ZEND_END_ARG_INFO()
+
+#define arginfo_odbc_connection_string_should_quote arginfo_odbc_connection_string_is_quoted
+
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_odbc_connection_string_quote, 0, 1, IS_STRING, 0)
+	ZEND_ARG_TYPE_INFO(0, str, IS_STRING, 0)
+ZEND_END_ARG_INFO()
+
 
 ZEND_FUNCTION(odbc_close_all);
 ZEND_FUNCTION(odbc_binmode);
@@ -307,6 +317,9 @@ ZEND_FUNCTION(odbc_tableprivileges);
 #if !defined(HAVE_DBMAKER) && !defined(HAVE_SOLID) && !defined(HAVE_SOLID_30) &&!defined(HAVE_SOLID_35)
 ZEND_FUNCTION(odbc_columnprivileges);
 #endif
+ZEND_FUNCTION(odbc_connection_string_is_quoted);
+ZEND_FUNCTION(odbc_connection_string_should_quote);
+ZEND_FUNCTION(odbc_connection_string_quote);
 
 
 static const zend_function_entry ext_functions[] = {
@@ -373,5 +386,8 @@ static const zend_function_entry ext_functions[] = {
 #if !defined(HAVE_DBMAKER) && !defined(HAVE_SOLID) && !defined(HAVE_SOLID_30) &&!defined(HAVE_SOLID_35)
 	ZEND_FE(odbc_columnprivileges, arginfo_odbc_columnprivileges)
 #endif
+	ZEND_FE(odbc_connection_string_is_quoted, arginfo_odbc_connection_string_is_quoted)
+	ZEND_FE(odbc_connection_string_should_quote, arginfo_odbc_connection_string_should_quote)
+	ZEND_FE(odbc_connection_string_quote, arginfo_odbc_connection_string_quote)
 	ZEND_FE_END
 };

ext/odbc/odbc_utils.c

@@ -0,0 +1,68 @@
+/*
+   +----------------------------------------------------------------------+
+   | Copyright (c) The PHP Group                                          |
+   +----------------------------------------------------------------------+
+   | This source file is subject to version 3.01 of the PHP license,      |
+   | that is bundled with this package in the file LICENSE, and is        |
+   | available through the world-wide-web at the following url:           |
+   | https://www.php.net/license/3_01.txt                                 |
+   | If you did not receive a copy of the PHP license and are unable to   |
+   | obtain it through the world-wide-web, please send a note to          |
+   | [email protected] so we can mail you a copy immediately.               |
+   +----------------------------------------------------------------------+
+   | Author: Calvin Buckley <[email protected]>                           |
+   +----------------------------------------------------------------------+
+*/
+
+#include "php.h"
+#include "php_odbc_utils.h"
+
+/*
+ * Utility functions for dealing with ODBC connection strings and other common
+ * functionality.
+ *
+ * While useful for PDO_ODBC too, this lives in ext/odbc because there isn't a
+ * better place for it.
+ */
+
+PHP_FUNCTION(odbc_connection_string_is_quoted)
+{
+	zend_string *str;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_STR(str)
+	ZEND_PARSE_PARAMETERS_END();
+
+	bool is_quoted = php_odbc_connstr_is_quoted(ZSTR_VAL(str));
+
+	RETURN_BOOL(is_quoted);
+}
+
+PHP_FUNCTION(odbc_connection_string_should_quote)
+{
+	zend_string *str;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_STR(str)
+	ZEND_PARSE_PARAMETERS_END();
+
+	bool should_quote = php_odbc_connstr_should_quote(ZSTR_VAL(str));
+
+	RETURN_BOOL(should_quote);
+}
+
+PHP_FUNCTION(odbc_connection_string_quote)
+{
+	zend_string *str;
+
+	ZEND_PARSE_PARAMETERS_START(1, 1)
+		Z_PARAM_STR(str)
+	ZEND_PARSE_PARAMETERS_END();
+
+	size_t new_size = php_odbc_connstr_estimate_quote_length(ZSTR_VAL(str));
+	zend_string *new_string = zend_string_alloc(new_size, 0);
+	php_odbc_connstr_quote(ZSTR_VAL(new_string), ZSTR_VAL(str), new_size);
+	/* reset length */
+	ZSTR_LEN(new_string) = strlen(ZSTR_VAL(new_string));
+	RETURN_STR(new_string);
+}

ext/odbc/php_odbc.c

@@ -34,6 +34,9 @@
 #include "php_globals.h"
 #include "odbc_arginfo.h"
 
+/* actually lives in main/ */
+#include "php_odbc_utils.h"
+
 #ifdef HAVE_UODBC
 
 #include <fcntl.h>
@@ -2169,8 +2172,38 @@ int odbc_sqlconnect(odbc_connection **conn, char *db, char *uid, char *pwd, int
 
 		if (strstr((char*)db, ";")) {
 			direct = 1;
-			if (uid && !strstr ((char*)db, "uid") && !strstr((char*)db, "UID")) {
-				spprintf(&ldb, 0, "%s;UID=%s;PWD=%s", db, uid, pwd);
+			/* Force UID and PWD to be set in the DSN */
+			bool is_uid_set = uid && *uid
+				&& !strstr(db, "uid=")
+				&& !strstr(db, "UID=");
+			bool is_pwd_set = pwd && *pwd
+				&& !strstr(db, "pwd=")
+				&& !strstr(db, "PWD=");
+			if (is_uid_set && is_pwd_set) {
+				char *uid_quoted = NULL, *pwd_quoted = NULL;
+				bool should_quote_uid = !php_odbc_connstr_is_quoted(uid) && php_odbc_connstr_should_quote(uid);
+				bool should_quote_pwd = !php_odbc_connstr_is_quoted(pwd) && php_odbc_connstr_should_quote(pwd);
+				if (should_quote_uid) {
+					size_t estimated_length = php_odbc_connstr_estimate_quote_length(uid);
+					uid_quoted = emalloc(estimated_length);
+					php_odbc_connstr_quote(uid_quoted, uid, estimated_length);
+				} else {
+					uid_quoted = uid;
+				}
+				if (should_quote_pwd) {
+					size_t estimated_length = php_odbc_connstr_estimate_quote_length(pwd);
+					pwd_quoted = emalloc(estimated_length);
+					php_odbc_connstr_quote(pwd_quoted, pwd, estimated_length);
+				} else {
+					pwd_quoted = pwd;
+				}
+				spprintf(&ldb, 0, "%s;UID=%s;PWD=%s", db, uid_quoted, pwd_quoted);
+				if (uid_quoted && should_quote_uid) {
+					efree(uid_quoted);
+				}
+				if (pwd_quoted && should_quote_pwd) {
+					efree(pwd_quoted);
+				}
 			} else {
 				ldb_len = strlen(db)+1;
 				ldb = (char*) emalloc(ldb_len);

ext/odbc/tests/odbc_utils.phpt

@@ -0,0 +1,81 @@
+--TEST--
+Test common ODBC string functionality
+--EXTENSIONS--
+odbc
+--FILE--
+<?php
+
+// 1. No, it's not quoted.
+// 2. Yes, it should be quoted because of the special character in the middle.
+$with_end_curly1 = "foo}bar";
+// 1. No, the unescaped special character in the middle breaks what would be quoted.
+// 2. Yes, it should be quoted because of the special character in the middle.
+//    Note that should_quote doesn't care about if the string is already quoted.
+//    That's why you should check if it is quoted first.
+$with_end_curly2 = "{foo}bar}";
+// 1. Yes, the special characters are escaped, so it's quoted.
+// 2. See $with_end_curly2; should_quote doesn't care about if the string is already quoted.
+$with_end_curly3 = "{foo}}bar}";
+// 1. No, it's not quoted.
+// 2. It doesn't need to be quoted because of no s
+$with_no_end_curly1 = "foobar";
+// 1. Yes, it is quoted and any characters are properly escaped.
+// 2. See $with_end_curly2.
+$with_no_end_curly2 = "{foobar}";
+
+echo "# Is quoted?\n";
+echo "With end curly brace 1: ";
+var_dump(odbc_connection_string_is_quoted($with_end_curly1));
+echo "With end curly brace 2: ";
+var_dump(odbc_connection_string_is_quoted($with_end_curly2));
+echo "With end curly brace 3: ";
+var_dump(odbc_connection_string_is_quoted($with_end_curly3));
+echo "Without end curly brace 1: ";
+var_dump(odbc_connection_string_is_quoted($with_no_end_curly1));
+echo "Without end curly brace 2: ";
+var_dump(odbc_connection_string_is_quoted($with_no_end_curly2));
+
+echo "# Should quote?\n";
+echo "With end curly brace 1: ";
+var_dump(odbc_connection_string_should_quote($with_end_curly1));
+echo "With end curly brace 2: ";
+var_dump(odbc_connection_string_should_quote($with_end_curly2));
+echo "With end curly brace 3: ";
+var_dump(odbc_connection_string_should_quote($with_end_curly3));
+echo "Without end curly brace 1: ";
+var_dump(odbc_connection_string_should_quote($with_no_end_curly1));
+echo "Without end curly brace 2: ";
+var_dump(odbc_connection_string_should_quote($with_no_end_curly2));
+
+echo "# Quote?\n";
+echo "With end curly brace 1: ";
+var_dump(odbc_connection_string_quote($with_end_curly1));
+echo "With end curly brace 2: ";
+var_dump(odbc_connection_string_quote($with_end_curly2));
+echo "With end curly brace 3: ";
+var_dump(odbc_connection_string_quote($with_end_curly3));
+echo "Without end curly brace 1: ";
+var_dump(odbc_connection_string_quote($with_no_end_curly1));
+echo "Without end curly brace 2: ";
+var_dump(odbc_connection_string_quote($with_no_end_curly2));
+
+?>
+--EXPECTF--
+# Is quoted?
+With end curly brace 1: bool(false)
+With end curly brace 2: bool(false)
+With end curly brace 3: bool(true)
+Without end curly brace 1: bool(false)
+Without end curly brace 2: bool(true)
+# Should quote?
+With end curly brace 1: bool(true)
+With end curly brace 2: bool(true)
+With end curly brace 3: bool(true)
+Without end curly brace 1: bool(false)
+Without end curly brace 2: bool(true)
+# Quote?
+With end curly brace 1: string(10) "{foo}}bar}"
+With end curly brace 2: string(13) "{{foo}}bar}}}"
+With end curly brace 3: string(15) "{{foo}}}}bar}}}"
+Without end curly brace 1: string(8) "{foobar}"
+Without end curly brace 2: string(11) "{{foobar}}}"

ext/pdo_odbc/odbc_driver.c

@@ -23,6 +23,8 @@
 #include "ext/standard/info.h"
 #include "pdo/php_pdo.h"
 #include "pdo/php_pdo_driver.h"
+/* this file actually lives in main/ */
+#include "php_odbc_utils.h"
 #include "php_pdo_odbc.h"
 #include "php_pdo_odbc_int.h"
 #include "zend_exceptions.h"
@@ -485,20 +487,43 @@ static int pdo_odbc_handle_factory(pdo_dbh_t *dbh, zval *driver_options) /* {{{
 		use_direct = 1;
 
 		/* Force UID and PWD to be set in the DSN */
-		if (dbh->username && *dbh->username && !strstr(dbh->data_source, "uid")
-				&& !strstr(dbh->data_source, "UID")) {
-			/* XXX: Do we check if password is null? */
+		bool is_uid_set = dbh->username && *dbh->username
+			&& !strstr(dbh->data_source, "uid=")
+			&& !strstr(dbh->data_source, "UID=");
+		bool is_pwd_set = dbh->password && *dbh->password
+			&& !strstr(dbh->data_source, "pwd=")
+			&& !strstr(dbh->data_source, "PWD=");
+		if (is_uid_set && is_pwd_set) {
+			char *uid = NULL, *pwd = NULL;
+			bool should_quote_uid = !php_odbc_connstr_is_quoted(dbh->username) && php_odbc_connstr_should_quote(dbh->username);
+			bool should_quote_pwd = !php_odbc_connstr_is_quoted(dbh->password) && php_odbc_connstr_should_quote(dbh->password);
+			if (should_quote_uid) {
+				size_t estimated_length = php_odbc_connstr_estimate_quote_length(dbh->username);
+				uid = emalloc(estimated_length);
+				php_odbc_connstr_quote(uid, dbh->username, estimated_length);
+			} else {
+				uid = dbh->username;
+			}
+			if (should_quote_pwd) {
+				size_t estimated_length = php_odbc_connstr_estimate_quote_length(dbh->password);
+				pwd = emalloc(estimated_length);
+				php_odbc_connstr_quote(pwd, dbh->password, estimated_length);
+			} else {
+				pwd = dbh->password;
+			}
 			size_t new_dsn_size = strlen(dbh->data_source)
-				+ strlen(dbh->username) + strlen(dbh->password)
+				+ strlen(uid) + strlen(pwd)
 				+ strlen(";UID=;PWD=") + 1;
 			char *dsn = pemalloc(new_dsn_size, dbh->is_persistent);
-			if (dsn == NULL) {
-				/* XXX: Do we inform the caller? */
-				goto fail;
-			}
-			snprintf(dsn, new_dsn_size, "%s;UID=%s;PWD=%s", dbh->data_source, dbh->username, dbh->password);
+			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;
+			if (uid && should_quote_uid) {
+				efree(uid);
+			}
+			if (pwd && should_quote_pwd) {
+				efree(pwd);
+			}
 		}
 
 		rc = SQLDriverConnect(H->dbc, NULL, (SQLCHAR *) dbh->data_source, strlen(dbh->data_source),