Bug #7235
closedtls: a rule stops working since 7.0.5
Description
An alert from the rule that inspects unencrypted TLS negotiation is not reported since 7.0.5
The configuration is 'app-layer.protocols.tls.encryption-handling=bypass'
reproducer:
https://github.com/ilya-bakhtin/suricata-verify/tree/tls-bypass-missing-event
Updated by Ilya Bakhtin 4 months ago
I have already inspected the changes.
A condition is changed when StreamTcpDetectLogFlush is called. But at this moment FLOW_NOPAYLOAD_INSPECTION flag is already set to the flow flags by the application layer (TLS).
Thus StreamTcpPseudoPacketCreateDetectLogFlush prepares a pseudo packet with PKT_NOPAYLOAD_INSPECTION flag. See stream-tcp.c:7090
It looks like a pattern of communication is not commonly used. "Application Data" chunks exist in the same packet with "Server Hello". The change did not take care of this possibility.
- if (FlowChangeProto(p->flow)) {
+ // this is the first packet that sets no payload inspection
+ bool setting_nopayload =
+ p->flow->alparser &&
+ AppLayerParserStateIssetFlag(p->flow->alparser, APP_LAYER_PARSER_NO_INSPECTION) &&
+ !(p->flags & PKT_NOPAYLOAD_INSPECTION);
+ if (FlowChangeProto(p->flow) || setting_nopayload) {
StreamTcpDetectLogFlush(tv, fw->stream_thread, p->flow, p, &fw->pq);
+ if (setting_nopayload) {
+ FlowSetNoPayloadInspectionFlag(p->flow);
+ }
Updated by Ilya Bakhtin 4 months ago
A proposed fix, i'll prepare PR soon
The rationale - we have to inherit bypass options from the current packet instead of flow.
The flow object flags are already altered and must not be used at this stage.
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c@ -7083,10 +7083,10
@ static void StreamTcpPseudoPacketCreateDetectLogFlush(ThreadVars *tv,
np->vlan_idx = f->vlan_idx;
np->livedev = (struct LiveDevice_ *)f->livedev;
- if (f->flags & FLOW_NOPACKET_INSPECTION) {
+ if (parent->flags & PKT_NOPACKET_INSPECTION) {
DecodeSetNoPacketInspectionFlag(np);
}
- if (f->flags & FLOW_NOPAYLOAD_INSPECTION) {
+ if (parent->flags & PKT_NOPAYLOAD_INSPECTION) {
DecodeSetNoPayloadInspectionFlag(np);
}
Updated by Juliana Fajardini Reichow 3 months ago
Updated by Juliana Fajardini Reichow 3 months ago
- Status changed from New to In Review
PR for review: https://github.com/OISF/suricata/pull/11690
Updated by Victor Julien 3 months ago
- Status changed from In Review to Resolved
- Target version changed from TBD to 8.0.0-beta1
- Label Needs backport to 7.0 added
Staged privately, will push after the weekend.
Updated by Philippe Antoine 3 months ago
- Status changed from Resolved to Closed