Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pgsql: add events to handle parser errors - v1 #9404

Closed
wants to merge 1 commit into from
Closed

pgsql: add events to handle parser errors - v1 #9404

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 49 additions & 4 deletions rust/src/pgsql/pgsql.rs
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ use super::parser::{self, ConsolidatedDataRowPacket, PgsqlBEMessage, PgsqlFEMess
use crate::applayer::*;
use crate::conf::*;
use crate::core::{AppProto, Flow, ALPROTO_FAILED, ALPROTO_UNKNOWN, IPPROTO_TCP};
use nom7::error::{Error, ErrorKind};
use nom7::{Err, IResult};
use std;
use std::collections::VecDeque;
Expand All @@ -34,6 +35,13 @@ static mut ALPROTO_PGSQL: AppProto = ALPROTO_UNKNOWN;

static mut PGSQL_MAX_TX: usize = 1024;

#[derive(Debug, PartialEq, Eq, AppLayerEvent)]
pub enum PgsqlEvent {
InvalidLength, // Can't identify the length field
TruncatedData, // Failed to parse due to missing data
MalformedData, // Enough data, but unexpected message format/fields
}

#[repr(u8)]
#[derive(Copy, Clone, PartialOrd, PartialEq, Eq, Debug)]
pub enum PgsqlTransactionState {
Expand Down Expand Up @@ -134,6 +142,7 @@ pub struct PgsqlState {
backend_pid: u32,
state_progress: PgsqlStateProgress,
tx_index_completed: usize,
events: u16,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field seems unused

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this makes sense or not, but... The implementation I mostly see, the events are set in the transactions. But sometimes, there may not be a transaction, yet. Would it make any sense to have this counter here, for such cases, at least?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense at some point to have this counter, but the code now does not make use of it

So, I would remove it until it is really needed

}

impl State<PgsqlTransaction> for PgsqlState {
Expand All @@ -151,7 +160,7 @@ impl Default for PgsqlState {
Self::new()
}
}

impl PgsqlState {
pub fn new() -> Self {
Self {
Expand All @@ -164,6 +173,17 @@ impl PgsqlState {
backend_pid: 0,
state_progress: PgsqlStateProgress::IdleState,
tx_index_completed: 0,
events: 0,
}
}

/// Set an event on the most recent transaction
fn set_event(&mut self, event: PgsqlEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write this in impl PgsqlTransaction ?

As it belongs to a transaction, not the state...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Connected to my wondering of what to do in case the event is the result of a parsing error, and to the possibility of not existing a transaction, I see that for there's a set_event_notx function (1ba62993d5fc6ac6fa8). Wondering if this could be the answer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may create a transaction for the sole purpose of having the event...
Not sure it is the best way...

if let Some(tx) = self.transactions.back_mut() {
tx.tx_data.set_event(event as u8);
self.events += 1;
} else {
SCLogDebug!("PGSQL: trying to set event {} on non-existing transaction", event as u8);
Comment on lines +180 to +186
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that most protos only add events to existing transactions. What if there's something with the very first request - which would lead to the creation of the transaction, that happens in case of successful parsing for pgsql, would it make sense to save those somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may create a transaction for the sole purpose of having the event...
Not sure it is the best way...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about the only way right now. Frames will help at some point when events are frames get logged:

https://redmine.openinfosecfoundation.org/issues/5826

}
}

Expand Down Expand Up @@ -356,9 +376,22 @@ impl PgsqlState {
_needed,
needed_estimation
);
SCLogDebug!("PgsqlEvent::TruncatedData");
self.set_event(PgsqlEvent::TruncatedData);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should set an event when a PGSQL payload is fragmented over multiple TCP packets...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oki 🙇🏽

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(As it is legit traffic)

return AppLayerResult::incomplete(consumed as u32, needed_estimation as u32);
}
Err(Err::Error(Error{code:ErrorKind::Verify, ..})) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not obvious to me why a verify error is invalidlength

In the future, I may add more pgsql parsing code with nom verify that is not about lengths

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true that >_<'
I'll re-write this part.

SCLogDebug!("PgsqlEvent::InvalidLength");
self.set_event(PgsqlEvent::InvalidLength);
AppLayerResult::err();
}
Err(Err::Error(Error{code:ErrorKind::Switch, ..})) => {
self.set_event(PgsqlEvent::MalformedData);
SCLogDebug!("PgsqlEvent::MalformedData");
return AppLayerResult::ok();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is possible to organize the code a bit differently :

  1. get the PDU (or error if there is an invalid length)
  2. parse the PDU (and if there is an error, still return AppLayerResult::ok() for next PDUs)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think that the two tickets should have their own commit)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that on the commit per ticket part!
wrt to the re-structuring, I'm trying to visualize how to do that with pgsql, as the protocol only has pseudo-headers, and some PDUs may come with pure data, for which information on how to parse them (including length) would be present in a previous message, only...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and some PDUs may come with pure data

Which ones ?
It looked to me that it is always one byte opcode, and 4 bytes length of PDU...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I didn't see this question before T__T
When the pgsql backend is answering to a query request, for instance, a single PGSQL message may span over several PDUs, and the intermediate PGSQL messages don't come with any header info.
Taking https://github.com/OISF/suricata-verify/blob/master/tests/pgsql/pgsql-5000-query-results/input.pcap as an example, in packet no. 32, the second to last PGSQL message has an informed length of 482 bytes, but the PGSQL PDU only has 132 bytes, so the rest of the message spills onto the next PGSQL PDU, and for that one we don't have info on how to read it.
The info is actually on the same second-to-last message, which indicates that the next data will be a Data column with 360 bytes, but this is only present in the previous message. The last message starts in the middle of a rule signature, which is part of the Data result, which actually started on the previous message.
I guess... this could become some transaction info that could be used for such cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: reminder that pgsql documentation indicates:
"Notice that although each message includes a byte count at the beginning, the message format is defined so that the message end can be found without reference to the byte count."
cf - https://www.postgresql.org/docs/current/protocol-message-formats.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still struggling a bit to figure out how to approach this bit for the PDU part. As I think of this, wondering if the introduction of the UnknownMessageType could be enough to handle this?
Introduced in 1ac5d97 and 0fc9ade

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is possible to organize the code a bit differently :

  1. get the PDU (or error if there is an invalid length)
  2. parse the PDU (and if there is an error, still return AppLayerResult::ok() for next PDUs)

Added this to a ticket, as it doesn't look trivial, still, for now https://redmine.openinfosecfoundation.org/issues/6654)

I think that part of this is somewhat addressed by the implementation of the UnknownMessageTypes - although not entirely, as we're not changing how the PDU is processed, just reducing the situations when the parser returns an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if the introduction of the UnknownMessageType could be enough to handle this?

Yes but not sure it is the best way to have duplicated code to get the length everywhere...

But I guess the functionality (return AppLayerResult::ok() so as to parse next PDUs) can be achieved even if the code is duplicated...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I tried to reduce that with the length function, but there's certainly a lot of room for improvement...

}
Err(_) => {
SCLogDebug!("Error while parsing PGSQL request");
return AppLayerResult::err();
}
}
Expand Down Expand Up @@ -505,10 +538,22 @@ impl PgsqlState {
needed_estimation,
&start
);
self.set_event(PgsqlEvent::TruncatedData);
SCLogDebug!("PgsqlEvent::TruncatedData");
return AppLayerResult::incomplete(consumed as u32, needed_estimation as u32);
}
Err(Err::Error(Error{code:ErrorKind::Verify, ..})) => {
SCLogDebug!("PgsqlEvent::InvalidLength");
self.set_event(PgsqlEvent::InvalidLength);
AppLayerResult::err();
}
Err(Err::Error(Error{code:ErrorKind::Switch, ..})) => {
self.set_event(PgsqlEvent::MalformedData);
SCLogDebug!("PgsqlEvent::MalformedData");
return AppLayerResult::ok();
Comment on lines +550 to +553
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must understand if I'm not seeing events with this one due to UnknownMessage type. But we only have that for BackEnd messages, so... must investigate. Personal note: tlpwhite jdbc pcap may help me here.

}
Err(_) => {
SCLogDebug!("Error while parsing PostgreSQL response");
SCLogDebug!("Error while parsing PGSQL response");
return AppLayerResult::err();
}
}
Expand Down Expand Up @@ -727,8 +772,8 @@ pub unsafe extern "C" fn rs_pgsql_register_parser() {
tx_comp_st_ts: PgsqlTransactionState::RequestReceived as i32,
tx_comp_st_tc: PgsqlTransactionState::ResponseDone as i32,
tx_get_progress: rs_pgsql_tx_get_alstate_progress,
get_eventinfo: None,
get_eventinfo_byid: None,
get_eventinfo: Some(PgsqlEvent::get_event_info),
get_eventinfo_byid: Some(PgsqlEvent::get_event_info_by_id),
localstorage_new: None,
localstorage_free: None,
get_tx_files: None,
Expand Down