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

Fixes for issues found by Semmle #1011

Merged
merged 12 commits into from
Jul 23, 2019
1 change: 0 additions & 1 deletion source/pdo_sqlsrv/pdo_dbh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1692,7 +1692,6 @@ void validate_stmt_options( _Inout_ sqlsrv_context& ctx, _Inout_ zval* stmt_opti

ZEND_HASH_FOREACH_KEY_VAL( options_ht, int_key, key, data ) {
int type = HASH_KEY_NON_EXISTENT;
int result = 0;
type = key ? HASH_KEY_IS_STRING : HASH_KEY_IS_LONG;
CHECK_CUSTOM_ERROR(( type != HASH_KEY_IS_LONG ), ctx, PDO_SQLSRV_ERROR_INVALID_STMT_OPTION ) {
throw core::CoreException();
Expand Down
32 changes: 17 additions & 15 deletions source/shared/core_conn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -702,24 +702,26 @@ void core_sqlsrv_get_client_info( _Inout_ sqlsrv_conn* conn, _Out_ zval *client_

bool core_is_conn_opt_value_escaped( _Inout_ const char* value, _Inout_ size_t value_len )
{
// if the value is already quoted, then only analyse the part inside the quotes and return it as
// unquoted since we quote it when adding it to the connection string.
if( value_len > 0 && value[0] == '{' && value[value_len - 1] == '}' ) {
++value;
value_len -= 2;
const char *pstr = value;
if (value_len > 0 && value[0] == '{' && value[value_len - 1] == '}') {
pstr = ++value;
value_len -= 2;
}
// check to make sure that all right braces are escaped
size_t i = 0;
while( ( value[i] != '}' || ( value[i] == '}' && value[i+1] == '}' )) && i < value_len ) {
// skip both braces
if( value[i] == '}' )
++i;
++i;
}
if( i < value_len && value[i] == '}' ) {
return false;

if (value_len == 1) {
return (value[0] != '}');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code seems to assume value_len >= 2? But what if value_len is 0? See my first comment above...

In fact, consider using strchr() for parsing the string, something like this:

const char *pstr = value;
if (value_len > 0 && value[0] == '{' && value[value_len - 1] == '}') {
        pstr = ++value;
        value_len -= 2;
}

if (value_len == 1) {
        return (value[0] != '}');
}

const char *pch = strchr(pstr, '}');
size_t i = 0;
while (pch != NULL && i < value_len) {
        i = pch - pstr + 1;
        if (i == value_len || (i < value_len && pstr[i] != '}')) {
            return false;
        }
        i++;    // skip the brace
        pch = strchr(pch + 2, '}'); // continue searching
}
return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the last call to strchr may overshoot the end of the buffer. But I will try it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @david-puglielli please double check whether this might happen, fix the formatting of this section and add an equivalent test for pdo_sqlsrv

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to get some idea from the deleted tests in #905

const char *pch = strchr(pstr, '}');
size_t i = 0;
while (pch != NULL && i < value_len) {
i = pch - pstr + 1;
if (i == value_len || (i < value_len && pstr[i] != '}')) {
return false;
}
i++; // skip the brace
pch = strchr(pch + 2, '}'); // continue searching
}
return true;
}

Expand Down
2 changes: 0 additions & 2 deletions source/shared/core_results.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -243,14 +243,12 @@ std::string getUTF8StringFromString( _In_z_ const SQLWCHAR* source )
{
// convert to regular character string first
char c_str[4] = "";
mbstate_t mbs;

SQLLEN i = 0;
std::string str;
while ( source[i] )
{
memset( c_str, 0, sizeof( c_str ) );
DWORD rc;
int cch = 0;
errno_t err = mplat_wctomb_s( &cch, c_str, sizeof( c_str ), source[i++] );
if ( cch > 0 && err == ERROR_SUCCESS )
Expand Down
3 changes: 0 additions & 3 deletions source/shared/core_sqlsrv.h
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ OACR_WARNING_POP
// constants for maximums in SQL Server
const int SS_MAXCOLNAMELEN = 128;
const int SQL_SERVER_MAX_FIELD_SIZE = 8000;
const int SQL_SERVER_MAX_PRECISION = 38;
const int SQL_SERVER_MAX_TYPE_SIZE = 0;
const int SQL_SERVER_MAX_PARAMS = 2100;
const int SQL_SERVER_MAX_MONEY_SCALE = 4;
Expand Down Expand Up @@ -998,8 +997,6 @@ class sqlsrv_context {
SQLSRV_ENCODING encoding_; // encoding of the context
};

const int SQLSRV_OS_VISTA_OR_LATER = 6; // major version for Vista

// maps an IANA encoding to a code page
struct sqlsrv_encoding {

Expand Down
2 changes: 1 addition & 1 deletion source/sqlsrv/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ zend_function_entry sqlsrv_functions[] = {
PHP_FE( sqlsrv_client_info, sqlsrv_client_info_arginfo )
PHP_FE( sqlsrv_server_info, sqlsrv_server_info_arginfo )
PHP_FE( sqlsrv_cancel, sqlsrv_cancel_arginfo )
PHP_FE( sqlsrv_free_stmt, sqlsrv_close_arginfo )
PHP_FE( sqlsrv_free_stmt, sqlsrv_free_stmt_arginfo )
PHP_FE( sqlsrv_field_metadata, sqlsrv_field_metadata_arginfo )
PHP_FE( sqlsrv_send_stream_data, sqlsrv_send_stream_data_arginfo )
PHP_FE( SQLSRV_SQLTYPE_BINARY, sqlsrv_sqltype_size_arginfo )
Expand Down
4 changes: 0 additions & 4 deletions source/sqlsrv/stmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ unsigned int current_log_subsystem = LOG_STMT;

// constants used as invalid types for type errors
const zend_uchar PHPTYPE_INVALID = SQLSRV_PHPTYPE_INVALID;
const int SQLTYPE_INVALID = 0;
const int SQLSRV_INVALID_PRECISION = -1;
const SQLUINTEGER SQLSRV_INVALID_SIZE = (~1U);
const int SQLSRV_INVALID_SCALE = -1;
Expand All @@ -51,8 +50,6 @@ const int SQLSRV_SIZE_MAX_TYPE = -1;
// constants for maximums in SQL Server
const int SQL_SERVER_MAX_FIELD_SIZE = 8000;
const int SQL_SERVER_MAX_PRECISION = 38;
const int SQL_SERVER_DEFAULT_PRECISION = 18;
const int SQL_SERVER_DEFAULT_SCALE = 0;

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

LOG_FUNCTION( "sqlsrv_field_metadata" );

Expand Down
70 changes: 70 additions & 0 deletions test/functional/sqlsrv/sqlsrv_escape_braces.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
--TEST--
Test that right braces are escaped correctly and that error messages are correct when they're not
--SKIPIF--
<?php require('skipif.inc'); ?>
--FILE--
<?php
$server = 'fakeserver';
$uid = 'sa';
$password = 'fakepassword';

// If the braces are fine, then we expect the connection to fail with a login timeout error
$braceError = "An unescaped right brace (}) was found";
$connError = (strtoupper(substr(php_uname('s'), 0, 3)) === 'WIN') ? "Could not open a connection to SQL Server" : "Login timeout expired";

// Every combination of one, two, three, or more right braces I can think of
$testStrings = array(array("}", $braceError),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As shown in #905 please add test cases with strings that consist of punctuation and/or space characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't really have anything to do with escaping braces though - the code changes only affect braces. Should there be a separate test for nonalphanumeric characters in strings?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok you can leave that. I think we have a backlog item created already

array("{", $connError),
array("{t}", $connError),
array("{}}", $braceError),
array("}}", $connError),
array("}}}", $braceError),
array("}}}}", $connError),
array("{}}}", $connError),
array("}{", $braceError),
array("}{{", $braceError),
array("test", $connError),
array("{test}", $connError),
array("{test", $connError),
array("test}", $braceError),
array("{{test}}", $braceError),
array("{{test}", $connError),
array("{{test", $connError),
array("test}}", $connError),
array("{test}}", $braceError),
array("test}}}", $braceError),
array("{test}}}", $connError),
array("{test}}}}", $braceError),
array("{test}}}}}", $connError),
array("{test}}}}}}", $braceError),
array("te}st", $braceError),
array("{te}st}", $braceError),
array("{te}}st}", $connError),
array("{te}}}st}", $braceError),
array("te}}s}t", $braceError),
array("te}}s}}t", $connError),
array("te}}}st", $braceError),
array("te}}}}st", $connError),
array("tes}}t", $connError),
array("te}s}}t", $braceError),
array("tes}}t}}", $connError),
array("tes}}t}}}", $braceError),
array("tes}t}}", $braceError),
);

foreach ($testStrings as $test) {

$conn = sqlsrv_connect($server, array('uid'=>$test[0], 'pwd'=>$password, 'LoginTimeout'=>1));

if (strpos(sqlsrv_errors()[0][2], $test[1]) === false) {
print_r("Wrong error message returned for test string ".$test[0].". Expected ".$test[1].", actual output:\n");
print_r(sqlsrv_errors());
}

unset($conn);
}

echo "Done.\n";
?>
--EXPECT--
Done.