PDO_ODBC can inject garbage into field values

Description

Running the following code works fine, as long as the content of the “MyLongText” database field is not longer than 2048 characters. The command var_dump($row); returns “string(2048)” and the expected field content.

When adding one additional character to the database field (in this example 2049 underscores), var_dump($row); returns “string(4098)” -- exactly twice the effective text length in the database field -- and shows that inside the text from the database field, some unreadable string is injected like:
�=@$��_@Pj_?_��`j?_�۶��___m_E_�9l��P_Pm_B_0��_��m_d

For field contents of more than 2048 characters, the length of the content returned by PDO is always doubled; 2050 characters in the database field are returned by var_dump as “string(4100)”.

Using “odbc_connect” instead of “new PDO” the command “var_dump($row1["MyLongText"]);” returns “string(2049)” – as expected - and shows the correct content from the database field “MyLongText”.

The installed ODBC driver on the server is provided by "Microsoft Access Database Engine 2016 Redistributable"

The issue with PDO_ODBC occurs with PHP 8.0, 8.1, 8.2, 8.3 and 8.4. When downgrading to PHP 7.4 this php code works as expected.
The behaviour is similar to that described under https://bugs.php.net/bug.php?id=81688 .

<?php

header("content-type: text/html; charset=utf-8");

$dbFile = "\\\\my-server\\my-path-to-the-database-file \\DBtest.accdb";

$sql = "SELECT MyLongText FROM Table1";

$pdo = new PDO("odbc:Driver={Microsoft Access Driver (*.mdb, *.accdb)};Dbq=".$dbFile.";Uid=; Pwd=;ExtendedAnsiSQL=1;");
$pdo->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );

$result = $pdo->query($sql);
$row = $result->fetchColumn(0);
	
echo "var_dump of PDO: ";
var_dump($row);

echo "<br>";
echo "<br>";
	
$conn = odbc_connect("Driver={Microsoft Access Driver (*.mdb, *.accdb)};Dbq=$dbFile","", "");
$query = odbc_exec($conn, $sql);
while($row1 = odbc_fetch_array($query)){
	echo "var_dump of odbc_connect: ";
	var_dump($row1["MyLongText"]);
}

Resulted in this output:

var_dump of PDO: string(4100) "______<more underscore>______�� ��fA��8�fA��_______<more underscore>_______"

var_dump of odbc_connect: string(2050) "______<more undescore>_______"

But I expected this output instead:

var_dump of PDO: string(2050) "______<more underscore>_______"

var_dump of odbc_connect: string(2050) "______<more undescore>_______"

PHP Version

8.3.12 x64 Non Thread Safe and 8.4.0 RC1 (Visual C++ Redistributable X64 (Visual C++ Redistributable for Visual Studio 2022) installed)

Operating System

Windows Server 2019 Standard, Internet Information Services (Version 10.01.17763.1) or Windows 10

Can you turn on ODBC tracing (MS documentation here) and provide the log? Ideally for both a version where it works and doesn't work.

It would also be helpful if you post the table definition.

It would also be helpful if you post the table definition.

You can find the table definitions in the file doc_Table1.pdf.

The table ‘Table1’ contains only one record:
ID = 1
MyLongText = “_____ … _____” (2049 times the character underscore)

Can you turn on ODBC tracing (MS documentation here) and provide the log? Ideally for both a version where it works and doesn't work.

I enabled the ODBC tracing but no file was created. I have created the new folder "c:\odbc_log" where the user "everybody" has read and write access and selected this folder as Log File Path. I checked the settings in the Registry; HKEY_LOCAL_MACHINE\SOFTWARE\WOW6432Node\ODBC\ODBC.INI\ODBC\Trace=1 and HKEY_LOCAL_MACHINE\SOFTWARE\ODBC\ODBC.INI\ODBC\Trace=1 looks fine.
Is the tracing dependent on the ODBC driver used?

After running <?php phpinfo(); and calling the above script again, the injected string was readable and looks like a part of the phpinfos (var_dump.pdf). It looks like the injected text comes from memory.

@studata

@studata, I can reproduce the problem, so likely there is no need for an ODBC trace from your side.

I'll follow up with an analysis soon.

Table setup (don't use sprintf() to construct queries in production):

// odbc_exec($odbc, "DROP TABLE Table1");

odbc_exec($odbc, "CREATE TABLE Table1 (Id INT, MyLongText Memo)");
odbc_exec($odbc, sprintf("INSERT INTO Table1 VALUES (1, '%s')", str_repeat("_", 2048)));
odbc_exec($odbc, sprintf("INSERT INTO Table1 VALUES (1, '%s')", str_repeat("_", 2049)));

Querying/fetching basically as in the OP.

For ext/odbc everything is fine, since we're doing only a single SQLGetData() with result->longreadlen == 4096 (odbc.defaultlrl):

php-src/ext/odbc/php_odbc.c

Line 1461 in 097edc8

    
           rc = SQLGetData(result->stmt, (SQLUSMALLINT)(i + 1), sql_c_type, buf, result->longreadlen + 1, &result->values[i].vallen);

For ext/pdo_odbc, everything is fine for the first row, but for the second row the driver doesn't behave as expected. The first SQLGetData()

php-src/ext/pdo_odbc/odbc_stmt.c

Lines 663 to 664 in 097edc8

    
           rc = SQLGetData(S->stmt, colno+1, C->is_unicode ? SQL_C_BINARY : SQL_C_CHAR, C->data, 
        
           		256, &C->fetched_len);

yields fetched_len == 4096 (and SQL_SUCCESS_WITH_INFO). The second SQLGetData()

php-src/ext/pdo_odbc/odbc_stmt.c

Line 690 in 097edc8

    
           rc = SQLGetData(S->stmt, colno+1, C->is_unicode ? SQL_C_BINARY : SQL_C_CHAR, buf2, 256, &C->fetched_len);

however, yields a fetched_len == 1794, while that should be 4096 - 255 == 3841. Apparently, the driver is confused because it reported a length of 4096, although actually the length is only 2049 (and 2049-255=1794). Anyhow, from there on it goes south quickly; in a debug build the following assertion would trigger

php-src/ext/pdo_odbc/odbc_stmt.c

Line 695 in 097edc8

ZEND_ASSERT(fixed_used <= used + 1);

but for a release build just some random garbage already in memory is "pasted" into the result string.

It seems to me we need to revisit #6716 which introduced the dependence on the fetched_len (there are a couple of related good ideas and discussions).

But even if we attempt a proper fix for master only, we should make sure that we don't leak memory contents (and perhaps increase the buffer size, what would mitigate the issues a bit) in any of the stable versions.

This part of PDO_ODBC has been a bit of a thorn in the side for me too, although I didn't realize there was a leak issue with it. For the sake of collating previous discussion: I think I made GH-10733 with some of the issues it has calculating buffer sizes, and GH-10809 where I tried to fix that issue (with help from Saki) albeit with some possible performance issues.

See also #6716 (comment).

A simple solution to this issue specifically always using SQL_BINARY (even if wide characters) sounds good, though I'd be worried it might not do automatic character conversion in the driver. That's pretty common with EBCDIC columns. Maybe it could be a toggle, but I don't think we want to add more knobs.

FWIW, I have a more general issue about a PDO binary type (partial implementation here, though some concerns) motivated by issues I've had with PDO_ODBC and DBLIB, but that's a more general thing. It also probably needs some discussion on internals if I want to dust it off...

Fix phpGH-16450: PDO_ODBC can inject garbage into field values

I've quickly changed to reading as SQL_C_BINARY (same as SQL_BINARY), and s/255/256, and got the desired result.

Nope, didn't work; I also had commented out the used adjustment.

FWIW, I have a more general issue about a PDO binary type (partial implementation here, though some concerns) motivated by issues I've had with PDO_ODBC and DBLIB, but that's a more general thing.

PARAM_BINARY sounds reasonable, but only would solve this for bound columns, not for direct fetches.

Anyhow, after further consideration, solving this issue might not be that hard. We could just turn the assertion into a runtime check. For drivers which properly count down StrLen_or_IndPtr nothing changes, but at least we can fix the problem for MS Access databases. See #16587.

@NattyNarwhal

@NattyNarwhal, I doubt that this would fix the issues reported as #10733, but maybe on top of #16587 the following would help:

 ext/pdo_odbc/odbc_stmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/pdo_odbc/odbc_stmt.c b/ext/pdo_odbc/odbc_stmt.c
index ffe09a6bf1..18fae62746 100644
--- a/ext/pdo_odbc/odbc_stmt.c
+++ b/ext/pdo_odbc/odbc_stmt.c
@@ -692,7 +692,7 @@ static int odbc_stmt_get_col(pdo_stmt_t *stmt, int colno, zval *result, enum pdo
 				/* adjust `used` in case we have proper length info from the driver */
 				if (orig_fetched_len >= 0 && C->fetched_len >= 0) {
 					SQLLEN fixed_used = orig_fetched_len - C->fetched_len;
-					if (fixed_used <= used + 1) {
+					if (fixed_used > 0 && fixed_used <= used + 1) {
 						used = fixed_used;
 					}
 				}

Fix GH-16450: PDO_ODBC can inject garbage into field values

Merge branch 'PHP-8.2' into PHP-8.3

Merge branch 'PHP-8.3' into PHP-8.4

Merge branch 'PHP-8.4'

Navigation Menu

Repository navigation

PDO_ODBC can inject garbage into field values #16450

Description

PHP Version

Operating System

5 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	rc = SQLGetData(S->stmt, colno+1, C->is_unicode ? SQL_C_BINARY : SQL_C_CHAR, C->data,
	256, &C->fetched_len);

PDO_ODBC can inject garbage into field values #16450

Description

Description

PHP Version

Operating System

Activity

NattyNarwhal commented on Oct 15, 2024

cmb69 commented on Oct 15, 2024

studata commented on Oct 15, 2024

studata commented on Oct 15, 2024

studata commented on Oct 15, 2024

cmb69 commented on Oct 15, 2024

cmb69 commented on Oct 15, 2024

NattyNarwhal commented on Oct 16, 2024

5 remaining items

NattyNarwhal commented on Oct 22, 2024

cmb69 commented on Oct 25, 2024

cmb69 commented on Oct 25, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions