Fix OOB reads upon malformed Format Description and v2 Rows Events#5419
Fix OOB reads upon malformed Format Description and v2 Rows Events#5419ParadoxV5 wants to merge 3 commits into
Conversation
The binary-parsing Format Description Event constructor did not validate content length beyond the superclass `is_valid()` call. When parsing a malformed FDE, to load the content fields added in FDE v4 that are missing in this not-FDE, the parser constructor would read from erroneous memory locations beyond the buffer. If this did not outright crash the program, this would corrupt the FDE. With the Format Description playing a critical role in determining how to parse the events to follow, a corrupted FDE would also corrupt (or trigger a crash in) the parsing of subsequent non-FDE events as well. This commit fills in the validation with a FDE-specific guard. It adds the FDE constant `ST_POST_HEADER_LEN_OFFSET` to assist with comparing to the correct minimum size in the future. (Both of these points are designed to merge with the superclass’s guard as part of the MDEV-30128 merger).
Neither the binary-parsing Format Description Event constructor nor the constructor-bypassing `get_checksum_alg()` function validated the content length of the passed event buffer. If they receive an FDE with undersized contents, they would obtain corrupt results from an erronous memory location, if not outright crash the program with that memory error. This commit fixes both sites by adding content length checks. Because the goal is not to solve the existence of two binary parsers, `get_checksum_alg()` receives a check duplicated from the Format Description constructor and is no longer a function that never errors.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses out-of-bounds reads and heap-buffer-overflow vulnerabilities (MDEV-39485, MDEV-40365, MDEV-40366) when parsing malformed binary log events, specifically Format_description_log_event and Rows_log_event. It introduces stricter bounds checks, refactors get_checksum_alg to safely return a boolean status, and adds corresponding regression tests. The reviewer suggested an improvement in Rows_log_event to ensure infoLen is at least EXTRA_ROW_INFO_HDR_BYTES to prevent potential infinite loops or out-of-bounds reads when parsing invalid extra row info headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ParadoxV5
left a comment
ParadoxV5
left a comment
There was a problem hiding this comment.
An issue with the tests are, because these low-level bugs impact every event stream, we “should” test all of those streams just because there is no dedicated unit test for the Events library.
This include Binlog Dump thread, IO thread, SQL thread, mariadb-binlog, SHOW BINLOG STATUS…
There was a problem hiding this comment.
Since MariaDB does not generate v2 Rows Events, here’s a handcrafted binlog file.
| bool get_checksum_alg(const uchar *buf, ulong len, | ||
| enum enum_binlog_checksum_alg *alg) |
There was a problem hiding this comment.
Andrei made a curious suggestion to always checksum FDEs.
Whether that’s a good idea aside, the entire get_checksum_alg() would not be needed if that means we fix FDEs to CRC32.
The current code design is rather silly:
We must first parse the FDE with get_checksum_alg(), checksum (or at least strip it) it accordingly, and then use the constructor to parse it again.
| if (event_len < LOG_EVENT_MINIMAL_HEADER_LEN + ST_POST_HEADER_LEN_OFFSET || | ||
| !Start_log_event_v3::is_valid()) |
There was a problem hiding this comment.
Merge Conflict with MDEV-30128 «remove support for 5.1- replication events» where Start_log_event_v3 was merged into Format_description_log_event
| # the second one has a bogus tagged data length, both have 0 columns. | ||
| --let SEARCH_PATTERN= [Uu]nknown [Ee]vent | ||
| --let SEARCH_FILE= $MYSQLTEST_VARDIR/tmp/invalid_row_v2_tag.sql | ||
| --exec $MYSQL_BINLOG --force-read --verbose suite/binlog/std_data/invalid_row_v2_tag.001 > $SEARCH_FILE |
There was a problem hiding this comment.
Testing with mariadb-binlog --verbose instead of the server because so far this client is the only one that actually cares about the extra data.
…ctor MariaDB recognizes Version 2 Rows Events from MySQL, including the format of the “extra data” field added in this version. (MDEV-5115) When parsing this extra data according to the format, whether this data has sufficient length was only checked by assertions in the `Rows_log_event` constructor and the `mariadb-binlog --verbose` printer. When parsing an event with malformed extra data, these assertions * would straight up terminate the program in debug builds. * were stripped in non-debug (release) builds. This would render the parser defenseless to reading from erroneous memory locations outside of the containing event, which will either crash the program or, for `mariadb-binlog --verbose`, snapshot the running memory to be exposed when outputting the event. This commit replaces those assertions with an actual validity check. Since MariaDB does not generate v2 Rows Events, the included test uses a handcrafted binlog file.
knielsen
left a comment
knielsen
left a comment
There was a problem hiding this comment.
It feels this is getting out of hand.
The MariaDB replication code is not written to be robust to arbitrary and/or malicious corrupted replication events. Like it or not, this is how it is currently. Obviously robust is better, but there are many other problems with the replication code as well.
We must avoid that replication development ends up being controlled by people shouting "security issue" or "CVE".
The replication slave has to trust the master to send valid data, this is fundamental to how it works. mysqlbinlog ideally shouldn't have to require such trust (though mysqlbinlog|mysql must), but I believe we have more pressing priorities. Such as a severe regression in 12.3 that silently corrupts replication configuration and potentially data.
So again design review: We need a strategic decision on what guarantees exactly replication can, and cannot give on corrupt/malicious event data, and how to avoid spending unwanted efforts on such issues.
3 participants
When parsing the v4 fields in Format Description and the extra data in v2 Rows Events in their parser constructors, whether the buffers had sufficient lengths was not properly checked for errors. (Details in individual commit messages)
This means parsing malformed events of those types can read from erroneous memory locations beyond the buffer, which will either crash the program or lead to eventual memory disclosure.
The Format Description function
get_checksum_alg()also doesn’t have the checks, though this function does not lead to memory disclosure.This PR fixes all of the above locations by adding content length checks.