pgsql: don't error out with PDU parsing errors - v2 #12609
Conversation
Some backend messages can be the shortest pgsql length possible, 4 bytes, but the parser expectd all messages to be longer than that. Related to Bug OISF#5524
The initial parsing for message type checking was more complex than needed be. Related to Bug OISF#5524
Building on top of work done by Jason Ish. Related to Bug OISF#5524
Even if unknown, if the message is properly parsed, allow the parser to proceed. Related to Bug OISF#5524
This allows the app-proto to continue onto parsing next PDUs, if possible. Bug OISF#5524
Events for: - parsing error when parsing pgsql packet length - parsing error for pgsql requests (post length parsing) - parsing error for pgsql responses (post length parsing) - too many transactions Include `pgsql-events.rules` file, and PGSQL events SID range definition Task OISF#5566
This may happen in some situations if the app-layer parser only sees unknown messages and sets an event: there will be an empty transaction, but nothing to log. Related to Task OISF#5566
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12609 +/- ##
==========================================
+ Coverage 80.74% 80.75% +0.01%
==========================================
Files 931 931
Lines 259144 259209 +65
==========================================
+ Hits 209242 209332 +90
+ Misses 49902 49877 -25
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 24761 |
| let (i, identifier) = verify(be_u8, |&x| x == b'T')(i)?; | ||
| let (i, length) = verify(be_u32, |&x| x > 6)(i)?; | ||
| //let (i, length) = parse_gt_length(i, 6)?; |
There was a problem hiding this comment.
nit: remove commented out code
There was a problem hiding this comment.
Ugh, that's what I get when having so much back and forth fixing commit history x___x sorry about that and the rust issue x___x
| // TODO Log anomaly event instead? | ||
| js.set_bool("request", false)?; | ||
| js.set_bool("response", false)?; | ||
| // TODO Log anomaly event? |
There was a problem hiding this comment.
in what case can this happen?
There was a problem hiding this comment.
Currently, we can have empty transactions for instance if we get parsing error(s) for the msg length from both response and request (or if there's only one pgsql message in the transaction). This happens with the pgsql-events test that's present in the SV PR that goes with this one:
{
"timestamp": "2022-01-04T03:54:04.788066+0000",
"flow_id": 1132919115245510,
"event_type": "pgsql",
"src_ip": "167.248.133.59",
"src_port": 47478,
"dest_ip": "10.44.3.101",
"dest_port": 5432,
"proto": "TCP",
"pkt_src": "stream (flow timeout)",
"pgsql": {
"tx_id": 1
}
}On the one hand, if we know we can "cause" this, I'm not sure if it makes sense to consider this an anomaly. On the other, I guess that ideally the nodes wouldn't send messages with unparsable length, which could indicate an anomaly...
There was a problem hiding this comment.
Hmm this record doesn't look very useful. There is no indication of an error or that an event has been set. How are other parsers handling this?
There was a problem hiding this comment.
SSH will return false if the transaction is empty, I could do that for PGSQL. The app-layer event is still created and the stats will still register that PGSQL was seen.
|
CI fixed with #12614 |
Or not. One issue remaining, still. |
Previous PR: #12543
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5524
https://redmine.openinfosecfoundation.org/issues/5566
It seemed to me that it made sense to have the events as part of this work
Describe changes:
pgsql-events.rulesfileProvide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#2299