Ssh frames 5734 v2 #11372
Ssh frames 5734 v2 #11372
Conversation
Ticket: 5734
for SSH packets that mark the end of plaintext
|
Information: QA ran without warnings. Pipeline 21265 |
| @@ -149,6 +159,30 @@ impl SSHState { | |||
| while !input.is_empty() { | |||
| match parser::ssh_parse_record(input) { | |||
| Ok((rem, head)) => { | |||
| let _pdu = Frame::new( | |||
There was a problem hiding this comment.
I think it would make sense to move this outside of Ok, as we can want the frames in malformed data as well. I suppose you would just open all 3 (RecordHdr with fixed len, the other two with -1 len), then set the final length on Ok.
In general, I am missing an explanation of the frames in the commit message.
Also should add docs for the frames, cf https://docs.suricata.io/en/latest/rules/smtp-keywords.html#frames
There was a problem hiding this comment.
I think it would make sense to move this outside of Ok, as we can want the frames in malformed data as well. I suppose you would just open all 3 (RecordHdr with fixed len, the other two with -1 len), then set the final length on Ok.
I am not sure it makes sense for SSH :
- malformed data can only be "negative length" for SSH record data (in SSH, there is the anti pattern that the length includes the fixed header length), and in this case, the flow is placed into error. So it is then impossible to "set the final length on Ok."
- for incomplete data, it is already handled in one way
Should I spend some time again to try to make a SV test that shows a different behavior with -1 len ?
In general, I am missing an explanation of the frames in the commit message.
Adding something
Also should add docs for the frames
I thought frames were automatically documented, by some rustdocs magic with // app-layer-frame-documentation tag start: FrameType enum
cc @jasonish ?
Add adding something like smtp
There was a problem hiding this comment.
I thought frames were automatically documented, by some rustdocs magic with
// app-layer-frame-documentation tag start: FrameType enum
cc @jasonish ?
Am not familiar with anything like this.
There was a problem hiding this comment.
See b846269
I had interpreted this wrongly
|
Continued in #11415 |
Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/5734
Describe changes:
SV_BRANCH=OISF/suricata-verify#1932
#11340 with clear PR history