Improve handling of malformed COM_STMT_EXECUTE packets#5188
Conversation
Summary of ChangesHello @JavierJF, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and compatibility of ProxySQL's handling of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces important improvements for handling malformed COM_STMT_EXECUTE packets. The addition of boundary checks in get_binds_from_pkt is a critical fix for security and stability. The change in parameter length decoding to align with MySQL's behavior is a good compatibility improvement. The code is well-structured, and the introduction of mysql_decode_length_ll to separate the 64-bit decoding logic is clean. The new test file is comprehensive and covers the new logic well. I have a few suggestions to improve code readability and maintainability.
|
Can one of the admins verify this patch? |
|
Add to whitelist |
|
retest this please |
These checks should prevent crashes when packets are received holding invalid lengths in the processed params.
According to MySQL protocol, variable length strings are encoded using length encoded integers. For reference, see: - https://dev.mysql.com/doc/dev/mysql-server/9.4.0/page_protocol_com_stmt_execute.html - https://dev.mysql.com/doc/dev/mysql-server/9.4.0/page_protocol_basic_dt_integers.html#a_protocol_type_int2 The protocol specifies that values greater than 2^24 (16777216) should be encoded using '0xFE + 8-byte integer'. Yet, in reality MySQL ignores the upper section of these 8-byte integers, treating them effectively like '4-bytes'. For the sake of compatibility this commit changes the decoding behavior for 'COM_STMT_EXECUTE' to match MySQL one. This different is subtle but important, since in practice MySQL itself doesn't use the '8 bytes' from the field. This means that connectors that are compatible with MySQL could find issues when sending these packets through ProxySQL (like NodeJS 'mysql2' connector which writes the 8-bytes as a 4-bytes duplication, motivating these changes), situation that could result in rejection due to malformed packet detection (or crashes/invalid handling in the worse case scenario). The previous decoding function is now renamed into 'mysql_decode_length_ll' to honor MySQL naming 'net_field_length_ll'. For now, this protocol change is limited to 'COM_STMT_EXECUTE'.
The test exercises the boundary checks for 'COM_STMT_EXECUTE' params decoding and ensures the correct processing of malformed packets which only mangle the upper '4-bytes' of the param length-encoded integer.
Signed-off-by: Miro Stauder <miro@proxysql.com>
Connection requires root credentials for modifying 'max_allowed_packet'.
Since the test requires to modify 'max_allowed_packet' to work, it cannot reuse previous connections from the conn_pool. New connections must be created, to ensure that the new defaults are used.
Replace 'MYSQL_QUERY' macro with verbose alternative 'MYSQL_QUERY_T'.
08a8e05 to
1a874de
Compare
|
This PR introduces two changes to improve the handle of malformed (according to protocol)
COM_STMT_EXECUTEpackets:COM_STMT_EXECUTEpackets.COM_STMT_EXECUTEpackets which have parameters with length bigger than2^24. In this scenario, ProxySQL was decoding packets using8-bytelength encoded integers, as protocol specifies. MySQL itself isn't honoring that description, and it's decoding parameter length using only the lower4-bytesfrom the8-bytesinteger. This change honors MySQL behavior and improves compatibility with clients that abuse this behavior. As an example of this, NodeJS connectormysql2is known to have this behavior.