From dd53a71ce204017ba9dedea5058c6a9f3523b7bd Mon Sep 17 00:00:00 2001 From: Juliana Fajardini Date: Tue, 8 Sep 2026 15:01:46 -0300 Subject: [PATCH] pgsql: better boundaries to row_description/data_row parse_row_description and parse_consolidated_data_row read a field_count from the message body and use that to iterate a slice, without checking whether the count actually fits the body. A backend-controlled attack could lead to fewer fields logged, or stalling of the parser to-client direction for the duration of the flow. A malformed_response event is set for pgsql, if either of those message's inner parsers has a field_count that can't be reconciled with the body slice. A new pgsql.response log field is added to indicate this, too: pgsql.response.row_description_malformed, a boolean. This wasn't added to the consolidated data row message, per its summarization nature. The row_description unit test buffer declared a length one byte longer than its three fields occupy. The old code silently discarded that surplus byte; the new one reports it, so the buffer was corrected. Ticket: #8885 (cherry picked from commit fe939e8da7203bab461114e2258ad386e9346284) --- etc/schema.json | 4 ++ rust/src/pgsql/logger.rs | 5 ++ rust/src/pgsql/parser.rs | 99 +++++++++++++++++++++++++++++----------- rust/src/pgsql/pgsql.rs | 6 +++ 4 files changed, 87 insertions(+), 27 deletions(-) diff --git a/etc/schema.json b/etc/schema.json index 5126af75ee..057048f94f 100644 --- a/etc/schema.json +++ b/etc/schema.json @@ -4770,6 +4770,10 @@ "routine": { "type": "string" }, + "row_description_malformed": { + "type": "boolean", + "description": "True when the field count declared in a RowDescription message disagreed with the message body delimited by its length field" + }, "secret_key": { "type": "integer" }, diff --git a/rust/src/pgsql/logger.rs b/rust/src/pgsql/logger.rs index 80e96b55a9..8dee604f19 100644 --- a/rust/src/pgsql/logger.rs +++ b/rust/src/pgsql/logger.rs @@ -125,6 +125,7 @@ fn log_request(tx: &PgsqlTransaction, flags: u32, js: &mut JsonBuilder) -> Resul identifier: _, row_cnt, data_size, + .. }) => { js.open_object(req.to_str())?; js.set_uint("msg_count", *row_cnt)?; @@ -279,6 +280,7 @@ fn log_response(res: &PgsqlBEMessage, jb: &mut JsonBuilder) -> Result<(), JsonEr identifier: _, row_cnt, data_size, + .. }) => { jb.open_object(res.to_str())?; jb.set_uint("row_count", *row_cnt)?; @@ -290,13 +292,16 @@ fn log_response(res: &PgsqlBEMessage, jb: &mut JsonBuilder) -> Result<(), JsonEr length: _, field_count, fields: _, + .. }) => { + jb.set_bool("row_description_malformed", res.is_malformed())?; jb.set_uint("field_count", *field_count)?; } PgsqlBEMessage::ConsolidatedDataRow(ConsolidatedDataRowPacket { identifier: _, row_cnt, data_size, + .. }) => { jb.set_uint("data_rows", *row_cnt)?; jb.set_uint("data_size", *data_size)?; diff --git a/rust/src/pgsql/parser.rs b/rust/src/pgsql/parser.rs index 33554b219d..f3be60a352 100644 --- a/rust/src/pgsql/parser.rs +++ b/rust/src/pgsql/parser.rs @@ -23,15 +23,16 @@ use crate::common::nom7::take_until_and_consume; use nom7::branch::alt; use nom7::bytes::streaming::{tag, take, take_until, take_until1}; use nom7::character::streaming::{alphanumeric1, char}; -use nom7::combinator::{all_consuming, cond, eof, map_parser, opt, peek, verify}; +use nom7::combinator::{all_consuming, complete, cond, eof, map_parser, opt, peek, verify}; use nom7::error::{make_error, ErrorKind, ParseError}; -use nom7::multi::{many1, many_m_n, many_till}; +use nom7::multi::{count, many1, many_till}; use nom7::number::streaming::{be_i16, be_i32}; use nom7::number::streaming::{be_u16, be_u32, be_u8}; use nom7::sequence::terminated; use nom7::{Err, IResult, Parser}; const PGSQL_LENGTH_FIELD: u32 = 4; +const PGSQL_ROW_HEADER_LENGTH: u32 = 6; const PGSQL_DUMMY_PROTO_MAJOR: u16 = 1234; // 0x04d2 const PGSQL_DUMMY_PROTO_CANCEL_REQUEST: u16 = 5678; // 0x162e @@ -249,6 +250,8 @@ pub(crate) struct BackendKeyDataMessage { #[derive(Debug, PartialEq, Eq)] pub(crate) struct ConsolidatedDataRowPacket { + // also built in pgsql.rs, as a per-transaction summary + pub is_malformed: bool, pub identifier: u8, pub row_cnt: u64, // row or msg cnt pub data_size: u64, @@ -361,6 +364,8 @@ impl PgsqlBEMessage { PgsqlBEMessage::CopyInResponse(m) | PgsqlBEMessage::CopyOutResponse(m) => { m.is_malformed } + PgsqlBEMessage::RowDescription(r) => r.is_malformed, + PgsqlBEMessage::ConsolidatedDataRow(d) => d.is_malformed, _ => false, } } @@ -466,6 +471,7 @@ pub(crate) struct RowField { #[derive(Debug, PartialEq, Eq)] pub(crate) struct RowDescriptionMessage { + is_malformed: bool, pub identifier: u8, pub length: u32, pub field_count: u16, @@ -1009,16 +1015,30 @@ fn parse_row_field(i: &[u8]) -> IResult<&[u8], RowField, PgsqlParseError<&[u8]>> } fn parse_row_description(i: &[u8]) -> IResult<&[u8], PgsqlBEMessage, PgsqlParseError<&[u8]>> { - let (i, identifier) = verify(be_u8, |&x| x == b'T')(i)?; - let (i, length) = parse_gte_length(i, 7)?; - let (i, field_count) = be_u16(i)?; - let (i, fields) = map_parser( - take(length - 6), - many_m_n(0, field_count.into(), parse_row_field), - )(i)?; + let (i, identifier) = verify(be_u8, |&x| x == b'T').parse(i)?; + let (i, length) = parse_gte_length(i, PGSQL_ROW_HEADER_LENGTH + 1)?; + let (i, field_count) = be_u16.parse(i)?; + let (i, body) = take(length - PGSQL_ROW_HEADER_LENGTH).parse(i)?; + let fields = + match all_consuming(count(complete(parse_row_field), field_count.into())).parse(body) { + Ok((_, fields)) => fields, + Err(_) => { + return Ok(( + i, + PgsqlBEMessage::RowDescription(RowDescriptionMessage { + is_malformed: true, + identifier, + length, + field_count, + fields: Vec::new(), + }), + )); + } + }; Ok(( i, PgsqlBEMessage::RowDescription(RowDescriptionMessage { + is_malformed: false, identifier, length, field_count, @@ -1127,7 +1147,9 @@ fn parse_consolidated_copy_data_out(i: &[u8]) -> IResult<&[u8], PgsqlBEMessage, let (i, _data) = take(length - PGSQL_LENGTH_FIELD)(i)?; SCLogDebug!("data_size is {:?}", _data); Ok(( - i, PgsqlBEMessage::ConsolidatedCopyDataOut(ConsolidatedDataRowPacket { + i, + PgsqlBEMessage::ConsolidatedCopyDataOut(ConsolidatedDataRowPacket { + is_malformed: false, identifier, row_cnt: 1, data_size: (length - PGSQL_LENGTH_FIELD) as u64 }) @@ -1140,7 +1162,9 @@ fn parse_consolidated_copy_data_in(i: &[u8]) -> IResult<&[u8], PgsqlFEMessage, P let (i, _data) = take(length - PGSQL_LENGTH_FIELD)(i)?; SCLogDebug!("data size is {:?}", _data); Ok(( - i, PgsqlFEMessage::ConsolidatedCopyDataIn(ConsolidatedDataRowPacket { + i, + PgsqlFEMessage::ConsolidatedCopyDataIn(ConsolidatedDataRowPacket { + is_malformed: false, identifier, row_cnt: 1, data_size: (length - PGSQL_LENGTH_FIELD) as u64 }) @@ -1185,20 +1209,31 @@ fn parse_copy_fail(i: &[u8]) -> IResult<&[u8], PgsqlFEMessage, PgsqlParseError<& // Currently, we don't store the actual DataRow messages, as those could easily become a burden, memory-wise // We use ConsolidatedDataRow to store info we still want to log: message size. // Later on, we calculate the number of lines the command actually returned by counting ConsolidatedDataRow messages -fn parse_consolidated_data_row( - i: &[u8], -) -> IResult<&[u8], PgsqlBEMessage, PgsqlParseError<&[u8]>> { - let (i, identifier) = verify(be_u8, |&x| x == b'D')(i)?; - let (i, length) = parse_gte_length(i, 7)?; - let (i, field_count) = be_u16(i)?; - // 6 here is for skipping length + field_count - let (i, rows) = map_parser( - take(length - 6), - many_m_n(0, field_count.into(), parse_data_row_value), - )(i)?; +fn parse_consolidated_data_row(i: &[u8]) -> IResult<&[u8], PgsqlBEMessage, PgsqlParseError<&[u8]>> { + let (i, identifier) = verify(be_u8, |&x| x == b'D').parse(i)?; + let (i, length) = parse_gte_length(i, PGSQL_ROW_HEADER_LENGTH + 1)?; + let (i, field_count) = be_u16.parse(i)?; + let (i, body) = take(length - PGSQL_ROW_HEADER_LENGTH).parse(i)?; + let rows = match all_consuming(count(complete(parse_data_row_value), field_count.into())) + .parse(body) + { + Ok((_, rows)) => rows, + Err(_) => { + return Ok(( + i, + PgsqlBEMessage::ConsolidatedDataRow(ConsolidatedDataRowPacket { + is_malformed: true, + identifier, + row_cnt: 1, + data_size: 0, + }), + )); + } + }; Ok(( i, PgsqlBEMessage::ConsolidatedDataRow(ConsolidatedDataRowPacket { + is_malformed: false, identifier, row_cnt: 1, data_size: add_up_data_size(rows), @@ -2471,12 +2506,12 @@ mod tests { // T .. // source @ . ....... version @ . ....... sid @ . . ..... let buffer: &[u8] = &[ - 0x54, 0x00, 0x00, 0x00, 0x50, 0x00, 0x03, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x00, + 0x54, 0x00, 0x00, 0x00, 0x4f, 0x00, 0x03, 0x73, 0x6f, 0x75, 0x72, 0x63, 0x65, 0x00, 0x00, 0x00, 0x40, 0x09, 0x00, 0x01, 0x00, 0x00, 0x00, 0x19, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x76, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x00, 0x00, 0x00, 0x40, 0x09, 0x00, 0x02, 0x00, 0x00, 0x00, 0x19, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x73, 0x69, 0x64, 0x00, 0x00, 0x00, 0x40, 0x09, 0x00, 0x03, 0x00, 0x00, - 0x00, 0x14, 0x00, 0x08, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, + 0x00, 0x14, 0x00, 0x08, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00, ]; let field1 = RowField { @@ -2512,8 +2547,9 @@ mod tests { let fields_vec = vec![field1, field2, field3]; let ok_res = PgsqlBEMessage::RowDescription(RowDescriptionMessage { + is_malformed: false, identifier: b'T', - length: 80, + length: 79, field_count: 3, fields: fields_vec, }); @@ -2667,8 +2703,17 @@ mod tests { 0x07, 0x32, 0x30, 0x32, 0x31, 0x37, 0x30, 0x31, ]; - let result = parse_consolidated_data_row(buffer); - assert!(result.is_ok()); + let ok_res = PgsqlBEMessage::ConsolidatedDataRow(ConsolidatedDataRowPacket { + is_malformed: false, + identifier: b'D', + row_cnt: 1, + data_size: 17, + }); + + assert_eq!( + parse_consolidated_data_row(buffer), + Ok((&buffer[36..], ok_res)) + ); } #[test] diff --git a/rust/src/pgsql/pgsql.rs b/rust/src/pgsql/pgsql.rs index 97b35dd2cb..488c5524de 100644 --- a/rust/src/pgsql/pgsql.rs +++ b/rust/src/pgsql/pgsql.rs @@ -431,6 +431,7 @@ impl PgsqlState { { let consolidated_copy_data = PgsqlFEMessage::ConsolidatedCopyDataIn(ConsolidatedDataRowPacket { + is_malformed: false, identifier: b'd', row_cnt: tx.get_row_cnt(), data_size: tx.data_size, // total byte count of all copy_data messages combined @@ -640,6 +641,9 @@ impl PgsqlState { tx.tx_res_state = PgsqlTxProgress::Received; } if matches!(response, PgsqlBEMessage::ConsolidatedDataRow(_)) { + if response.is_malformed() { + tx.tx_data.set_event(PgsqlEvent::MalformedResponse as u8); + } tx.incr_row_cnt(); } else if matches!(response, PgsqlBEMessage::CommandComplete(_)) && tx.get_row_cnt() > 0 @@ -647,6 +651,7 @@ impl PgsqlState { // let's summarize the info from the data_rows in one response let consolidated_data_row = PgsqlBEMessage::ConsolidatedDataRow(ConsolidatedDataRowPacket { + is_malformed: false, identifier: b'D', row_cnt: tx.get_row_cnt(), data_size: tx.data_size, // total byte count of all data_row messages combined @@ -664,6 +669,7 @@ impl PgsqlState { // let's summarize the info from the data_rows in one response let consolidated_copy_data = PgsqlBEMessage::ConsolidatedCopyDataOut( ConsolidatedDataRowPacket { + is_malformed: false, identifier: b'd', row_cnt: tx.get_row_cnt(), data_size: tx.data_size, // total byte count of all data_row messages combined