Fix dns in tcp flow not detected

[zagge-cgeo]

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/

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/4759

Describe changes: Make suricata-verify pass

This replaces https://github.com/OISF/suricata/pull/7683

[codecov[bot]]

Codecov Report

Merging #7693 (e75c4fc) into master (f3d3274) will increase coverage by 0.04%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7693      +/-   ##
==========================================
+ Coverage   75.88%   75.92%   +0.04%     
==========================================
  Files         659      659              
  Lines      185668   185667       -1     
==========================================
+ Hits       140893   140967      +74     
+ Misses      44775    44700      -75     

Flag	Coverage Δ
fuzzcorpus	60.74% <ø> (+0.14%)	:arrow_up:
suricata-verify	52.51% <ø> (-0.08%)	:arrow_down:
unittests	60.71% <ø> (+<0.01%)	:arrow_up:

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

[victorjulien]

Fuzz finds a way to create lots of transactions that are never closed, this is introduced by the changes here.

[catenacyber]

Make suricata-verify pass

Which test are you talking about ?

[zagge-cgeo]

Make suricata-verify pass

Which test are you talking about ?

I'm talking about the unit tests run by suricata-verify that where failing in the first version of the patch

https://github.com/OISF/suricata/pull/7683/checks

[zagge-cgeo]

Forcing dns=11 Too many open transactions for protocol dns Assertion failure: dns

These is now failing as there is a limit of 4096 transactions in the unit tests. However the testcase create more than that number of transactions in the failing test. The test did not fail before as all transactions where ignored and none of them matched the rule in tcp.

Do you have an idea on how this could be fixed?

[catenacyber]

Make suricata-verify pass

Which test are you talking about ?

I'm talking about the unit tests run by suricata-verify that where failing in the first version of the patch

Ok thanks, usually we describe what the PR does + in another line the changes from last PR

And I guess the elements from the redmine ticket can be put into a new Suricata-Verify test.

I can reproduce the bug. But I wonder what the right fix is.

Fix dns in tcp flow not detected

The flow is detected as DNS, but the signature does not trigger...

Why do you say transactions are bidirectional For DNS over TCP ? I do not see so...

And I have no idea why adding a tls signature influences here...

[catenacyber]

Interesting fact: the alert happens 2 packets later than the logging when there is not the tls rule...

[zagge-cgeo]

Make suricata-verify pass

Which test are you talking about ?

I'm talking about the unit tests run by suricata-verify that where failing in the first version of the patch

Ok thanks, usually we describe what the PR does + in another line the changes from last PR

And I guess the elements from the redmine ticket can be put into a new Suricata-Verify test.

I can reproduce the bug. But I wonder what the right fix is.

Fix dns in tcp flow not detected

The flow is detected as DNS, but the signature does not trigger...

Why do you say transactions are bidirectional For DNS over TCP ? I do not see so...

And I have no idea why adding a tls signature influences here...

It is not the tls signature. Any signature that uses tcp will result in the behaviour.

The problem happens when there is any rule using tcp, the dns package is only looked at when the content of the request is not yet available and therefore it is not analysed afterwards as it is marked as process already.

[catenacyber]

Thanks for this analysis. So, it looks like the problem is broader, and is the fact that detection does not happen when logging does...

[catenacyber]

So, I understand this far :

• packet 4 : dns query
• packet 5 : ack of dns query and logging, no detection as packet is to client + BUG : transaction gets cleaned up by AppLayerParserTransactionsCleanup cf if (is_unidir && tx_skipped && !inspected) {
• packet 7 : running detection as packet is to server

[catenacyber]

Stranger results with

alert dns any any -> any any (msg:"DNS standard opcode"; dns.opcode: 0; id:1; rev:1;)
alert dns any any -> any any (msg:".com in DNS query"; dns.query; content:".com"; sid:2; rev:1;)

No alerts at all...

[victorjulien]

Could be a case of a missing AppLayerParserTriggerRawStreamReassembly after the TCP record has been received.

[catenacyber]

@jasonish I think your commit 60ebc27c4eb755800e6d3f4ec1a5d55a5230a214 is responsible for this bug.

For unidirectional transactions, we may run detection on the other side than the transaction.

For instance (cf previous rules posted), we may have dns rules in both directions, a packet to client asking a dns query, and thus we will run DetectRunTx which will update new_detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; for the tocliendt direction.

And then AppLayerParserTransactionsCleanup cleans up the transaction as detection has been run on one side, even if it was not the relevant side. But there is no way currently for unidirectional transactions to tell their direction...

[jasonish]

@jasonish I think your commit 60ebc27 is responsible for this bug.

For unidirectional transactions, we may run detection on the other side than the transaction.

For instance (cf previous rules posted), we may have dns rules in both directions, a packet to client asking a dns query, and thus we will run DetectRunTx which will update new_detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; for the tocliendt direction.

And then AppLayerParserTransactionsCleanup cleans up the transaction as detection has been run on one side, even if it was not the relevant side. But there is no way currently for unidirectional transactions to tell their direction...

So we should be looking at a fix that is outside of DNS and generic to all unidirectional transactions?

[catenacyber]

So we should be looking at a fix that is outside of DNS and generic to all unidirectional transactions?

I think so indeed

[jasonish]

So we should be looking at a fix that is outside of DNS and generic to all unidirectional transactions?

I think so indeed

Have you looked deep enough for a possible fix? I don't want to repeat what work might have already been done.

[catenacyber]

Just reverted the culprit commit as premature optimisation.

To keep it optimized, I think you need to implement a trait for unidirectional transaction to tell its direction. I did not write anything like it yet

[jasonish]

Just reverted the culprit commit as premature optimisation.

To keep it optimized, I think you need to implement a trait for unidirectional transaction to tell its direction. I did not write anything like it yet

Ok. So each unidirectional parser needs an update and not just the outer transaction handling.

[catenacyber]

Ok. So each unidirectional parser needs an update and not just the outer transaction handling.

I see 3 ways :

• revert the optimization (is it really helpful ?)
• change every unidirectional parser so that a transaction has the direction and use it to set inspected = true;
• Think more and find a better way :-p

[jasonish]

• revert the optimization (is it really helpful ?)

Its not an optimization, but instead fixes a real issue. Of course, 2 years ago we weren't reliable at putting ticket numbers in commits, but I think this is it: https://redmine.openinfosecfoundation.org/issues/3877

[catenacyber]

Indeed you know better, that is why I called on you Jason ;-)

[catenacyber]

So, closing this one in favor of #7835