Fix app-layer protocol detect patterns for SSLv2

[giannitedesco]

Make sure these boxes are signed before submitting your Pull Request -- thank you.

• [x] I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
• [x] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
• [x] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:

The SSLv2 record format starts with 2 byte length, then it has a 1 byte message-type field set to |01| for the client handshake, then a 2 byte protocol version.

The SSLv3 record format (in use with modern TLS) starts with the message-type of |16| and then the protocol, followed by the length.

This means that several of the patterns in the SSL app-layer detection code are incorrect because they're looking in the wrong place in the packet.

suricata-verify-pr: 555 OISF/suricata-verify#555 #suricata-verify-repo: #suricata-verify-branch: #suricata-update-pr: #suricata-update-repo: #suricata-update-branch: #libhtp-pr: #libhtp-repo: #libhtp-branch:

[giannitedesco]

@catenacyber created new PR, actually I got my remotes mixed up so I was rebasing on origin/master not upstream/master, woops :)

[codecov[bot]]

Codecov Report

Merging #6770 (171b078) into master (ddf9c9d) will increase coverage by 1.29%. The diff coverage is 100.00%.

:exclamation: Current head 171b078 differs from pull request most recent head 02c8ce2. Consider uploading reports for the commit 02c8ce2 to get more accurate results

@@            Coverage Diff             @@
##           master    #6770      +/-   ##
==========================================
+ Coverage   75.82%   77.12%   +1.29%     
==========================================
  Files         656      615      -41     
  Lines      190051   185613    -4438     
==========================================
- Hits       144102   143149     -953     
+ Misses      45949    42464    -3485     

Flag	Coverage Δ
fuzzcorpus	52.98% <100.00%> (-7.44%)	:arrow_down:
suricata-verify	52.67% <100.00%> (+1.14%)	:arrow_up:
unittests	63.08% <100.00%> (+2.06%)	:arrow_up:

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

[catenacyber]

Thanks CI looks better. The failures are not related to the PR

[victorjulien]

I need to look at this again. I looked earlier w/o understanding the logic and not seeing any test cases pass/fail in relation to this PR.

SSL3/TLS records can certainly start with other values than |16|.

[victorjulien]

@catenacyber can you dig a bit deeper into this?

[catenacyber]

not seeing any test cases pass/fail in relation to this PR.

The S-V tests fail without this PR and succeed with it...

For instance, sslv2-tls1_0-client-handshake.pcap tcp payload begins with bytes 80 80 01 03 01. First 2 bytes are SSLv2, Third byte is 1:client hello, Byte 4 and 5 are request version 03.01 being TLS 1.0

[giannitedesco]

@giannitedesco can you rebase both this PR and the S-V one ? It looks like sslv2-handshake is now failing with this PR

Flow has

  "app_proto": "tls",
  "app_proto_tc": "failed",

as in master

Those are supposed to fail, there is no SSLv2 decoder. The tests are included so that they will verify the SSLv2 decoder when it is added.

Is there a way to mark tests as "ignore failures" or should I redo them to just verify the protocol detection works?

[giannitedesco]

@catenacyber So I just commented out the SSLv2 protocol decode stuff, so they can be uncommented if/when that gets implemented. At least the test can verify the detection of protocol part then.

[catenacyber]

So I just commented out the SSLv2 protocol decode stuff, so they can be uncommented if/when that gets implemented. At least the test can verify the detection of protocol part then.

@inashivb is there a skip keyword for Suricata-verify ?

[catenacyber]

Those are supposed to fail, there is no SSLv2 decoder. The tests are included so that they will verify the SSLv2 decoder when it is added.

The commit message in this PR seems to say otherwise. Is it fixing SSLv2 or SSLv3 detect patterns ?

[giannitedesco]

The commit message in this PR seems to say otherwise. Is it fixing SSLv2 or SSLv3 detect patterns ?

This is fixing SSLv2 detect patterns.

There are two distinct things, whether we detect an off-port flow as SSLv2 (which current code does not, and this patch fixes). But then there is decoding the SSLv2 frames, after you have identified SSL/TLS and associated he decoder to that flow.

There are actually numerous things that the SSL/TLS decoder does not support and one of them is SSLv2, which has an incompatible frame format and totally different handshake message structure. SSLv3 and TLS use the same frame formats and are broadly compatible with each other.

[giannitedesco]

Anyway, adding support for decoding SSLv2 (and fragmented handshakes for that matter) is another question, and they are much bigger jobs.

[catenacyber]

I see that 01 03 01 is indeed used for sslv2-tls1_0-client-handshake.pcap

But I do not see which pattern is used for sslv2-handshake.pcap Is something missing ?

Anyway, adding support for decoding SSLv2 (and fragmented handshakes for that matter) is another question, and they are much bigger jobs.

Ok, understood

[catenacyber]

Flow has

  "app_proto": "tls",
  "app_proto_tc": "failed",

If we want to fix SSLv2 detect patterns, we should also detect the patterns to client (from server) Here, there are only to server (from client), right ?

[inashivb]

@inashivb is there a skip keyword for Suricata-verify ?

There is one but seems like it is

1. bound to features (e.g. https://github.com/OISF/suricata-verify/blob/master/tests/filestore-v2.3-fserror/test.yaml#L7) or
2. skipping an entire test (e.g. https://github.com/OISF/suricata-verify/blob/master/tests/dhcp-request-flood/test.yaml#L3)

I tried it on specific filters and it does not seem to work that way as of now.

[giannitedesco]

Flow has

  "app_proto": "tls",
  "app_proto_tc": "failed",

If we want to fix SSLv2 detect patterns, we should also detect the patterns to client (from server) Here, there are only to server (from client), right ?

That's right, I can investigate adding the reverse direction filter, too, thanks.

[giannitedesco]

@inashivb is there a skip keyword for Suricata-verify ?

There is one but seems like it is

1. bound to features (e.g. https://github.com/OISF/suricata-verify/blob/master/tests/filestore-v2.3-fserror/test.yaml#L7)
   or

2. skipping an entire test (e.g. https://github.com/OISF/suricata-verify/blob/master/tests/dhcp-request-flood/test.yaml#L3)

I tried it on specific filters and it does not seem to work that way as of now.

Aha, thanks, okay, for now I will forget about decoder events in the test cases and we can add tests for the decoder if and when the decoder is added in future.

[victorjulien]

I remain confused about this PR. The linked test pass in master and master6. What is the issue?

[giannitedesco]

@victorjulien the pattern that's being fixed is definitely wrong, but what's happening is that another more general pattern is overriding it in this test.

I need to get back into this because I need to determine if the broken pattern is, effectively, dead code that can be removed. Or whether I can come up with a test which properly tests it!

Sorry for the confusion. I should be able to get back into this some time in the next couple of weeks. Just been busy with work/life etc!

[catenacyber]

Thanks @giannitedesco I am closing this, waiting for a new version then