Skip to content

Fix mysqlnd protocol errors #16421

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Fix mysqlnd protocol errors #16421

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cfffbfa
Remove dead code
kamil-tekiela Oct 13, 2024
84e2267
Set client error consistently in case of malformed packet
kamil-tekiela Oct 13, 2024
5c97e8c
Convert warning into a client error
kamil-tekiela Oct 13, 2024
4fdb417
Remove unnecessary warnings
kamil-tekiela Oct 13, 2024
240d885
Replace 3 more warnings with errors
kamil-tekiela Nov 21, 2024
5f09ee6
Add ERROR:
kamil-tekiela Nov 22, 2024
dcf44b6
Convert one more warning and add tests
kamil-tekiela Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ext/mysqli/tests/ghsa-h35g-vwh6-m678-auth-message.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,5 @@ print "done!";
[*] Sending - Server Greeting: 580000000a352e352e352d31302e352e31382d4d6172696144420003000000473e3f6047257c6700fef7080200ff81150000000000000f0000006c6b55463f49335f686c6431006d7973716c5f6e61746976655f70617373776f7264
[*] Received: 6900000185a21a00000000c0080000000000000000000000000000000000000000000000726f6f7400006d7973716c5f6e61746976655f70617373776f7264002c0c5f636c69656e745f6e616d65076d7973716c6e640c5f7365727665725f686f7374093132372e302e302e31
[*] Sending - Malicious OK Auth Response [Extract heap through buffer over-read]: 0900000200000002000000fcff

Warning: mysqli::__construct(): OK packet message length is past the packet size in %s on line %d
Unknown error while trying to connect via tcp://127.0.0.1:50001
OK packet message length is past the packet size
done!
13 changes: 5 additions & 8 deletions ext/mysqli/tests/ghsa-h35g-vwh6-m678-def.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@ $conn = new mysqli($servername, $username, $password, "", $port);

echo "[*] Running query on the fake server...\n";

$result = $conn->query("SELECT * from users");

