Encrypted TLS bypass dependency on stream.bypass

Developers
1

Hi,
I’ve been messing around with bypassing TLS encrypted traffic with app-layer.protocols.tls.encryption-handling set to bypass and stream.bypass set to yes.
Some other threads on the topic:
Bypassing encrypted traffic does not seem to work - Help - Suricata
Stream bypass documentation note - Developers - Suricata
and my own: Bypass-mark/bypass-mask not working in NFQ mode - Help - Suricata

This works as documented, but I now have the unwanted side-effect where my other non-TLS streams are bypassed once I’ve reached the reassembly depth.
I looked at the code, and from what I could tell, there’s no real reason for this dependency, it’s a simple if: suricata/stream-tcp.c at master · OISF/suricata (github.com)

STREAMTCP_FLAG_BYPASS is only set by StreamTcpSetSessionBypassFlag
which is only called once in the entire codebase in the app-layer parser and only when APP_LAYER_PARSER_BYPASS_READY is set, which is only turned on when ssl_config.encrypt_mode is set to SSL_CNF_ENC_HANDLE_BYPASS, here and here.

In other words, there are no other flows that set STREAMTCP_FLAG_BYPASS as far as I can tell.

So my question is - why is this dependency here? Why not call PacketBypassCallback without asking if (StreamTcpBypassEnabled())?

Thanks.

2

What version are you using and how does your suricata.yaml look like for those tests?

3

I’m using Suricata 6.0.1.
The relevant parts of the config are:

stream:
  bypass: yes
  memcap: 64mb
  checksum-validation: yes
  inline: auto
  reassembly:
    memcap: 256mb
    depth: 1mb
    toserver-chunk-size: 2560
    toclient-chunk-size: 2560
    randomize-chunk-size: yes

app-layer:
  protocols:
    tls:
      enabled: yes
      detection-ports:
        dp: 443
      ja3-fingerprints: yes
      encryption-handling: bypass

But this doesn’t seem version related, it’s right there in the code and this behavior is explicitly documented (ignoring the documentation error calling it encrypt-handling instead of encryption-handling):

When encrypt-handling is set to bypass , all processing of this session is stopped. No further parsing and inspection happens. If stream.bypass is enabled this will lead to the flow being bypassed, either inside Suricata or by the capture method if it supports it and is configured for it.

The question is why? I don’t see any reason to couple these 2 features (not to say that there is no reason, just saying I don’t see it). The only reasoning I can think of is that it just follows the common coding pattern/convention where PacketBypassCallback(p) is called only if StreamTcpBypassEnabled() returns true, regardless of whether that’s an actual prerequisite.

4

Any input on this?
I removed the said if statement in a private fork, and the feature works without enabling stream.bypass as expected.

5

Hey Dean,

I personally don’t see any dependency between the stream and TLS bypass at the moment. You might be right about the coding convention or alternatively, the initial thought could have been in a sense like TCP bypass is a prerequisite to enable bypass of the TCP traffic subdomain. Would it be possible for you to create a PR for that so it goes through our CI checks? Maybe it will trip something.
Thank you.

Another alternative that I see:
Considering line 657 in stream-tcp-reassemble.c can you try to set the depth to 0? That should mean that there is no depth limit and you essentially get what you are after. That way you wouldn’t need to modify Suricata source code.

6

Hi!

I’m trying to understand your problem.

but I now have the unwanted side-effect where my other non-TLS streams are bypassed once I’ve reached the reassembly depth.

From what I understand, this seems to have been enabled by stream.bypass: yes. We are telling Suricata to bypass the entire flow once stream.reassembly.depth is reached irrespective of tls with this setting. So, I am unable to understand why this is an “unwanted” side-effect? :thinking:

why is this dependency here? Why not call PacketBypassCallback without asking if (StreamTcpBypassEnabled())?

My guess is: to avoid aggressive bypassing. If we did that, we would end up bypassing the flows irrespective of stream.bypass being set or not. This can potentially lead to some rules that were expected to match on the traffic that is now bypassed; not match. (Edit: detection is disabled at this point so I take back this point.) This is probably the reason every time we call PacketBypassCallback, we check for sure if stream.bypass is enabled except for when the bypass happens through rules or exception policies.

However, I would expect tls.encryption-handling: bypass to indeed bypass the encrypted flow and override the stream bypass settings. Perhaps someone more experienced could shed a light on if and why such an expectation would be wrong.

I might be incorrect above but this part of the code is intriguing to me so I’m interested in hearing of what you think.

As for the solution for now, as Lukas pointed out, stream.reassembly.depth can be set to 0 (which means infinite) and should not lead to bypassing of the other non-tls traffic in your setup.

7

From what I understand, this seems to have been enabled by stream.bypass: yes . We are telling Suricata to bypass the entire flow once stream.reassembly.depth is reached irrespective of tls with this setting. So, I am unable to understand why this is an “unwanted” side-effect? :thinking:

Judging from the rest of your comment, it seems like you understand exactly what I mean by “unwanted side-effect”.
I want: Bypass encrypted traffic in a TLS session.
I get: Bypass encrypted traffic in a TLS session + bypass the entire stream when stream.reassembly.depth is reached in all other flows (TLS or otherwise).

The bold part is the unwanted side effect.

My guess is: to avoid aggressive bypassing. If we did that, we would end up bypassing the flows irrespective of stream.bypass being set or not. This can potentially lead to some rules that were expected to match on the traffic that is now bypassed; not match. This is probably the reason every time we call PacketBypassCallback , we check for sure if stream.bypass is enabled except for when the bypass happens through rules or exception policies.

That would be true if that section of the code would be for any flow, but it’s only relevant to TLS flows. The part about bypassing once stream.reassembly.depth has been filled is another part of the code. so there’s no “aggressive bypassing”, it’s only bypassing the encrypted part of the TLS session, which is what the feature is all about.

As for setting stream.reassembly.depth to 0 like Lukas and you (Shivani) suggested, that would probably work, but it would create another side-effect of having an infinite reassembly depth :slight_smile:, my guess is that it could lead to some memory issues, but I didn’t actually go deep into the code to verify that intuition.

Like I mentioned, I already made the change on a local fork and it seems to work as expected: TLS is being bypassed only on the encrypted part, without stream.bypass being enabled.

I’ll try to create a PR as per Lukas’ suggestion and see if it triggers anything, will updated here when that happens.

Thanks for your input!
Dean.

1 Like
8

As suggested, I created a PR, workflows are awaiting approval, so would appreciate one :slight_smile:
Decouple stream.bypass dependency from TLS encrypted bypass by msdean · Pull Request #9127 · OISF/suricata (github.com)

1 Like
9

Hi,
Seems like all tests passed, the commit message was reviewed and approved, and I ticked all the PR boxes.
Can someone please review and approve the merge?

Thanks,
Dean.

10

Thanks, I think we need a redmine ticket to track this (and backport it)

11

It is also set in rust/src/ssh/ssh.rs