pgsql: improve probing function - v1 #9881
Conversation
We had unkonwn message type for the backend, but not the frontend messages. It's important to better identify those to improve pgsql probing functions. Related to Bug OISF#6080
Some non-pgsql traffic seen by Suricata is mistankenly identified as pgsql, as the probing function is too generic. Now, if the parser sees an unknown message type, even if it looks like pgsql, it will return unknown proto. Bug OISF#6080
Remove code for logging fields that should never be logged in the first place.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9881 +/- ##
==========================================
- Coverage 82.45% 82.44% -0.01%
==========================================
Files 973 973
Lines 273063 273082 +19
==========================================
- Hits 225155 225145 -10
- Misses 47908 47937 +29
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 16711 |
| length: _, | ||
| payload: _, | ||
| }) => { | ||
| // We don't expect to log this, as the probing function will fail the proto if we see |
There was a problem hiding this comment.
We should log the identifier, should we not ?
There was a problem hiding this comment.
But if we return ALPROTO_FAILED, how would we have these messages in a transaction, available to be logged here?
There was a problem hiding this comment.
If it is not the first PDU.
Probing is only for the first PDU (or the first ones)
Logging is for every PDU
There was a problem hiding this comment.
AH! Okie, will do.
There was a problem hiding this comment.
Discussing this with Victor, we have decided to keep this as a separate step, so we can consider a few things before implementing this. Ticket: https://redmine.openinfosecfoundation.org/issues/6576
There was a problem hiding this comment.
Makes sense for me as well to separate probing and logging of unknown pgsql identifiers in different PRs/redmine tickets.
There was a problem hiding this comment.
Thanks for the feedback. Do you think more info is needed in the ticket created?
There was a problem hiding this comment.
Just linked this GitHub discussion.
I think it is good for now. And we can focus on the probing part aka https://redmine.openinfosecfoundation.org/issues/6080
| }) => { | ||
| // jb.set_string_from_bytes("identifier", identifier.to_vec())?; |
There was a problem hiding this comment.
I would just log the identifier
| @@ -558,8 +563,20 @@ pub unsafe extern "C" fn rs_pgsql_probing_parser_ts( | |||
| if input_len >= 1 && !input.is_null() { | |||
|
|
|||
| let slice: &[u8] = build_slice!(input, input_len as usize); | |||
| if probe_ts(slice) { | |||
There was a problem hiding this comment.
Is probe_ts still used ?
There was a problem hiding this comment.
Yes, it is used by pgsql::parse_request to decide whether there's a gap or not. (cf https://github.com/OISF/suricata/blob/master/rust/src/pgsql/pgsql.rs#L316)
| match parser::parse_request(slice) { | ||
| Ok((_, request)) => { | ||
| if let PgsqlFEMessage::UnknownMessageType(_regular_packet) = request { | ||
| return ALPROTO_UNKNOWN; |
There was a problem hiding this comment.
I would return ALPROTO_FAILED :
In probing functions ALPROTO_UNKNOWN means we need more data. In this case, we would loop forever returning ALPROTO_UNKNOWN
There was a problem hiding this comment.
Ah, sorry, I didn't know that. Will change.
| Ok((_, _response)) => { | ||
| Ok((_, response)) => { | ||
| if let PgsqlBEMessage::UnknownMessageType(_regular_packet) = response { | ||
| return ALPROTO_UNKNOWN; |
There was a problem hiding this comment.
ALPROTO_FAILED same as above
|
Replaced by: #9901 |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/6080
Describe changes:
ALPROTO_UNKNOWNwhen an unknown message type is seen/parsed - thought that this might make more sense thanALPROTO_FAILED@catenacyber, but will change if you think otherwiseDidn't add SV tests as this was triggered by a TLPR pcap and I wasn't able to easily reproduce a pcap, but created SV tests locally and it looks fixed.