From 0fc9ade7a9f672560a276bb40d97cf07ee89dbac Mon Sep 17 00:00:00 2001 From: Juliana Fajardini Date: Fri, 4 Feb 2022 10:58:27 +0000 Subject: [PATCH] pgsql: fix uint overflow and optimizations Fuzzers found a possible integer overflow bug when parsing response messages. To fix that, removed the case where we incremented the parsed field length and created a new message type for situations where Suri parsers an Unknown message. This is good because there may happen that an unknown message to Suri is valid, and in this case, we would still be able to log it. Philippe Antoine found the bug while fuzzing with rust debug assertions. Bug #5016 --- rust/src/pgsql/logger.rs | 9 ++++++ rust/src/pgsql/parser.rs | 62 ++++++++++++++++++++++------------------ rust/src/pgsql/pgsql.rs | 2 +- 3 files changed, 44 insertions(+), 29 deletions(-) diff --git a/rust/src/pgsql/logger.rs b/rust/src/pgsql/logger.rs index f0b0d57947..87926d81f5 100644 --- a/rust/src/pgsql/logger.rs +++ b/rust/src/pgsql/logger.rs @@ -187,6 +187,15 @@ fn log_response(res: &PgsqlBEMessage, jb: &mut JsonBuilder) -> Result<(), JsonEr }) => { jb.set_string_from_bytes(res.to_str(), payload)?; } + PgsqlBEMessage::UnknownMessageType(RegularPacket { + identifier: _, + length, + payload, + }) => { + // jb.set_string_from_bytes("identifier", identifier.to_vec())?; + jb.set_uint("length", (*length).into())?; + jb.set_string_from_bytes("payload", payload)?; + } PgsqlBEMessage::AuthenticationOk(_) | PgsqlBEMessage::AuthenticationKerb5(_) | PgsqlBEMessage::AuthenticationCleartextPassword(_) diff --git a/rust/src/pgsql/parser.rs b/rust/src/pgsql/parser.rs index 5e041a7d68..8f2827b16c 100644 --- a/rust/src/pgsql/parser.rs +++ b/rust/src/pgsql/parser.rs @@ -270,6 +270,7 @@ pub enum PgsqlBEMessage { RowDescription(RowDescriptionMessage), ConsolidatedDataRow(ConsolidatedDataRowPacket), NotificationResponse(NotificationResponse), + UnknownMessageType(RegularPacket), } impl PgsqlBEMessage { @@ -301,6 +302,7 @@ impl PgsqlBEMessage { } PgsqlBEMessage::ConsolidatedDataRow(_) => "data_row", PgsqlBEMessage::NotificationResponse(_) => "notification_response", + PgsqlBEMessage::UnknownMessageType(_) => "unknown_message_type" } } @@ -773,7 +775,7 @@ fn pgsql_parse_authentication_message<'a>(i: &'a [u8]) -> IResult<&'a [u8], Pgsq }))) }, 12 => { - let (i, signature) = rest(i)?; + let (i, signature) = take(length - 8)(i)?; Ok((i, PgsqlBEMessage::AuthenticationSASLFinal( AuthenticationMessage { identifier, @@ -1065,24 +1067,30 @@ fn parse_notification_response(i: &[u8]) -> IResult<&[u8], PgsqlBEMessage> { pub fn pgsql_parse_response(i: &[u8]) -> IResult<&[u8], PgsqlBEMessage> { let (i, pseudo_header) = peek(tuple((be_u8, be_u32)))(i)?; - let (i, message) = map_parser( - take(pseudo_header.1 + 1), - |b| { + let (i, message) = match pseudo_header.0 { - b'E' => pgsql_parse_error_response(b), - b'K' => parse_backend_key_data_message(b), - b'N' => pgsql_parse_notice_response(b), - b'R' => pgsql_parse_authentication_message(b), - b'S' => parse_parameter_status_message(b), - b'C' => parse_command_complete(b), - b'Z' => parse_ready_for_query(b), - b'T' => parse_row_description(b), - b'A' => parse_notification_response(b), - b'D' => parse_consolidated_data_row(b), - // _ => {} // TODO add an unknown message type here? - _ => return Err(Err::Error(make_error(i, ErrorKind::Switch))), - } - })(i)?; + b'E' => pgsql_parse_error_response(i)?, + b'K' => parse_backend_key_data_message(i)?, + b'N' => pgsql_parse_notice_response(i)?, + b'R' => pgsql_parse_authentication_message(i)?, + b'S' => parse_parameter_status_message(i)?, + b'C' => parse_command_complete(i)?, + b'Z' => parse_ready_for_query(i)?, + b'T' => parse_row_description(i)?, + b'A' => parse_notification_response(i)?, + b'D' => parse_consolidated_data_row(i)?, + // _ => return Err(Err::Error(make_error(i, ErrorKind::Switch))), + _ => { + let (i, payload) = rest(i)?; + let unknown = PgsqlBEMessage::UnknownMessageType (RegularPacket{ + identifier: pseudo_header.0, + length: pseudo_header.1, + payload: payload.to_vec(), + }); + (i, unknown) + } + + }; Ok((i, message)) } @@ -1872,16 +1880,14 @@ mod tests { 0x2b, 0x4a, 0x36, 0x79, 0x78, 0x72, 0x66, 0x77, 0x2f, 0x7a, 0x7a, 0x70, 0x38, 0x59, 0x54, 0x39, 0x65, 0x78, 0x56, 0x37, 0x73, 0x38, 0x3d, ]; - let result_err = pgsql_parse_response(bad_buf); - match result_err { - Err(Err::Error(err)) => { - assert_eq!(err.code, ErrorKind::Switch); - } - Err(Err::Incomplete(_)) => { - panic!("Unexpected Incomplete, should be ErrorKind::Switch"); - } - _ => panic!("Unexpected behavior, expected Error"), - } + let (remainder, result) = pgsql_parse_response(bad_buf).expect("parsing sasl final response failed"); + let res = PgsqlBEMessage::UnknownMessageType(RegularPacket { + identifier: b'`', + length: 54, + payload: bad_buf.to_vec(), + }); + assert_eq!(result, res); + assert!(remainder.is_empty()); } #[test] diff --git a/rust/src/pgsql/pgsql.rs b/rust/src/pgsql/pgsql.rs index 2b4886675b..84abec83da 100644 --- a/rust/src/pgsql/pgsql.rs +++ b/rust/src/pgsql/pgsql.rs @@ -551,7 +551,7 @@ pub unsafe extern "C" fn rs_pgsql_probing_parser_tc( Err(Err::Incomplete(_)) => { return ALPROTO_UNKNOWN; } - Err(_) => { + Err(_e) => { return ALPROTO_FAILED; } }