Skip to content

Commit 4161768

Browse files
authored
fix(mysql): harden protocol decoders against malformed packets from hostile servers (#4458)
Found during a security review of server-controlled parsing paths (threat model: rogue or MITM MySQL/MariaDB server; handshake parsing is reachable pre-authentication). ### Does your PR solve an issue? No tracking issue — reported here directly. This extends the fix pattern of #4176 (truncated `OK` packet) to the rest of the protocol; #4451 documents the same review on the PostgreSQL side. ### The problem - `recv_packet`'s continuation loop accumulated an unbounded number of `0xFFFFFF`-sized parts: a hostile server streams endless 16 MiB frames and the client OOMs at near-zero bandwidth cost. - The result-set column count (a length-encoded u64) fed `HashMap::with_capacity` / `Vec::reserve` directly; a ~10-byte packet claiming 2^40 columns aborts the process. - Dozens of decode sites call panicking `bytes::Buf` getters / slice indexing / `advance` on untrusted buffers: truncated ERR packets, every field of `HandshakeV10` (including the attacker-controlled auth-plugin-data length), length-encoded strings claiming more than the remaining bytes, `TextRow` field-count mismatch, `BinaryRow` (null bitmap, temporal length byte, fixed-width and lenenc value bounds — plus an `unreachable!()` actually reachable via a NULL-typed column), short EOF packets, and a 1-byte `caching_sha2_password` continue packet. The OkPacket decoder was hardened in #4176; the rest of the protocol was not. ### The change - Cumulative 1 GiB cap on packet reassembly (`MySql`'s own `max_allowed_packet` hard ceiling); protocol error beyond it. - Result sets claiming more than 4096 columns (the MySQL server hard limit) are protocol errors before any allocation. - All listed sites check remaining length up front and return `Error::Protocol` describing expectation vs. reality; the `unreachable!()` is now an error. ### Is this a breaking change? No: every changed path converts a panic or abort into a returned `Err`, which callers already handle (the pool discards the connection). Newly-observable edges: aggregate packets over 1 GiB and result sets over 4096 columns are now errors rather than allocation attempts. ### Regression tests 15 new unit tests, including a sweep asserting that every strict prefix of captured MySQL 8.0.18 and MariaDB 10.4.7 handshake packets now errors instead of panicking. Each test fails (panics) without the fix. `cargo test -p sqlx-mysql --lib` passes.
1 parent 39eff06 commit 4161768

9 files changed

Lines changed: 433 additions & 5 deletions

File tree

‎sqlx-mysql/src/connection/auth.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,13 @@ impl AuthPlugin {
4242
) -> Result<bool, Error> {
4343
match self {
4444
AuthPlugin::CachingSha2Password if packet[0] == 0x01 => {
45+
if packet.len() < 2 {
46+
return Err(err_protocol!(
47+
"expected at least 2 bytes in caching_sha2_password authentication packet but got {}",
48+
packet.len()
49+
));
50+
}
51+
4552
match packet[1] {
4653
// fast_auth_success — the server still sends a trailing
4754
// OK_Packet, so yield back to the handshake loop and let

‎sqlx-mysql/src/connection/executor.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,15 @@ async fn recv_result_metadata(
435435
// the result-set metadata is primarily a listing of each output
436436
// column in the result-set
437437

438+
// MySQL (and MariaDB) enforce a hard limit of 4096 columns per result set;
439+
// reject larger claimed counts before they are used to size allocations,
440+
// as a hostile server could otherwise force an arbitrarily large allocation
441+
if num_columns > 4096 {
442+
return Err(err_protocol!(
443+
"result-set metadata claims {num_columns} columns but the maximum supported is 4096"
444+
));
445+
}
446+
438447
let mut column_names = HashMap::with_capacity(num_columns);
439448

440449
columns.clear();

‎sqlx-mysql/src/connection/stream.rs‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ use crate::protocol::response::{EofPacket, ErrPacket, OkPacket, Status};
1111
use crate::protocol::{Capabilities, Packet};
1212
use crate::{MySqlConnectOptions, MySqlDatabaseError};
1313

14+
/// The maximum total payload that we accept for a single (possibly reassembled)
15+
/// packet: 1 GiB, MySQL's own hard maximum for `max_allowed_packet`.
16+
///
17+
/// Without this cap, a hostile server could stream an unbounded number of
18+
/// 16 MiB packet parts and exhaust the client's memory.
19+
const MAX_PACKET_PAYLOAD_LEN: usize = 1024 * 1024 * 1024;
20+
1421
pub struct MySqlStream<S = Box<dyn Socket>> {
1522
// Wrapping the socket in `Box` allows us to unsize in-place.
1623
pub(crate) socket: BufferedSocket<S>,
@@ -155,6 +162,14 @@ impl<S: Socket> MySqlStream<S> {
155162
while last_read == 0xFF_FF_FF {
156163
let part = self.recv_packet_part().await?;
157164
last_read = part.len();
165+
166+
if final_payload.len() + part.len() > MAX_PACKET_PAYLOAD_LEN {
167+
return Err(err_protocol!(
168+
"reassembled packet payload of {} bytes exceeds maximum packet payload of {MAX_PACKET_PAYLOAD_LEN} bytes",
169+
final_payload.len() + part.len()
170+
));
171+
}
172+
158173
final_payload.extend_from_slice(&part);
159174
}
160175
final_payload.into()

‎sqlx-mysql/src/io/buf.rs‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,13 @@ impl MySqlBufExt for Bytes {
6262
let size = usize::try_from(size)
6363
.map_err(|_| err_protocol!("string length overflows usize: {size}"))?;
6464

65+
if size > self.remaining() {
66+
return Err(err_protocol!(
67+
"length-encoded string declares {size} bytes but only {} bytes remain",
68+
self.remaining()
69+
));
70+
}
71+
6572
self.get_str(size)
6673
}
6774

@@ -70,6 +77,54 @@ impl MySqlBufExt for Bytes {
7077
let size = usize::try_from(size)
7178
.map_err(|_| err_protocol!("string length overflows usize: {size}"))?;
7279

80+
if size > self.remaining() {
81+
return Err(err_protocol!(
82+
"length-encoded byte string declares {size} bytes but only {} bytes remain",
83+
self.remaining()
84+
));
85+
}
86+
7387
Ok(self.split_to(size))
7488
}
7589
}
90+
91+
#[cfg(test)]
92+
mod tests {
93+
use super::*;
94+
95+
#[test]
96+
fn get_str_lenenc_reads_within_bounds() {
97+
let mut buf = Bytes::from_static(&[3, b'a', b'b', b'c']);
98+
99+
assert_eq!(buf.get_str_lenenc().unwrap(), "abc");
100+
assert!(buf.is_empty());
101+
}
102+
103+
#[test]
104+
fn get_str_lenenc_rejects_oversized_length() {
105+
// 0xfc prefixed length of 0x0100 (256) but only 3 bytes of data
106+
let mut buf = Bytes::from_static(&[0xfc, 0x00, 0x01, b'a', b'b', b'c']);
107+
108+
let err = buf.get_str_lenenc().unwrap_err();
109+
110+
assert!(err.to_string().contains("256 bytes"), "{err}");
111+
}
112+
113+
#[test]
114+
fn get_bytes_lenenc_reads_within_bounds() {
115+
let mut buf = Bytes::from_static(&[3, b'a', b'b', b'c']);
116+
117+
assert_eq!(&*buf.get_bytes_lenenc().unwrap(), b"abc");
118+
assert!(buf.is_empty());
119+
}
120+
121+
#[test]
122+
fn get_bytes_lenenc_rejects_oversized_length() {
123+
// 0xfc prefixed length of 0xffff (65535) but only 1 byte of data
124+
let mut buf = Bytes::from_static(&[0xfc, 0xff, 0xff, b'a']);
125+
126+
let err = buf.get_bytes_lenenc().unwrap_err();
127+
128+
assert!(err.to_string().contains("65535 bytes"), "{err}");
129+
}
130+
}

‎sqlx-mysql/src/protocol/connect/handshake.rs‎

Lines changed: 101 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,29 +29,74 @@ pub(crate) struct Handshake {
2929

3030
impl ProtocolDecode<'_> for Handshake {
3131
fn decode_with(mut buf: Bytes, _: ()) -> Result<Self, Error> {
32+
// NOTE: the handshake packet is received before authentication, so a
33+
// hostile server can send anything here; every fixed-width read is
34+
// validated against the remaining buffer to avoid panicking on
35+
// truncated packets
36+
if buf.is_empty() {
37+
return Err(err_protocol!(
38+
"expected protocol version (1 byte) in Handshake packet but got 0 bytes"
39+
));
40+
}
41+
3242
let protocol_version = buf.get_u8(); // int<1>
3343
let server_version = buf.get_str_nul()?; // string<NUL>
44+
45+
if buf.remaining() < 13 {
46+
return Err(err_protocol!(
47+
"expected connection id (4 bytes), auth-plugin-data-part-1 (8 bytes) and reserved byte (13 bytes total) in Handshake packet but got {} bytes",
48+
buf.remaining()
49+
));
50+
}
51+
3452
let connection_id = buf.get_u32_le(); // int<4>
3553
let auth_plugin_data_1 = buf.get_bytes(8); // string<8>
3654

3755
buf.advance(1); // reserved: string<1>
3856

57+
if buf.remaining() < 2 {
58+
return Err(err_protocol!(
59+
"expected capability flags part 1 (2 bytes) in Handshake packet but got {} bytes",
60+
buf.remaining()
61+
));
62+
}
63+
3964
let capabilities_1 = buf.get_u16_le(); // int<2>
4065
let mut capabilities = Capabilities::from_bits_truncate(capabilities_1.into());
4166

67+
if buf.remaining() < 5 {
68+
return Err(err_protocol!(
69+
"expected collation (1 byte), status (2 bytes) and capability flags part 2 (2 bytes) in Handshake packet but got {} bytes",
70+
buf.remaining()
71+
));
72+
}
73+
4274
let collation = buf.get_u8(); // int<1>
4375
let status = Status::from_bits_truncate(buf.get_u16_le());
4476

4577
let capabilities_2 = buf.get_u16_le(); // int<2>
4678
capabilities |= Capabilities::from_bits_truncate(((capabilities_2 as u32) << 16).into());
4779

80+
if buf.is_empty() {
81+
return Err(err_protocol!(
82+
"expected auth-plugin-data length (1 byte) in Handshake packet but got 0 bytes"
83+
));
84+
}
85+
4886
let auth_plugin_data_len = if capabilities.contains(Capabilities::PLUGIN_AUTH) {
4987
buf.get_u8()
5088
} else {
5189
buf.advance(1); // int<1>
5290
0
5391
};
5492

93+
if buf.remaining() < 10 {
94+
return Err(err_protocol!(
95+
"expected reserved bytes (6 bytes) and reserved bytes or capability flags part 3 (4 bytes) in Handshake packet but got {} bytes",
96+
buf.remaining()
97+
));
98+
}
99+
55100
buf.advance(6); // reserved: string<6>
56101

57102
if capabilities.contains(Capabilities::MYSQL) {
@@ -62,8 +107,18 @@ impl ProtocolDecode<'_> for Handshake {
62107
}
63108

64109
let auth_plugin_data_2 = if capabilities.contains(Capabilities::SECURE_CONNECTION) {
65-
let len = cmp::max(auth_plugin_data_len.saturating_sub(9), 12);
66-
let v = buf.get_bytes(len as usize);
110+
let len = cmp::max(auth_plugin_data_len.saturating_sub(9), 12) as usize;
111+
112+
// the attacker-controlled length must fit in the remaining buffer
113+
// *with* the trailing NUL-terminator, i.e. `len + 1` bytes
114+
if buf.remaining() <= len {
115+
return Err(err_protocol!(
116+
"expected auth-plugin-data-part-2 ({len} bytes) and NUL-terminator in Handshake packet but got {} bytes",
117+
buf.remaining()
118+
));
119+
}
120+
121+
let v = buf.get_bytes(len);
67122
buf.advance(1); // NUL-terminator
68123

69124
v
@@ -197,3 +252,47 @@ fn test_decode_handshake_mariadb_10_4_7() {
197252
&[116, 54, 76, 92, 106, 34, 100, 83, 85, 49, 52, 79, 112, 104, 57, 34, 60, 72, 53, 110,]
198253
);
199254
}
255+
256+
#[test]
257+
fn test_decode_handshake_truncated_never_panics() {
258+
const MYSQL_8_0_18: &[u8] = b"\n8.0.18\x00\x19\x00\x00\x00\x114aB0c\x06g\x00\xff\xff\xff\x02\x00\xff\xc7\x15\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00tL\x03s\x0f[4\rl4. \x00caching_sha2_password\x00";
259+
const MARIA_DB_10_4_7: &[u8] = b"\n5.5.5-10.4.7-MariaDB-1:10.4.7+maria~bionic\x00\x0b\x00\x00\x00t6L\\j\"dS\x00\xfe\xf7\x08\x02\x00\xff\x81\x15\x00\x00\x00\x00\x00\x00\x07\x00\x00\x00U14Oph9\"<H5n\x00mysql_native_password\x00";
260+
261+
for packet in [MYSQL_8_0_18, MARIA_DB_10_4_7] {
262+
// every strict prefix of a valid handshake is an error, never a panic
263+
for len in 0..packet.len() {
264+
assert!(
265+
Handshake::decode(packet[..len].into()).is_err(),
266+
"expected error for handshake truncated to {len} bytes"
267+
);
268+
}
269+
270+
// and the full packet still decodes
271+
assert!(Handshake::decode(packet.into()).is_ok());
272+
}
273+
}
274+
275+
#[test]
276+
fn test_decode_handshake_rejects_oversized_auth_plugin_data_len() {
277+
// 0xc7ff capability flags part 2 sets SECURE_CONNECTION and PLUGIN_AUTH,
278+
// so the auth-plugin-data length byte governs the part-2 read; it claims
279+
// 255 bytes (246 after subtracting 9) but only a few bytes follow
280+
let mut packet = Vec::new();
281+
packet.push(10_u8); // protocol version
282+
packet.extend_from_slice(b"8.0.0\0"); // server version
283+
packet.extend_from_slice(&1_u32.to_le_bytes()); // connection id
284+
packet.extend_from_slice(b"01234567"); // auth-plugin-data-part-1
285+
packet.push(0); // reserved
286+
packet.extend_from_slice(&0xffff_u16.to_le_bytes()); // capability flags part 1
287+
packet.push(0); // collation
288+
packet.extend_from_slice(&2_u16.to_le_bytes()); // status
289+
packet.extend_from_slice(&0xc7ff_u16.to_le_bytes()); // capability flags part 2
290+
packet.push(255); // auth-plugin-data length
291+
packet.extend_from_slice(&[0; 6]); // reserved
292+
packet.extend_from_slice(&[0; 4]); // reserved
293+
packet.extend_from_slice(b"short-junk\0");
294+
295+
let err = Handshake::decode(packet.into()).unwrap_err();
296+
297+
assert!(err.to_string().contains("auth-plugin-data-part-2"), "{err}");
298+
}

‎sqlx-mysql/src/protocol/response/eof.rs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,36 @@ impl ProtocolDecode<'_, Capabilities> for EofPacket {
2828
));
2929
}
3030

31+
if buf.remaining() < 4 {
32+
return Err(err_protocol!(
33+
"expected warnings (2 bytes) and status flags (2 bytes) in EOF_Packet but got {} bytes",
34+
buf.remaining()
35+
));
36+
}
37+
3138
let warnings = buf.get_u16_le();
3239
let status = Status::from_bits_truncate(buf.get_u16_le());
3340

3441
Ok(Self { status, warnings })
3542
}
3643
}
44+
45+
#[test]
46+
fn test_decode_eof_packet() {
47+
let p = EofPacket::decode_with(
48+
b"\xfe\x00\x00\x02\x00".as_ref().into(),
49+
Capabilities::empty(),
50+
)
51+
.unwrap();
52+
53+
assert_eq!(p.warnings, 0);
54+
assert!(p.status.contains(Status::SERVER_STATUS_AUTOCOMMIT));
55+
}
56+
57+
#[test]
58+
fn test_decode_eof_packet_truncated() {
59+
// only 2 bytes after the 0xfe header, need 4
60+
let p = EofPacket::decode_with(b"\xfe\x00\x00".as_ref().into(), Capabilities::empty());
61+
62+
assert!(p.is_err());
63+
}

‎sqlx-mysql/src/protocol/response/err.rs‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,28 @@ impl ProtocolDecode<'_, Capabilities> for ErrPacket {
2525
));
2626
}
2727

28+
if buf.remaining() < 2 {
29+
return Err(err_protocol!(
30+
"expected error code (2 bytes) in ERR_Packet but got {} bytes",
31+
buf.remaining()
32+
));
33+
}
34+
2835
let error_code = buf.get_u16_le();
2936
let mut sql_state = None;
3037

3138
if capabilities.contains(Capabilities::PROTOCOL_41) {
3239
// If the next byte is '#' then we have a SQL STATE
3340
if buf.starts_with(b"#") {
3441
buf.advance(1);
42+
43+
if buf.remaining() < 5 {
44+
return Err(err_protocol!(
45+
"expected SQL state (5 bytes) after '#' marker in ERR_Packet but got {} bytes",
46+
buf.remaining()
47+
));
48+
}
49+
3550
sql_state = Some(buf.get_str(5)?);
3651
}
3752
}
@@ -69,3 +84,25 @@ fn test_decode_err_packet_unknown_database() {
6984
assert_eq!(p.sql_state.as_deref(), Some("42000"));
7085
assert_eq!(&p.error_message, "Unknown database \'unknown\'");
7186
}
87+
88+
#[test]
89+
fn test_decode_err_packet_truncated_error_code() {
90+
// only 1 byte of the 2 byte error code is present
91+
const ERR_TRUNCATED_CODE: &[u8] = b"\xff\x84";
92+
93+
let err =
94+
ErrPacket::decode_with(ERR_TRUNCATED_CODE.into(), Capabilities::PROTOCOL_41).unwrap_err();
95+
96+
assert!(err.to_string().contains("expected error code"), "{err}");
97+
}
98+
99+
#[test]
100+
fn test_decode_err_packet_truncated_sql_state() {
101+
// '#' marker but fewer than 5 bytes of SQL state
102+
const ERR_TRUNCATED_STATE: &[u8] = b"\xff\x84\x04#420";
103+
104+
let err =
105+
ErrPacket::decode_with(ERR_TRUNCATED_STATE.into(), Capabilities::PROTOCOL_41).unwrap_err();
106+
107+
assert!(err.to_string().contains("expected SQL state"), "{err}");
108+
}

0 commit comments

Comments
 (0)