Skip to content

Comments

Websockets 2695 v15#10176

Closed
catenacyber wants to merge 15 commits intoOISF:masterfrom
catenacyber:websockets-2695-v15
Closed

Websockets 2695 v15#10176
catenacyber wants to merge 15 commits intoOISF:masterfrom
catenacyber:websockets-2695-v15

Conversation

@catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Jan 16, 2024

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2695

Describe changes:

SV_BRANCH=pr/1571

OISF/suricata-verify#1571

#10173 with rebase on top of #10175 (to get green CI for check rust)

I think this is good enough for a first version even if there may be improvements (that should happen in later tickets) :

  • using latest HTTP/1 transaction in a single rule, instead of splitting rule and using flowbits. Should the Websocket state own the last HTTP1 transaction ? And if so, need to see with DOH2 as well the mechanism to get the right transaction for each keyword...
  • support websockets over HTTP/2 : need more design
    This is a big one as websockets over HTTP/2 only use a single HTTP/2 stream and not the whole TCP connection which keeps having newer regular HTTP/2 streams (or other websocket ones). That means a HTTP2 transactions will own a Websocket State + some streaming buffer as TCP...

@suricata-qa
Copy link

WARNING:

field baseline test %
build_asan

Pipeline 17539

catenacyber and others added 15 commits January 16, 2024 15:42
So that we can write enip.revision: 0x203

Ticket: 6645
Ticket: 6647

Allows keywords using integers to use strings in signature
parsing based on a rust enumeration with a derive.
Ticket: 6648

Like &0x40=0x40 to test for a specific bit set
Ticket: 6628

Document the generic detection capabilities for integer keywords.
and make every integer keyword pointing to this section.
if no config option is found,
as is done for udp

Ticket: 6304
Including the one for websocket over HTTP/2
port is used in AppLayerProtoDetectProbingParserPort
and not in AppLayerProtoDetectProbingParserElement
As for WebSocket which is detected only by protocol change.
When there is a protocol change, and a specific protocol is
expected, like WebSeocket, always run it, no matter the port.
@catenacyber
Copy link
Contributor Author

catenacyber commented Jan 16, 2024

CI should be red/SV is failing because OISF/suricata-verify#1571 is missing to use OISF/suricata-verify#1490

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.tcp.pseudo 2810 3014 107.26%

Pipeline 17554

@zoomequipd
Copy link

Question based on the changes to the docs:

does this mean that we need to rewrite our use of integer-keywords (dsize, bsize, urilen, etc)?

based on the examples

 bsize:10; --> bsize:integer 10;

is that right or am i misunderstanding?

@catenacyber
Copy link
Contributor Author

Question based on the changes to the docs:

does this mean that we need to rewrite our use of integer-keywords (dsize, bsize, urilen, etc)?

based on the examples

 bsize:10; --> bsize:integer 10;

is that right or am i misunderstanding?

#10179 is the PR for integer keywords.

You do not need to rewrite anything bsize:10; works
bsize:integer 10; does not work.

So, the doc looks unclear. How would you spell it ?

@zoomequipd
Copy link

#10179 is the PR for integer keywords.

sorry, I was looking through this PR and noticed it, would you prefer this convo to occur on #10179?

@catenacyber
Copy link
Contributor Author

#10179 is the PR for integer keywords.

sorry, I was looking through this PR and noticed it, would you prefer this convo to occur on #10179?

Ideally yes but we can keep it here no big deal...

@zoomequipd
Copy link

zoomequipd commented Jan 21, 2024

support websockets over HTTP/2 : need pcap

was the pcap I shared via redmine not sufficient or do you just want another ticket for that feature?

@victorjulien victorjulien marked this pull request as draft January 22, 2024 08:28
@catenacyber
Copy link
Contributor Author

support websockets over HTTP/2 : need pcap

was the pcap I shared via redmine not sufficient or do you just want another ticket for that feature?

@zoomequipd thanks for the pcap, it is enough for now, this was just a bad copy/paste from old data, my mistake.

This is a big one as websockets over HTTP/2 only use a single HTTP/2 stream and not the whole TCP connection which keeps having newer regular HTTP/2 streams (or other websocket ones). That means a HTTP2 transactions will own a Websocket State + some streaming buffer as TCP...

This still stands...

@catenacyber catenacyber marked this pull request as ready for review January 23, 2024 08:17
@catenacyber
Copy link
Contributor Author

Rebased in #10375

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.

3 participants