-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
pgsql: add events to handle parser errors - v1 #9404
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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 { | ||
|
@@ -134,6 +142,7 @@ pub struct PgsqlState { | |
backend_pid: u32, | ||
state_progress: PgsqlStateProgress, | ||
tx_index_completed: usize, | ||
events: u16, | ||
} | ||
|
||
impl State<PgsqlTransaction> for PgsqlState { | ||
|
@@ -151,7 +160,7 @@ impl Default for PgsqlState { | |
Self::new() | ||
} | ||
} | ||
|
||
impl PgsqlState { | ||
pub fn new() -> Self { | ||
Self { | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you write this in As it belongs to a transaction, not the state... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: |
||
} | ||
} | ||
|
||
|
@@ -356,9 +376,22 @@ impl PgsqlState { | |
_needed, | ||
needed_estimation | ||
); | ||
SCLogDebug!("PgsqlEvent::TruncatedData"); | ||
self.set_event(PgsqlEvent::TruncatedData); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oki 🙇🏽 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, ..})) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not obvious to me why a In the future, I may add more pgsql parsing code with nom There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. true that >_<' |
||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I think that the two tickets should have their own commit) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Roger that on the commit per ticket part! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Which ones ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, I didn't see this question before T__T There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to self: reminder that pgsql documentation indicates: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
} | ||
} | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
Err(_) => { | ||
SCLogDebug!("Error while parsing PostgreSQL response"); | ||
SCLogDebug!("Error while parsing PGSQL response"); | ||
return AppLayerResult::err(); | ||
} | ||
} | ||
|
@@ -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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field seems unused
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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