Skip to content

Commit 1a2b493

Browse files
david-pugliellidavid-puglielli
authored
Fixed for issues found by Semmle (#1011)
* Removed unneeded constants * Fixed sqlsrv_free_stmt argument info * Fixed brace escape to avoid buffer overflow * Fixed brace escape and added test * Debugging test failure on Bamboo * Removed debugging output * Debugging test failure on Bamboo * Removed debugging output * Added more test cases * Changed range check to use strchr * Added pdo test * Fixed test and formatting
1 parent b839ede commit 1a2b493

File tree

8 files changed

+165
-24
lines changed

8 files changed

+165
-24
lines changed

‎source/pdo_sqlsrv/pdo_dbh.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1692,7 +1692,6 @@ void validate_stmt_options( _Inout_ sqlsrv_context& ctx, _Inout_ zval* stmt_opti
16921692

16931693
ZEND_HASH_FOREACH_KEY_VAL( options_ht, int_key, key, data ) {
16941694
int type = HASH_KEY_NON_EXISTENT;
1695-
int result = 0;
16961695
type = key ? HASH_KEY_IS_STRING : HASH_KEY_IS_LONG;
16971696
CHECK_CUSTOM_ERROR(( type != HASH_KEY_IS_LONG ), ctx, PDO_SQLSRV_ERROR_INVALID_STMT_OPTION ) {
16981697
throw core::CoreException();

‎source/shared/core_conn.cpp

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -702,22 +702,32 @@ void core_sqlsrv_get_client_info( _Inout_ sqlsrv_conn* conn, _Out_ zval *client_
702702

703703
bool core_is_conn_opt_value_escaped( _Inout_ const char* value, _Inout_ size_t value_len )
704704
{
705-
// if the value is already quoted, then only analyse the part inside the quotes and return it as
706-
// unquoted since we quote it when adding it to the connection string.
707-
if( value_len > 0 && value[0] == '{' && value[value_len - 1] == '}' ) {
708-
++value;
705+
if (value_len == 0) {
706+
return true;
707+
}
708+
709+
if (value_len == 1) {
710+
return (value[0] != '}');
711+
}
712+
713+
const char *pstr = value;
714+
if (value_len > 0 && value[0] == '{' && value[value_len - 1] == '}') {
715+
pstr = ++value;
709716
value_len -= 2;
710717
}
711-
// check to make sure that all right braces are escaped
718+
719+
const char *pch = strchr(pstr, '}');
712720
size_t i = 0;
713-
while( ( value[i] != '}' || ( value[i] == '}' && value[i+1] == '}' )) && i < value_len ) {
714-
// skip both braces
715-
if( value[i] == '}' )
716-
++i;
717-
++i;
718-
}
719-
if( i < value_len && value[i] == '}' ) {
720-
return false;
721+
722+
while (pch != NULL && i < value_len) {
723+
i = pch - pstr + 1;
724+
725+
if (i == value_len || (i < value_len && pstr[i] != '}')) {
726+
return false;
727+
}
728+
729+
i++; // skip the brace
730+
pch = strchr(pch + 2, '}'); // continue searching
721731
}
722732

723733
return true;

‎source/shared/core_results.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,14 +243,12 @@ std::string getUTF8StringFromString( _In_z_ const SQLWCHAR* source )
243243
{
244244
// convert to regular character string first
245245
char c_str[4] = "";
246-
mbstate_t mbs;
247246

248247
SQLLEN i = 0;
249248
std::string str;
250249
while ( source[i] )
251250
{
252251
memset( c_str, 0, sizeof( c_str ) );
253-
DWORD rc;
254252
int cch = 0;
255253
errno_t err = mplat_wctomb_s( &cch, c_str, sizeof( c_str ), source[i++] );
256254
if ( cch > 0 && err == ERROR_SUCCESS )

‎source/shared/core_sqlsrv.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,6 @@ OACR_WARNING_POP
170170
// constants for maximums in SQL Server
171171
const int SS_MAXCOLNAMELEN = 128;
172172
const int SQL_SERVER_MAX_FIELD_SIZE = 8000;
173-
const int SQL_SERVER_MAX_PRECISION = 38;
174173
const int SQL_SERVER_MAX_TYPE_SIZE = 0;
175174
const int SQL_SERVER_MAX_PARAMS = 2100;
176175
const int SQL_SERVER_MAX_MONEY_SCALE = 4;
@@ -998,8 +997,6 @@ class sqlsrv_context {
998997
SQLSRV_ENCODING encoding_; // encoding of the context
999998
};
1000999

1001-
const int SQLSRV_OS_VISTA_OR_LATER = 6; // major version for Vista
1002-
10031000
// maps an IANA encoding to a code page
10041001
struct sqlsrv_encoding {
10051002

‎source/sqlsrv/init.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ zend_function_entry sqlsrv_functions[] = {
219219
PHP_FE( sqlsrv_client_info, sqlsrv_client_info_arginfo )
220220
PHP_FE( sqlsrv_server_info, sqlsrv_server_info_arginfo )
221221
PHP_FE( sqlsrv_cancel, sqlsrv_cancel_arginfo )
222-
PHP_FE( sqlsrv_free_stmt, sqlsrv_close_arginfo )
222+
PHP_FE( sqlsrv_free_stmt, sqlsrv_free_stmt_arginfo )
223223
PHP_FE( sqlsrv_field_metadata, sqlsrv_field_metadata_arginfo )
224224
PHP_FE( sqlsrv_send_stream_data, sqlsrv_send_stream_data_arginfo )
225225
PHP_FE( SQLSRV_SQLTYPE_BINARY, sqlsrv_sqltype_size_arginfo )

‎source/sqlsrv/stmt.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ unsigned int current_log_subsystem = LOG_STMT;
4242

4343
// constants used as invalid types for type errors
4444
const zend_uchar PHPTYPE_INVALID = SQLSRV_PHPTYPE_INVALID;
45-
const int SQLTYPE_INVALID = 0;
4645
const int SQLSRV_INVALID_PRECISION = -1;
4746
const SQLUINTEGER SQLSRV_INVALID_SIZE = (~1U);
4847
const int SQLSRV_INVALID_SCALE = -1;
@@ -51,8 +50,6 @@ const int SQLSRV_SIZE_MAX_TYPE = -1;
5150
// constants for maximums in SQL Server
5251
const int SQL_SERVER_MAX_FIELD_SIZE = 8000;
5352
const int SQL_SERVER_MAX_PRECISION = 38;
54-
const int SQL_SERVER_DEFAULT_PRECISION = 18;
55-
const int SQL_SERVER_DEFAULT_SCALE = 0;
5653

5754
// default class used when no class is specified by sqlsrv_fetch_object
5855
const char STDCLASS_NAME[] = "stdclass";
@@ -470,7 +467,6 @@ PHP_FUNCTION( sqlsrv_fetch_array )
470467
PHP_FUNCTION( sqlsrv_field_metadata )
471468
{
472469
sqlsrv_stmt* stmt = NULL;
473-
SQLSMALLINT num_cols = -1;
474470

475471
LOG_FUNCTION( "sqlsrv_field_metadata" );
476472

‎test/functional/pdo_sqlsrv/pdo_escape_braces.phpt

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
--TEST--
2+
Test that right braces are escaped correctly and that error messages are correct when they're not
3+
--SKIPIF--
4+
<?php require('skipif.inc'); ?>
5+
--FILE--
6+
<?php
7+
$server = 'fakeserver';
8+
$uid = 'sa';
9+
$password = 'fakepassword';
10+
11+
// If the braces are fine, then we expect the connection to fail with a login timeout error
12+
$braceError = "An unescaped right brace (}) was found";
13+
$connError = (strtoupper(substr(php_uname('s'), 0, 3)) === 'WIN') ? "Could not open a connection to SQL Server" : "Login timeout expired";
14+
15+
// Every combination of one, two, three, or more right braces I can think of
16+
$testStrings = array(array("}", $braceError),
17+
array("{", $connError),
18+
array("{t}", $connError),
19+
array("{}}", $braceError),
20+
array("}}", $connError),
21+
array("}}}", $braceError),
22+
array("}}}}", $connError),
23+
array("{}}}", $connError),
24+
array("}{", $braceError),
25+
array("}{{", $braceError),
26+
array("test", $connError),
27+
array("{test}", $connError),
28+
array("{test", $connError),
29+
array("test}", $braceError),
30+
array("{{test}}", $braceError),
31+
array("{{test}", $connError),
32+
array("{{test", $connError),
33+
array("test}}", $connError),
34+
array("{test}}", $braceError),
35+
array("test}}}", $braceError),
36+
array("{test}}}", $connError),
37+
array("{test}}}}", $braceError),
38+
array("{test}}}}}", $connError),
39+
array("{test}}}}}}", $braceError),
40+
array("te}st", $braceError),
41+
array("{te}st}", $braceError),
42+
array("{te}}st}", $connError),
43+
array("{te}}}st}", $braceError),
44+
array("te}}s}t", $braceError),
45+
array("te}}s}}t", $connError),
46+
array("te}}}st", $braceError),
47+
array("te}}}}st", $connError),
48+
array("tes}}t", $connError),
49+
array("te}s}}t", $braceError),
50+
array("tes}}t}}", $connError),
51+
array("tes}}t}}}", $braceError),
52+
array("tes}t}}", $braceError),
53+
);
54+
55+
foreach ($testStrings as $test) {
56+
57+
try {
58+
$conn = new PDO("sqlsrv:Server=".$server.";LoginTimeout=1;", $test[0], $password);
59+
} catch (PDOException $e) {
60+
if (strpos($e->getMessage(), $test[1]) === false) {
61+
print_r("Wrong error message returned for test string ".$test[0].". Expected ".$test[1].", actual output:\n");
62+
print_r($e->getMessage);
63+
echo "\n";
64+
}
65+
}
66+
}
67+
68+
echo "Done.\n";
69+
?>
70+
--EXPECT--
71+
Done.

‎test/functional/sqlsrv/sqlsrv_escape_braces.phpt

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
--TEST--
2+
Test that right braces are escaped correctly and that error messages are correct when they're not
3+
--SKIPIF--
4+
<?php require('skipif.inc'); ?>
5+
--FILE--
6+
<?php
7+
$server = 'fakeserver';
8+
$uid = 'sa';
9+
$password = 'fakepassword';
10+
11+
// If the braces are fine, then we expect the connection to fail with a login timeout error
12+
$braceError = "An unescaped right brace (}) was found";
13+
$connError = (strtoupper(substr(php_uname('s'), 0, 3)) === 'WIN') ? "Could not open a connection to SQL Server" : "Login timeout expired";
14+
15+
// Every combination of one, two, three, or more right braces I can think of
16+
$testStrings = array(array("}", $braceError),
17+
array("{", $connError),
18+
array("{t}", $connError),
19+
array("{}}", $braceError),
20+
array("}}", $connError),
21+
array("}}}", $braceError),
22+
array("}}}}", $connError),
23+
array("{}}}", $connError),
24+
array("}{", $braceError),
25+
array("}{{", $braceError),
26+
array("test", $connError),
27+
array("{test}", $connError),
28+
array("{test", $connError),
29+
array("test}", $braceError),
30+
array("{{test}}", $braceError),
31+
array("{{test}", $connError),
32+
array("{{test", $connError),
33+
array("test}}", $connError),
34+
array("{test}}", $braceError),
35+
array("test}}}", $braceError),
36+
array("{test}}}", $connError),
37+
array("{test}}}}", $braceError),
38+
array("{test}}}}}", $connError),
39+
array("{test}}}}}}", $braceError),
40+
array("te}st", $braceError),
41+
array("{te}st}", $braceError),
42+
array("{te}}st}", $connError),
43+
array("{te}}}st}", $braceError),
44+
array("te}}s}t", $braceError),
45+
array("te}}s}}t", $connError),
46+
array("te}}}st", $braceError),
47+
array("te}}}}st", $connError),
48+
array("tes}}t", $connError),
49+
array("te}s}}t", $braceError),
50+
array("tes}}t}}", $connError),
51+
array("tes}}t}}}", $braceError),
52+
array("tes}t}}", $braceError),
53+
);
54+
55+
foreach ($testStrings as $test) {
56+
57+
$conn = sqlsrv_connect($server, array('uid'=>$test[0], 'pwd'=>$password, 'LoginTimeout'=>1));
58+
59+
if (strpos(sqlsrv_errors()[0][2], $test[1]) === false) {
60+
print_r("Wrong error message returned for test string ".$test[0].". Expected ".$test[1].", actual output:\n");
61+
print_r(sqlsrv_errors());
62+
}
63+
64+
unset($conn);
65+
}
66+
67+
echo "Done.\n";
68+
?>
69+
--EXPECT--
70+
Done.

0 commit comments

Comments
 (0)