detect: log app-layer metadata in alert with single tx #12195
Conversation
Ticket: 7199 Uses a config parameter detect.force-applayer-findtx to enable this behavior (off by default) This feature is requested for use cases with signatures not using app-layer keywords but still targetting application layer transactions, such as pass/drop rule combination, or lua usage. This overrides the previous behavior of checking if the signature has a content match, by checking if there is only one live transaction, in addition to the config parameter being set.
catenacyber
requested review from
jufajardini,
victorjulien and
a team
as code owners
| @@ -70,6 +70,21 @@ Alerts are event records for rule matches. They can be amended with | |||
| metadata, such as the application layer record (HTTP, DNS, etc) an | |||
| alert was generated for, and elements of the rule. | |||
|
|
|||
| The alert is amended with application layer metadata for signatures | |||
| using application layer keywords. It is also the case for protocols | |||
| over UDP as each single packet is expected to contain a PDU. | |||
There was a problem hiding this comment.
@inashivb is this the case for DCERPC/UDP with fragments ?
Is DCERPC/UDP with fragments = multiple packets but only one tx for the reassembled stuff ?
| @@ -2940,6 +2940,12 @@ static int DetectEngineCtxLoadConf(DetectEngineCtx *de_ctx) | |||
| "and 255, will default to 4"); | |||
| } | |||
| } | |||
| int force_applayer = 0; | |||
| if ((ConfGetBool("detect.force-tie-applayer-tx", &force_applayer)) == 1) { | |||
There was a problem hiding this comment.
I guess we could use more strings and have an enumeration
- tie first
- tie last
- tie if only one
- tie never
And even combine with tie if content match (PACKET_ALERT_FLAG_STREAM_MATCH), tie if UDP...
| void *tx_ptr = | ||
| AppLayerParserGetTx(pflow->proto, pflow->alproto, pflow->alstate, txid); | ||
| AppLayerTxData *txd = | ||
| tx_ptr ? AppLayerParserGetTxData(pflow->proto, pflow->alproto, tx_ptr) | ||
| : NULL; | ||
| if (txd && txd->stream_logged < de_ctx->stream_tx_log_limit) { | ||
| alert_flags |= PACKET_ALERT_FLAG_TX; | ||
| if (pflow->proto != IPPROTO_UDP) { |
There was a problem hiding this comment.
feels a bit weird to be a in a block about stream logging, then seeing udp here?
There was a problem hiding this comment.
stream_logged and stream_tx_log_limit are bad names I am afraid
There was a problem hiding this comment.
Renaming variables and commenting in another commit
| txid = AppLayerParserGetTransactionInspectId(pflow->alparser, dir); | ||
| if ((s->alproto != ALPROTO_UNKNOWN && pflow->proto == IPPROTO_UDP) || | ||
| (de_ctx->force_applayer && | ||
| AppLayerParserGetTxCnt(pflow, pflow->alstate) == txid + 1)) { |
There was a problem hiding this comment.
this is missing the case where AppLayerParserGetTxCnt==1, like for TLS. In this case we can trust the TX is relevant.
There was a problem hiding this comment.
this is missing the obvious case where AppLayerParserGetTxCnt==1,
I do not think it is missing : in this case txid==0, so AppLayerParserGetTxCnt==1==txid + 1
And I do not think it is obvious that we can trust the TX is relevant
There was a problem hiding this comment.
Adding test case in OISF/suricata-verify#2157
There was a problem hiding this comment.
this is missing the obvious case where AppLayerParserGetTxCnt==1,
I do not think it is missing : in this case txid==0, so AppLayerParserGetTxCnt==1==txid + 1
And I do not think it is obvious that we can trust the TX is relevant
But this still relies on de_ctx->force_applayer, while the idea is that you can just use that one tx
|
Next in #12197 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7199
May also solve https://redmine.openinfosecfoundation.org/issues/7406 and https://redmine.openinfosecfoundation.org/issues/7350
Describe changes:
SV_BRANCH=OISF/suricata-verify#2146
#12168 with review taken into account