5912 rfb skip v4

[satta]

Previous PR: #9064

Link to redmine ticket: #5912

Describe changes to previous PR:

• Add Redmine bug number to main commit with relevant changes.

[catenacyber]

Thanks Sascha.

CI : green
Code : a few questions
Commits segmentation : ok for me
Commit messages : ok for me. @satta the ticket number should be on its own line as Ticket: #5912 cf git log | grep '#'
Git ID set : ok
CLA : ok
Doc update : none needed
Redmine ticket : ok
Rustfmt : ok
Tests : I wonder if the IPS app-layer.error-policy: drop-flow should be tested somehow
Dependencies added: none

[codecov]

Codecov Report

Merging #9071 (d0dc924) into master (643e674) will decrease coverage by 0.09%.
The diff coverage is 40.17%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9071      +/-   ##
==========================================
- Coverage   82.35%   82.26%   -0.09%     
==========================================
  Files         969      969              
  Lines      273655   273721      +66     
==========================================
- Hits       225359   225173     -186     
- Misses      48296    48548     +252     

Flag	Coverage Δ
fuzzcorpus	64.51% <38.26%> (-0.07%)	⬇️
suricata-verify	60.58% <24.34%> (-0.07%)	⬇️
unittests	62.89% <7.69%> (-0.02%)	⬇️

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

[satta]

Thanks Sascha.

CI : green Code : a few questions Commits segmentation : ok for me Commit messages : ok for me. @satta the ticket number should be on its own line as Ticket: #5912 cf git log | grep '#'

Will keep this in mind for next time.

[victorjulien]

Thanks Sascha.
CI : green Code : a few questions Commits segmentation : ok for me Commit messages : ok for me. @satta the ticket number should be on its own line as Ticket: #5912 cf git log | grep '#'

Will keep this in mind for next time.

Patched this up in staging.

[satta]

[...]

Patched this up in staging.

Thanks!

[victorjulien]

It gives more RFB flow and transactions, and fewer errors. W/o looking at the (private) traffic, does this seem plausible based on the changes?

See #9083 (comment)

[satta]

I would say so. The point of this change was to reduce the amount of app-layer errors when technically there is no error but we just don't implement all of RFB's variety in the field.
Error should now be restricted to cases where we can't even see a proper handshake at the beginning of the flow.

[victorjulien]

I would say so. The point of this change was to reduce the amount of app-layer errors when technically there is no error but we just don't implement all of RFB's variety in the field. Error should now be restricted to cases where we can't even see a proper handshake at the beginning of the flow.

Thanks Sascha!

[victorjulien]

Merged in #9083, thanks!

[catenacyber]

For information @satta the rfb error comes from a real RFB conversation, but the TCP client first sends Data: 0500040003c61ef656 before the RFB version...

[catenacyber]

More important @satta, every time you self.state = parser::RFBGlobalState::Skip;, you should also current_transaction.complete = true;

Otherwise, the transaction does not get logged...

[catenacyber]

And it looks like the client can send its version first, which is not handled bu the current state machine

[satta]

And it looks like the client can send its version first, which is not handled bu the current state machine

I can't see any indication of this being valid in the protocol [1][2] -- the server always advertises its version first AFAICS. Could we be seeing one-sided traffic or something like that there?

[1] https://github.com/rfbproto/rfbproto/blob/master/rfbproto.rst#handshaking-messages
[2] https://datatracker.ietf.org/doc/html/rfc6143#section-7.1.1

[satta]

For information @satta the rfb error comes from a real RFB conversation, but the TCP client first sends Data: 0500040003c61ef656 before the RFB version...

No idea what this could be. Can you share a pcap?

[satta]

More important @satta, every time you self.state = parser::RFBGlobalState::Skip;, you should also current_transaction.complete = true;

Otherwise, the transaction does not get logged...

I guess we can still log whatever data we have so far then, right. I'll add that.

[catenacyber]

These pcaps cannot be shared unfortunately

the server always advertises its version first AFAICS.

Both sides, TCP client first, then TCP server sends the RFB version

[satta]

the server always advertises its version first AFAICS.

Both sides, TCP client first, then TCP server sends the RFB version

Where do you see this? The specs I linked in my previous message above as well as the pcaps I have recorded from my own traffic state that the server that is contacted via TCP sends its version to the connecting client first to make it possible for the client to check if a common protocol version can be found:

Handshaking begins by the server sending the client a ProtocolVersion message. This lets the 
client know which is the highest RFB protocol version number supported by the server. The 
client then replies with a similar message giving the version number of the protocol which 
should actually be used [...]

[catenacyber]

I see this in private pcaps :-/

Is it a mismatch between TCP server and RFB server ?

[satta]

I see this in private pcaps :-/

Is it a mismatch between TCP server and RFB server ?

Don't think so. All the pcaps I've seen agree with the spec. Could it be that the pcap you're seeing this in might have a collection artifact of some sort? Can you confirm the session is valid? Can Wireshark dissect the communication correctly?

[catenacyber]

Wireshark dissects correctly but says Server protocol version for the first one which comes from the TCP client.