Skip to content

detect: do not run tx detection on non established packets#10304

Closed
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:tcp-flag-established-even-on-error-v2
Closed

detect: do not run tx detection on non established packets#10304
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:tcp-flag-established-even-on-error-v2

Conversation

@catenacyber
Copy link
Contributor

Follows commit 2fb5059, concurrent against #10286

Link to redmine ticket:
Should there be one specific for this ?

Describe changes:

  • stream/tcp: flag established even on error (error being Suricata StreamTcpStateDispatch returning an error value)

SV_BRANCH=OISF/suricata-verify#1623

#10299 alternative to run less code when packet is not flagged as established (instead of flagging more packets as established)

@catenacyber catenacyber changed the title detect: do not run tx detection on non establishged packets detect: do not run tx detection on non established packets Feb 2, 2024
/* run tx/state inspection. Don't call for ICMP error msgs. */
if (pflow && pflow->alstate && likely(pflow->proto == p->proto)) {
if (pflow && pflow->alstate && likely(pflow->proto == p->proto) &&
(p->flags & PKT_STREAM_EST)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing I would like to know here is if TCP fast open, data on syn, IPS mode (so app-layer is called immediately) also sets this flag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should a similar check be done before the tx logging is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should a similar check be done before the tx logging is called?

There are likely other places where this optimization can happen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the flag will only be set for TCP I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing I would like to know here is if TCP fast open, data on syn, IPS mode (so app-layer is called immediately) also sets this flag

I do not think it does :
Code should get into StreamTcpPacketStateNone, case if (p->tcph->th_flags & TH_SYN) and TCP_HAS_TFO so not going to set PKT_STREAM_EST because not in same code path as StreamTcpPacketStateNone

@victorjulien
Copy link
Member

replaced by #10307

can you open tickets for:
tcp fast open issue
error handling issue

@catenacyber
Copy link
Contributor Author

replaced by #10307

can you open tickets for: tcp fast open issue error handling issue

https://redmine.openinfosecfoundation.org/issues/6743
https://redmine.openinfosecfoundation.org/issues/6744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants