PostgreSQL: add initial support (#4241) - v4 #6463
Conversation
- add nom parsers for decoding most messages from StartupPhase and SimpleQuery subprotocols - add unittests - tests/fuzz: add pgsql to confyaml
When an SSL Accepted is parsed, suri calls STARTTLS and sets PgsqlStateProgress to Finished. Known bug: Suri won't identify flow as PGSQL, as of now, if there's just an SSL Handshake and then tls traffic (as seen in this pcap: https://git.lekensteyn.nl/peter/wireshark-notes/commit/tls/ pgsql-ssl.pcapng?id=836b6f746df24aa04fa29b71806d8d0e496c2a68
Also: delete uneeded unittests API call in C
The CommandComplete messages c-string terminator didn't add value to the log messages, this seems to fix that.
When pgsql doesn't find a transaction, we'll return AppLayerResult::ok(). This was missing in the parse_request function.
Codecov Report
@@ Coverage Diff @@
## master #6463 +/- ##
==========================================
- Coverage 77.01% 76.97% -0.04%
==========================================
Files 613 615 +2
Lines 186715 186794 +79
==========================================
- Hits 143795 143788 -7
- Misses 42920 43006 +86
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 4510 |
| #define __APP_LAYER_PGSQL_H__ | ||
|
|
||
| void RegisterPgsqlParsers(void); | ||
| void PgsqlParserRegisterTests(void); |
There was a problem hiding this comment.
unused PgsqlParserRegisterTests
| @@ -0,0 +1,45 @@ | |||
| /* Copyright (C) 2021 Open Information Security Foundation | |||
There was a problem hiding this comment.
I think you do not need these files cf rs_dhcp_register_parser
| @@ -77,6 +77,7 @@ include = [ | |||
| "SIPState", | |||
| "ModbusState", | |||
| "CMark", | |||
| "PgsqlTransaction", | |||
There was a problem hiding this comment.
For the C side to see PgsqlTransaction, as it was not directly mentioned in the extern C function declarations. I'll check if it can be removed now, with the other removals that you pointed out...
There was a problem hiding this comment.
Thanks. Why do we need C to see PgsqlTransaction ?
There was a problem hiding this comment.
I was getting compilation errors without that, I believe. I think that can be because Suri core is still in C? To be honest, I don't fully understand all the connections and how things work in the background, yet. But will answer once things are clearer in my head.
There was a problem hiding this comment.
Tried removing it now: output-json-pgsql.c complains, because I'm using PgsqlTransaction there. So, that's what made me add it :P
There was a problem hiding this comment.
If you look at the DNS parser, it doesn't bother with this assignment from void * to DNSTransaction before passing to Rust, it just passes through the void pointer which works just as well. So thats one way to remove this item from the cbindgen config. Unfortunately in this context it doesn't provide any extra type safety.
There was a problem hiding this comment.
I see, thanks for the lead. Is that the preferred way to address such cases?
There was a problem hiding this comment.
I see, thanks for the lead. Is that the preferred way to address such cases?
Yeah, I think so. The Rust function takes a void ptr as well and does its own cast, so doing it C provides no value.
There was a problem hiding this comment.
Noted! Thanks again :)
| fn log_pgsql(tx: &PgsqlTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { | ||
| js.set_uint("tx_id", tx.tx_id)?; | ||
| if !tx.requests.is_empty() { | ||
| if tx.requests.len() > 1 { |
There was a problem hiding this comment.
I think this deserves some doc : it looks like we can have multiple responses for one request ? all inside one transaction...
There was a problem hiding this comment.
Yes, that is so. In many cases, a transaction will be comprised of one request and a few replies, in a specific order. Thanks, I'll add that as part of the docs requirement.
| Ok(js) | ||
| } | ||
|
|
||
| // fn log_row_description(columns: &Vec<RowField>) -> Result<JsonBuilder, JsonError> |
There was a problem hiding this comment.
That is commented out for now, but could possibly be used when we have extended logs. Do I remove them, for now, then?
There was a problem hiding this comment.
Or add a config option to have extended logging ?
I would delete it for now...
There was a problem hiding this comment.
I think the plan is for the extended logging to come in a future version... Ok, I'll delete it, thanks :)
| let mut jb = JsonBuilder::new_object(); | ||
| for field in error_fields { | ||
| match field.field_type { | ||
| PgsqlErrorNoticeFieldType::SeverityLocalizable => { |
There was a problem hiding this comment.
Could this rather be jb.set_string_from_bytes(field.field_type.to_str(), &field.field_value)?; for all field types ?
cc @jasonish
There was a problem hiding this comment.
Yes, I think so. Add a to_str() -> &str on PgsqlErrorNoticeFieldType could shorten this up to just the fields we might want to skip.
|
Looks like there could be some doc cf
You can also add some app-layer events in cf I guess it can wait on detection, but then Looks really awesome by the way |
| CommandCompletedReceived, | ||
| ErrorMessageReceived, // TODO do we really need this one? | ||
| ConnectionTerminated, | ||
| UnknownState, // TODO do we really need this one? |
There was a problem hiding this comment.
Looks like we should have error and stop parsing...
There was a problem hiding this comment.
for the UnknownState, right? Thanks :)
| /// | ||
| /// PGSQL messages don't have a header per se, so we parse the slice for an ok() | ||
| fn probe_ts(input: &[u8]) -> bool { | ||
| SCLogDebug!("We are in probe_ts"); |
There was a problem hiding this comment.
Can we get an incomplete result ?
There was a problem hiding this comment.
if we have a gap in the stream, that can result in a temporary incomplete result, is that so? If it is, then I think it could happen. But I must probably investigate this further.
There was a problem hiding this comment.
if we have a gap in the stream, that can result in a temporary incomplete result, is that so?
Not exactly.
Rather if the client sends the TCP data one byte at a time in each packet...
There was a problem hiding this comment.
Hm... I haven't found mentions of that, so far, in the documentation. Could you recommend a good way for one to verify this?
There was a problem hiding this comment.
cf rs_http2_probing_parser_tc returning either ALPROTO_FAILED, ALPROTO_UNKNOWN (for incomplete) or ALPROTO_HTTP2
Thanks, and thanks again for all the feedback, I'm slowly digesting and incorporating them. :) |
| pub extern "C" fn rs_pgsql_logger_log(tx: *mut std::os::raw::c_void, js: &mut JsonBuilder) -> bool { | ||
| let tx_safe: &mut PgsqlTransaction; | ||
| unsafe { | ||
| tx_safe = cast_pointer!(tx, PgsqlTransaction); | ||
| } |
There was a problem hiding this comment.
This whole function should be made unsafe now. Sometime after you started this we moved all functions that do a raw pointer cast to unsafe.
catenacyber
added
needs rebase
|
Replaced by: #6630 |
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/4241
Previous PR: #6437
Describe changes:
CommandCompleteConsiderations: Pcap-cnt seems to be always 4 more than in Wireshark, have to investigate it. Timestamp doesn't always show the same thing as in Wireshark, too. Not sure yet what causes that, but wondering if it could be related to when is Suri considering that the transaction is completed...
suricata-verify-pr: 556
OISF/suricata-verify#556