Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

app-layer: websockets protocol support #10014

Closed
wants to merge 1 commit into from

Conversation

catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Dec 8, 2023

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

Describe changes:

  • app-layer: websockets protocol support
  • rust : derive for protocol enumerations strings
SV_BRANCH=pr/1528

OISF/suricata-verify#1528

#9989 with more TODOs done

What is left todo :

  • keyword for payload with its reassembly + streaming...?
  • test with other pcaps from the redmine issue

Ticket: 2695

Introduces a device EnumStringU8 to ease the use of enumerations
in protocols : logging the string out of the u8,
and for detection, parsing the u8 out of the string
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #10014 (82550d9) into master (bdec2d8) will decrease coverage by 0.15%.
Report is 22 commits behind head on master.
The diff coverage is 77.09%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10014      +/-   ##
==========================================
- Coverage   82.47%   82.33%   -0.15%     
==========================================
  Files         970      976       +6     
  Lines      271355   271720     +365     
==========================================
- Hits       223798   223716      -82     
- Misses      47557    48004     +447     
Flag Coverage Δ
fuzzcorpus 64.29% <56.98%> (-0.29%) ⬇️
suricata-verify 61.33% <74.02%> (-0.01%) ⬇️
unittests 62.82% <18.99%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

websocket.mask
--------------

A boolean to tell if the payload is masked.
Copy link
Member

Choose a reason for hiding this comment

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

Ah I see this is a bool. But why not the mask value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not seem meaningful to me.
What seems meaningful is if the payload is masked or not.

Copy link
Member

Choose a reason for hiding this comment

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

In general, keys are often used to identify malware / malware families, so I think we should make it available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@victorjulien
Copy link
Member

keyword for payload with its reassembly + streaming...?

Payload inspection can be per message, not cross message reassembly of sorts seems necessary. The size can be large, and should probably treated like file.data or http.request_body, so with configurable limits.

Additionally @zoomequipd mentioned elsewhere today that the original URI and perhaps a few other things from the HTTP request before the switch to websocket would be very helpful in certain malware detections.

@suricata-qa
Copy link

WARNING:

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

Pipeline 16967

@catenacyber
Copy link
Contributor Author

Replaced by #10060

@catenacyber
Copy link
Contributor Author

Additionally @zoomequipd mentioned elsewhere today that the original URI and perhaps a few other things from the HTTP request before the switch to websocket would be very helpful in certain malware detections.

Should not this be done with flowbits ?
That is have a rule for HTTP that will set the flow bit and no alert.
And have rules on websockets that use the flow bit ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants