app-layer: websockets protocol support #9989
Conversation
|
WARNING:
Pipeline 16907 |
|
Also #9870 should be merged before this to avoid a short-lived output-json-websockets.c file ;-) |
|
QA : it is expected to have a rise of pseudo packets on the protocol change from HTTP1 to WebSocket |
etc/schema.json
Outdated
| @@ -3823,6 +3823,9 @@ | |||
| }, | |||
| "tls": { | |||
| "$ref": "#/$defs/stats_applayer_error" | |||
| }, | |||
| "websockets": { | |||
There was a problem hiding this comment.
rfc 6455 refers to it as "WebSocket" (singular), so lets use singular in our code and docs
rust/src/websockets/logger.rs
Outdated
|
|
||
| fn log_websockets(tx: &WebSocketsTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { | ||
| js.open_object("websockets")?; | ||
| js.set_bool("mask", tx.pdu.mask)?; |
There was a problem hiding this comment.
would it make sense to log the mask value?
There was a problem hiding this comment.
I do not think so.
Frame can be used to detect on it...
rust/src/websockets/websockets.rs
Outdated
| self.transactions.push_back(tx); | ||
| } | ||
| Err(nom::Err::Incomplete(_)) => { | ||
| // Not enough data. just ask for one more byte. |
There was a problem hiding this comment.
as the header seems to have a payload_len, could we use that here?
Also what is the max payload size?
There was a problem hiding this comment.
A frame, per spec can be up to 9,223,372,036,854,775,807 bytes I believe.
There was a problem hiding this comment.
frames the size of the universe, of course
There was a problem hiding this comment.
but I guess it means we need to consume all payload data directly, even if incomplete. Process them the same as other streaming buffers like http body and file.data
There was a problem hiding this comment.
frames the size of the universe, of course
In practice, most implementations don't allow extremely large payloads, I think 16kb even being on the extreme.. But we of course still need to deal with universe sized frames somehow.
There was a problem hiding this comment.
as the header seems to have a payload_len, could we use that here?
Indeed, I was just lazy :-p
frames the size of the universe, of course
half the size of the universe :-p
other streaming buffers like http body and file.data
Are there other examples than http body ?
Another way could be to set a config limit (and skip the rest of the payload)
src/app-layer-htp.c
Outdated
| } | ||
| SCReturnStruct(APP_LAYER_OK); | ||
| } else if (bstr_cmp_c_nocase(h->value, "WebSocket") == 0) { | ||
| if (AppLayerProtoDetectGetProtoName(ALPROTO_WEBSOCKETS) == NULL) { |
There was a problem hiding this comment.
maybe we can cache this as a flag in the htp_state? Seems like a fairly expensive lookup of a static value (same for http2 a few lines up)
There was a problem hiding this comment.
What do you want to cache ?
The header Upgrade value being WebSocket ?
(we compare it only once)
There was a problem hiding this comment.
AppLayerProtoDetectGetProtoName does a table lookup, but the answer will always be the same. So we could look it up once and store the result
There was a problem hiding this comment.
Oh. Why is this "table lookup" fairly expensive ?
To me, it looks negligible next to the string comparison
| break; | ||
| } | ||
| hstate->slice = NULL; | ||
| if (AppLayerExpectationCreate(f, STREAM_TOCLIENT | STREAM_TOSERVER, f->sp, |
There was a problem hiding this comment.
surprised to see this. Expectations are about related but unique flows (so different 5 tuple), that is not the case here, right?
There was a problem hiding this comment.
Websocket cannot be recognized by a fixed pattern, neither by a probing parser... (and they should not as we should not recognize this protocol without the HTTP1 handshake )
So, expectation looks the only option left...
Another way would be AppLayerRequestProtocolChange-ish call to force the protocol change... (instead of running TcpProtocolDetect)
Expectations are about related but unique flows (so different 5 tuple), that is not the case here, right?
Here, we have one 5-tuple flow, but expectations work here even if they were meant for ftp-data, so for related but different flows. They also work for a single flow changing protocol.
TL;DR This is a POC/draft and I will reach out about this again in a good version if I do not find anything better until then ;-)
There was a problem hiding this comment.
Expectations create a new flow, so shouldn't be used here. It's the wrong API for the problem.
Forcing the protocol change, perhaps with some validation that it indeed looks like websock, sounds better.
There was a problem hiding this comment.
Forcing the protocol change, perhaps with some validation that it indeed looks like websock, sounds better.
Will look into it (validation will be checking 3 reserved bits are 0, not so great)
Expectations create a new flow, so shouldn't be used here. It's the wrong API for the problem.
The way I understood is that expectation do not create a new flow.
A packet creates a new flow, which may be checked against current expectations to recognize its protocol.
What is wrong in my understanding ?
There was a problem hiding this comment.
Indeed, not a flow but something worse: a entry in the ip_pair table, pretty expensive operation.
There was a problem hiding this comment.
Now, that looks evil 👿
Thanks for the explanation :-)
There was a problem hiding this comment.
Got a new hack for next PR iteration
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
catenacyber
force-pushed
the
websockets-2695-v2
branch
from
17f6ef0
to
82550d9
Compare
|
Replaced by #10014 |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2695
Describe changes:
Just a draft to show started work
OISF/suricata-verify#1519
#9987 with more TODOs done
What is left todo :