Websockets 2695 v9 #10104
Websockets 2695 v9 #10104
Conversation
if no config option is found, as is done for udp Ticket: 6304
Ticket: 6647 Allows keywords using integers to use strings in signature parsing based on a rust enumeration with a derive.
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.
So that we can write enip.revision: 0x203 Ticket: 6645
Ticket: 6648 Like &0x40=0x40 to test for a specific bit set
Ticket: 2695
catenacyber
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
|
The check rust failure is due to the use of a new version of clippy and will fail master (jsonbuilder.rs) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #10104 +/- ##
==========================================
- Coverage 82.19% 81.96% -0.23%
==========================================
Files 975 980 +5
Lines 271940 272486 +546
==========================================
- Hits 223523 223352 -171
- Misses 48417 49134 +717
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
WARNING:
Pipeline 17317 |
|
WARNING:
Pipeline 17320 |
| #[no_mangle] | ||
| pub unsafe extern "C" fn SCWebSocketParseOpcode( | ||
| ustr: *const std::os::raw::c_char, | ||
| ) -> *mut DetectUintData<u8> { | ||
| let ft_name: &CStr = CStr::from_ptr(ustr); //unsafe | ||
| if let Ok(s) = ft_name.to_str() { | ||
| if let Some(ctx) = WebSocketOpcode::to_detect_ctx(s) { | ||
| let boxed = Box::new(ctx); | ||
| return Box::into_raw(boxed) as *mut _; | ||
| } | ||
| } | ||
| return std::ptr::null_mut(); | ||
| } |
There was a problem hiding this comment.
As we return DetectUintData<u8>, it would be cleaner if we parsed that I think, like:
if let Ok(ctx) = DetectUintData::<u8>::from_str(s) {
let boxed = Box::new(ctx);
return Box::into_raw(boxed) as *mut _;
}
would require this impl though:
impl<T: DetectIntType> FromStr for DetectUintData<T> {
type Err = ();
fn from_str(s: &str) -> Result<Self, Self::Err> {
if let Ok((_, v)) = detect_parse_uint::<T>(s) {
Ok(v)
} else {
Err(())
}
}
}
There was a problem hiding this comment.
Consider this patch to keep things generic: https://gist.github.com/jasonish/43f6672af27fc541352285853cd64edc
Or lets give it a better name. This is an enum type that is tightly couple to integer types and detectuintdata..
So maybe DetectIntEnum. This fits with where this trait is currently implemented, in the detect crate, and now that the naming is clear, I think its more fair to put arbitrary code in that that is less generic.
There was a problem hiding this comment.
How does the patch wok for websocket.opcode: 1 ? (numeric value instead of string value)
There was a problem hiding this comment.
How does the patch wok for
websocket.opcode: 1? (numeric value instead of string value)
Oh, the tests must not test for that as S-V did pass.
I do have the trait there so you can do:
let ctx = DetectUintData::from_str(s)
as well. So you would try one that the other. It just moves the conversions to be a long side their datatype, rather than somewhere that seems a little random.
There was a problem hiding this comment.
It just moves the conversions to be a long side their datatype, rather than somewhere that seems a little random.
Seems right, see d5f1bf2 for next iteration
|
Draft until #10110 is merged and this is rebased on it |
|
Continues in #10119 |
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/2695
https://redmine.openinfosecfoundation.org/issues/6647
https://redmine.openinfosecfoundation.org/issues/6645
https://redmine.openinfosecfoundation.org/issues/6648
Describe changes:
OISF/suricata-verify#1571
#10093 with
I think this is good enough for a first version even if there may be improvements (that can happen in later tickets) :
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
#10089 and #9870 should likely precede this