if ($result) {
$all_fields = $result->fetch_fields();
var_dump($result->fetch_all(MYSQLI_ASSOC));
var_dump(get_object_vars($all_fields[0])["def"]);
try {
$result = $conn->query("SELECT * from users");
} catch (mysqli_sql_exception $exception) {
echo $exception->getMessage() . PHP_EOL;
}

$conn->close();
Expand All @@ -42,6 +40,5 @@ print "done!";
[*] Running query on the fake server...
[*] Received: 140000000353454c454354202a2066726f6d207573657273
[*] Sending - Malicious Tabular Response [Extract heap through buffer over-read]: 01000001011e0000020164016401640164016401640c3f000b000000030350000000fd000001aa05000003fe00002200040000040135017405000005fe00002200

Warning: mysqli::query(): Protocol error. Server sent default for unsupported field list (mysqlnd_wireprotocol.c:%d) in %s on line %d
Server sent default for unsupported field list
done!
12 changes: 7 additions & 5 deletions ext/mysqli/tests/ghsa-h35g-vwh6-m678-filename.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,12 @@ $process->wait();
$conn = new mysqli($servername, $username, $password, "", $port);
echo "[*] Running query on the fake server...\n";

$result = $conn->query("SELECT * from users");
try {
$result = $conn->query("SELECT * from users");
} catch (mysqli_sql_exception $exception) {
echo $exception->getMessage() . PHP_EOL;
}

$info = mysqli_info($conn);

var_dump($info);
Expand All @@ -35,9 +40,6 @@ print "done!";
[*] Running query on the fake server...
[*] Received: 140000000353454c454354202a2066726f6d207573657273
[*] Sending - Malicious Tabular Response [Extract heap through buffer over-read]: 0900000100000000000000fa65

Warning: mysqli::query(): RSET_HEADER packet additional data length is past 249 bytes the packet size in %s on line %d

Warning: mysqli::query(): Error reading result set's header in %s on line %d
RSET_HEADER packet additional data length is past the packet size
NULL
done!
2 changes: 2 additions & 0 deletions ext/mysqlnd/mysqlnd_ps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,8 @@ mysqlnd_stmt_read_prepare_response(MYSQLND_STMT * s)
conn->payload_decoder_factory->m.init_prepare_response_packet(&prepare_resp);

if (FAIL == PACKET_READ(conn, &prepare_resp)) {
// There might have been a connection specific error which we must report as a statement error
COPY_CLIENT_ERROR(stmt->error_info, *conn->error_info);
goto done;
}

Expand Down
3 changes: 0 additions & 3 deletions ext/mysqlnd/mysqlnd_result.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,9 +183,6 @@ mysqlnd_query_read_result_set_header(MYSQLND_CONN_DATA * conn, MYSQLND_STMT * s)
UPSERT_STATUS_SET_AFFECTED_ROWS_TO_ERROR(conn->upsert_status);

if (FAIL == (ret = PACKET_READ(conn, &rset_header))) {
if (conn->error_info->error_no != CR_SERVER_GONE_ERROR && conn->error_info->error_no != CR_CLIENT_INTERACTION_TIMEOUT) {
php_error_docref(NULL, E_WARNING, "Error reading result set's header");
}
break;
}

Expand Down
79 changes: 22 additions & 57 deletions ext/mysqlnd/mysqlnd_wireprotocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,9 +491,7 @@ php_mysqlnd_greet_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("GREET packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "GREET packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "GREET packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -738,8 +736,7 @@ php_mysqlnd_auth_response_read(MYSQLND_CONN_DATA * conn, void * _packet)
/* p can get past packet size when getting field length so it needs to be checked first
* and after that it can be checked that the net_len is not greater than the packet size */
if ((p - buf) > packet->header.size || packet->header.size - (p - buf) < net_len) {
DBG_ERR_FMT("OK packet message length is past the packet size");
php_error_docref(NULL, E_WARNING, "OK packet message length is past the packet size");
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "OK packet message length is past the packet size");
DBG_RETURN(FAIL);
}
packet->message_len = net_len;
Expand All @@ -756,9 +753,7 @@ php_mysqlnd_auth_response_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("OK packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "AUTH_RESPONSE packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "AUTH_RESPONSE packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -893,9 +888,7 @@ php_mysqlnd_ok_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("OK packet %zu bytes shorter than expected", p - begin - packet->header.size);
Copy link
Member

Choose a reason for hiding this comment

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

why are you getting rid of all DBG_ERR_FMT?

Copy link
Member

Choose a reason for hiding this comment

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

I guess that they will get logged from error (haven't checked though) but that bytes info can be useful there in some cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not all, just a couple that I saw now. Why are you asking? Are you using them? I think we should remove all of them, but it's a lot of work and since it's dead code, there's little harm in keeping them in the code. If someone ever needs to debug this piece of code, and they use this debug feature, they can add these lines themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case people don't know. DBG_ERR_FMT is a macro that can be enabled with some compiler option and when it is on, mysqlnd will spew out debugging information to CLI. If it's off, then the compiler simply discards these lines.

php_error_docref(NULL, E_WARNING, "OK packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "OK packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -980,9 +973,7 @@ php_mysqlnd_eof_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("EOF packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "EOF packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "EOF packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -1067,6 +1058,9 @@ php_mysqlnd_rset_header_read(MYSQLND_CONN_DATA * conn, void * _packet)
DBG_ENTER("php_mysqlnd_rset_header_read");

if (FAIL == mysqlnd_read_packet_header_and_body(&(packet->header), pfc, vio, stats, error_info, connection_state, buf, buf_len, "resultset header", PROT_RSET_HEADER_PACKET)) {
if (!error_info->error_no) {
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Error reading result set's header");
}
DBG_RETURN(FAIL);
}
BAIL_IF_NO_MORE_DATA;
Expand Down Expand Up @@ -1125,12 +1119,7 @@ php_mysqlnd_rset_header_read(MYSQLND_CONN_DATA * conn, void * _packet)
/* p can get past packet size when getting field length so it needs to be checked first
* and after that it can be checked that the len is not greater than the packet size */
if ((p - buf) > packet->header.size || packet->header.size - (p - buf) < len) {
size_t local_file_name_over_read = ((p - buf) - packet->header.size) + len;
DBG_ERR_FMT("RSET_HEADER packet additional data length is past %zu bytes the packet size",
local_file_name_over_read);
php_error_docref(NULL, E_WARNING,
"RSET_HEADER packet additional data length is past %zu bytes the packet size",
local_file_name_over_read);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "RSET_HEADER packet additional data length is past the packet size");
DBG_RETURN(FAIL);
}
packet->info_or_local_file.s = mnd_emalloc(len + 1);
Expand All @@ -1151,9 +1140,7 @@ php_mysqlnd_rset_header_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(ret);
premature_end:
DBG_ERR_FMT("RSET_HEADER packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "RSET_HEADER packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "RSET_HEADER packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -1229,8 +1216,7 @@ php_mysqlnd_rset_field_read(MYSQLND_CONN_DATA * conn, void * _packet)
DBG_RETURN(PASS);
} else if (EODATA_MARKER == *p && packet->header.size < 8) {
/* Premature EOF. That should be COM_FIELD_LIST. But we don't support COM_FIELD_LIST anymore, thus this should not happen */
DBG_ERR("Premature EOF. That should be COM_FIELD_LIST");
php_error_docref(NULL, E_WARNING, "Premature EOF in result field metadata");
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Premature EOF in result field metadata");
DBG_RETURN(FAIL);
}

Expand All @@ -1245,12 +1231,10 @@ php_mysqlnd_rset_field_read(MYSQLND_CONN_DATA * conn, void * _packet)

/* 1 byte length */
if (UNEXPECTED(12 != *p)) {
DBG_ERR_FMT("Protocol error. Server sent false length. Expected 12 got %d", (int) *p);
php_error_docref(NULL, E_WARNING, "Protocol error. Server sent false length. Expected 12");
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Protocol error. Server sent false length. Expected 12");
}

if ((size_t)((p - begin) + 12) > packet->header.size) {
php_error_docref(NULL, E_WARNING, "Premature end of data (mysqlnd_wireprotocol.c:%u)", __LINE__);
goto premature_end;
}

Expand Down Expand Up @@ -1288,10 +1272,7 @@ php_mysqlnd_rset_field_read(MYSQLND_CONN_DATA * conn, void * _packet)
(len = php_mysqlnd_net_field_length(&p)) &&
len != MYSQLND_NULL_LENGTH)
{
DBG_ERR_FMT("Protocol error. Server sent default for unsupported field list");
php_error_docref(NULL, E_WARNING,
"Protocol error. Server sent default for unsupported field list (mysqlnd_wireprotocol.c:%u)",
__LINE__);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Server sent default for unsupported field list");
DBG_RETURN(FAIL);
}

Expand Down Expand Up @@ -1349,14 +1330,10 @@ php_mysqlnd_rset_field_read(MYSQLND_CONN_DATA * conn, void * _packet)
DBG_RETURN(PASS);

faulty_or_fake:
DBG_ERR_FMT("Protocol error. Server sent NULL_LENGTH. The server is faulty");
php_error_docref(NULL, E_WARNING, "Protocol error. Server sent NULL_LENGTH."
" The server is faulty");
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Protocol error. Server sent NULL_LENGTH. The server is faulty");
DBG_RETURN(FAIL);
premature_end:
DBG_ERR_FMT("RSET field packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "Result set field packet %zu bytes "
"shorter than expected", p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Result set field packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -1862,9 +1839,9 @@ php_mysqlnd_prepare_read(MYSQLND_CONN_DATA * conn, void * _packet)

if (data_size != PREPARE_RESPONSE_SIZE_41 &&
data_size != PREPARE_RESPONSE_SIZE_50 &&
!(data_size > PREPARE_RESPONSE_SIZE_50)) {
data_size <= PREPARE_RESPONSE_SIZE_50) {
DBG_ERR_FMT("Wrong COM_STMT_PREPARE response size. Received %u", data_size);
php_error(E_WARNING, "Wrong COM_STMT_PREPARE response size. Received %u", data_size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Wrong COM_STMT_PREPARE response size");
DBG_RETURN(FAIL);
}

Expand Down Expand Up @@ -1896,9 +1873,7 @@ php_mysqlnd_prepare_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("PREPARE packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "PREPARE packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "PREPARE packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -1966,9 +1941,7 @@ php_mysqlnd_chg_user_read(MYSQLND_CONN_DATA * conn, void * _packet)

DBG_RETURN(PASS);
premature_end:
DBG_ERR_FMT("CHANGE_USER packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "CHANGE_USER packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "CHANGE_USER packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -2050,9 +2023,7 @@ php_mysqlnd_sha256_pk_request_response_read(MYSQLND_CONN_DATA * conn, void * _pa
DBG_RETURN(PASS);

premature_end:
DBG_ERR_FMT("OK packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "SHA256_PK_REQUEST_RESPONSE packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "SHA256_PK_REQUEST_RESPONSE packet is shorter than expected");
DBG_RETURN(FAIL);
}
/* }}} */
Expand Down Expand Up @@ -2163,9 +2134,7 @@ php_mysqlnd_cached_sha2_result_read(MYSQLND_CONN_DATA * conn, void * _packet)
DBG_RETURN(PASS);

premature_end:
DBG_ERR_FMT("OK packet %zu bytes shorter than expected", p - begin - packet->header.size);
php_error_docref(NULL, E_WARNING, "SHA256_PK_REQUEST_RESPONSE packet %zu bytes shorter than expected",
p - begin - packet->header.size);
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "SHA256_PK_REQUEST_RESPONSE packet is shorter than expected");
DBG_RETURN(FAIL);
}

Expand Down Expand Up @@ -2615,7 +2584,7 @@ static enum_func_status
MYSQLND_METHOD(mysqlnd_protocol, send_command_handle_response)(
MYSQLND_PROTOCOL_PAYLOAD_DECODER_FACTORY * payload_decoder_factory,
const enum mysqlnd_packet_type ok_packet,
const bool silent,
const bool silent, /* This is a dead parameter */
const enum php_mysqlnd_server_command command,
const bool ignore_upsert_status, /* actually used only by LOAD DATA. COM_QUERY and COM_EXECUTE handle the responses themselves */

Expand All @@ -2638,12 +2607,8 @@ MYSQLND_METHOD(mysqlnd_protocol, send_command_handle_response)(
break;
default:
SET_CLIENT_ERROR(error_info, CR_MALFORMED_PACKET, UNKNOWN_SQLSTATE, "Malformed packet");
php_error_docref(NULL, E_ERROR, "Wrong response packet %u passed to the function", ok_packet);
break;
}
if (!silent && error_info->error_no == CR_MALFORMED_PACKET) {
php_error_docref(NULL, E_WARNING, "Error while reading %s's response packet. PID=%d", mysqlnd_command_to_text[command], getpid());
}
DBG_INF(ret == PASS ? "PASS":"FAIL");
DBG_RETURN(ret);
}
Expand Down