pgsql: don't error out with PDU parsing errors - v3 #12614
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 #12614 +/- ##
==========================================
+ Coverage 80.74% 80.75% +0.01%
==========================================
Files 931 931
Lines 259144 259208 +64
==========================================
+ Hits 209242 209328 +86
+ Misses 49902 49880 -22
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 24779 |
victorjulien
approved these changes
Feb 19, 2025
Merged
|
Merged in #12625, thanks! |
catenacyber
reviewed
Feb 19, 2025
| if let PgsqlFEMessage::UnknownMessageType(_) = request { | ||
| return ALPROTO_FAILED; | ||
| } | ||
| Ok((_, _)) => { |
There was a problem hiding this comment.
This does not look right to me : the code does not match the commit message : this is probing, not parsing
I do not think we want to detect pgsql when we see unknown messages
catenacyber
reviewed
Feb 19, 2025
| // 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.
return Error so that caller returns false ?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous PR: #12609
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5524
https://redmine.openinfosecfoundation.org/issues/5566
Describe changes:
#12609, fixing:
Provide values to any of the below to override the defaults.
SV_BRANCH=OISF/suricata-verify#2299