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
![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.
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.
|
Kristian Nielsen ***@***.***> writes:
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.
To elaborate on this:
So, the backgroud is that the code in replication that reads/deserializes
replication events from the bytes read from binlog / relaylog, is
traditionally written to blindly trust that the events are not
corrupt/invalidly formatted. In many places there are no checks for eg.
too-short event, or such checks are done using assertions, meaning that
invalid events can cause out-of-buffer reads or other invalid memory
references.
And now, it seems an influx of bug reports are coming our way, with these
bugs being touted as "security bugs".
First, I want to encourage everybody to oppose the strong tendency to claim
that "this is a security issue, so my bug is more important than your bug".
Let's focus on the actual impact of the individual bug, and not play into
the agendas of any over-zealous "security experts".
Here is an example extract from a bug report:
The Format Description plays a critical role in determining field offsets
for ALL subsequent event constructors from its post_header_len array and
common_header_len fields. Corrupted header lengths will most likely
instruct subsequent event parsers to read their body data at incorrect
locations, possibly beyond their event buffers.
This means the impact is not just OOB reads: parsing corruption can lead
to a wide range of misbehaviours, most likely a crash.
An opening for RCE is also possible if the binlog data happens to line up
just right, though attack vectors (rogue replication source, MITM on
unencrypted replication, crafted binlog files) could have directly
injected Query events for ACE instead of engineering such through
corrupted Format Descriptions.)
Trying to claim an RCE (remote code execution) bug here is just nonsense.
An attacker that controls the contents of replication events already has the
ability to run arbitrary queries with full privileges, create arbitrary
accounts, etc. And corrupt header lengths read from invalid memory are no
different from corrupt header lengths contained inside an event. Let's work
to have a sober and objective description of the nature and impact of bugs,
and push back against reporters trying to make their report seem somehow
more important.
So, the slave basically has to trust the data received from a master, by the
nature of how replication works. I still consider it a bug that the code
doesn't check and prevent out-of-buffer reads. I've tried to avoid such bugs
in new code, but these bugs in old code have existed for decades. Should we
now prioritise fixing such bugs over other issues?
If we decide to fix such old bugs, I do not think it is productive to handle
then piece by piece with individual bug reports and CVEs and what not. It's
a general thing, when we find and fix a bug in one place in the code, we
should always think if similar bugs could exist elsewhere, and then fix it
all.
So should we go through the event reading code once and for all and fix any
missing buffer size checks etc? Is this something the Corporation will
prioritise, is it something I should allocate time for, or what do you
think?
- Kristian.
|
|
ParadoxV5 ***@***.***> writes:
1. Fixes [MDEV-40365](https://jira.mariadb.org/browse/MDEV-40365)
2. Fixes [MDEV-40366](https://jira.mariadb.org/browse/MDEV-40366)
3. Fixes [MDEV-39485](https://jira.mariadb.org/browse/MDEV-39485)
Hi Jimmy,
Here is my review of the patches for these MDEVs, a total of 3 commits:
I read through the patches in some detail, and it looks like quite a good
effort, thanks! The patch generally looks good.
The only change I will ask for is to mark all the added error checks with
unlikely(), for example:
if (unlikely((end - pos) <= EXTRA_ROW_INFO_LEN_OFFSET))
The reason for this is to reduce the impact of adding over time a lot of
extra conditional jumps to the generated code, which will never be true in
normal operation. By using the unlikely() macro on these, the compiler will
be able to generate code such that the generated conditional branch will be
not-taken in the common case of no error/corrupt event. A taken branch can
be significantly more expensive than a not-taken branch since it reduces the
effectiveness of the instruction cache (the part of the cacheline following
the taken branch is wasted). Instruction cache is a common bottleneck for
CPU performance in MariaDB.
Now for the code parsing format description events, this is of minimal
impact since this type of event is uncommon, but I still suggest to use the
unlikely() macro everywhere in event parsing code just for consistency.
Other than this, just a few very minor comments/suggestions:
+ switch (event_type) {
// all following START events in the current file are without checksum
- if (event_type == START_EVENT_V3)
+ case START_EVENT_V3:
(const_cast< Format_description_log_event *>(fdle))->checksum_alg= BINLOG_CHECKSUM_ALG_OFF;
+ // fall-through
I found this unusual switch() hard to read, with fall-through and case
default: in the middle.
Couldn't this be simpler just like this?
if (event_type == FORMAT_DESCRIPTION_EVENT) {
...
} else {
if (event_type == START_EVENT_V3) {
...
}
...
}
+ ulong checksum_alg_offset=
+ len - BINLOG_CHECKSUM_LEN - BINLOG_CHECKSUM_ALG_DESC_LEN;
+ if (checksum_alg_offset < POST_HEADER_LEN_OFFSET)
+ DBUG_RETURN(true);
If len would be less than 5, this would underflow and the check would be
incorrect, therefore I usually avoid subtraction and checking for less-than,
instead checking eg. BINLOG_CHECKSUM_LEN + BINLOG_CHECKSUM_ALG_DESC_LEN +
POST_HEADER_LEN_OFFSET > len.
However, in this case the code is correct because of this check above:
+ if (len < POST_HEADER_LEN_OFFSET)
+ DBUG_RETURN(true);
So just a general warning about underflow (which you may already be aware
of), up to you if you want to change it here.
- if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
+ if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT &&
+ get_checksum_alg(buf, event_len, &checksum_alg))
{
- checksum_alg= get_checksum_alg(buf, event_len);
+ error= ER_SLAVE_RELAY_LOG_WRITE_FAILURE;
+ unlock_data_lock= FALSE;
+ goto err;
}
else if (buf[EVENT_TYPE_OFFSET] == START_EVENT_V3)
This is a very trivial point, but the logic here should really be:
if (buf[EVENT_TYPE_OFFSET] == FORMAT_DESCRIPTION_EVENT)
{
if (get_checksum_alg(buf, event_len, &checksum_alg))
{
error= ER_SLAVE_RELAY_LOG_WRITE_FAILURE;
unlock_data_lock= FALSE;
goto err;
}
}
else if (buf[EVENT_TYPE_OFFSET] == START_EVENT_V3)
- Kristian.
|
Not quite. An attacker that controls the contents of replication events already has the ability to run arbitrary queries with full privileges, create arbitrary accounts, etc. But he cannot access files outside of |
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.
…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.
4 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.