Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dns: Remove parser buffering code #5087

Closed
wants to merge 2 commits into from
Closed

Conversation

jlucovsky
Copy link
Contributor

Continuation of #5020

This PR removes the buffering code from the DNS TCP parsers.

make check Rust tests succeed; suricata-verify tests also succeed.

Link to redmine ticket: 3538

Describe changes:

  • Rebased

0x65, 0x03, 0x63, 0x6F, 0x6D, 0x00,
0x00, 0x10, 0x00, 0x01, };

uint8_t buf3[] = { 0x00, 44, /* len 44 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test did not get translated to Rust nor Suricata-verify, did it ?

You can take a look at OISF/libhtp#269 to produce a pcap out of these buffers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to translate the full test to Rust? Makes sense as a Suricata-Verify test I guess, but out of scope for a DNS unit test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth considering for a new protos. Do they require this test? In theory if Suricata gets it right for one proto, it chould get it right for all, provided the proto returns the right response code - which can be tested as a unit test.

Because creating S-V tests for this is much harder if you don't already have a pcap ready.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because creating S-V tests for this is much harder if you don't already have a pcap ready.

Suricata-Verify is ok indeed, cf the proposed PR to create a pcap out of this unit test

In theory if Suricata gets it right for one proto, it should get it right for all, provided the proto returns the right response code

Well, that kind of mistake happens cf #4980

Indeed, we can reduce this to a unit test that Incomplete gets returned with the right values

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that kind of mistake happens cf #4980

Can you think of an easy way to create SV tests for this? I guess it would need to take a pcap, re organize the data over more TCP packets.. Not saying it shouldn't need to be tested, but how we can go about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input file should be something like

s2c 650342
c2s 1010
c2s 1212121516
s2c 667EF4CC

Do you want me to tweak the htptopcap.py script to use an input file like this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catenacyber Please do and we can give it a try?

Copy link
Contributor

@catenacyber catenacyber Jun 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a python script and its input file.

txt2pcap.py

import sys
import binascii
from threading import Thread
import time
import socket

# Create a pcap from a htp test file
# Launches a server on port 8080
# Launches a client in another thread that connects to it
# Both client and server read the htp test file
# And they send and receive data as described (without analysing it)
# So, you need to capture traffic on port 8080 while running the script

def removeOneEOL(s):
    r = s
    if r[-1] == '\n':
        r = r[:-1]
        if r[-1] == '\r':
            r = r[:-1]
    return r

PCAP_TCP_PORT = 5353

class ServerThread(Thread):

    def __init__(self, filename):
        Thread.__init__(self)
        self.filename = filename

    def run(self):
        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        s.bind(("127.0.0.1", PCAP_TCP_PORT))
        s.listen(1)
        conn, addr = s.accept()
        f = open(self.filename)
        sending = ""
        receiving = ""

        for l in f.readlines():
            data = binascii.unhexlify(l.split()[1])
            if l.split()[0] == "s2c":
                conn.send(data)
                print "server sent", len(data)
            else:
                data = conn.recv(len(data))
                print "server recvd", len(data)

        conn.close()
        s.close()
        f.close()


class ClientThread(Thread):

    def __init__(self, filename):
        Thread.__init__(self)
        self.filename = filename

    def run(self):
        time.sleep(1)
        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        s.connect(("127.0.0.1", PCAP_TCP_PORT))
        f = open(self.filename)
        sending = ""
        receiving = ""

        for l in f.readlines():
            data = binascii.unhexlify(l.split()[1])
            if l.split()[0] != "s2c":
                s.send(data)
                print "client sent", len(data)
            else:
                data = s.recv(len(data))
                print "client recvd", len(data)

        s.close()
        f.close()

t1 = ServerThread(sys.argv[1])
t2 = ClientThread(sys.argv[1])

# Launch threads
t1.start()
t2.start()

# Wait for threads to finish
t1.join()
t2.join()

dns.txt

c2s 001c103201000001000000000000
c2s 06676F6F676C6503636F6D0000100001
s2c 002c10328180000100010000000006676F6F676C6503636F6D0000010001c00c00010001000140ef000401020304
c2s 001c11330100000100000000000006676F6F676C65036E65740000100001

Run python txttopcap.py dns.txt while capturing traffic and you get a nice pcap
Wireshark shoed the tcp reassembly nicely for the 2 first packets

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the status of this PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is OK as is. The question is more generic in how should any protocol that uses the engine provided buffering be tested?

@victorjulien victorjulien added the needs rebase Needs rebase to master label Jul 14, 2020
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase and provide a link to a SV test?

@jlucovsky
Copy link
Contributor Author

Continued in #5189

@jlucovsky jlucovsky closed this Jul 15, 2020
@jlucovsky jlucovsky deleted the 3538/3 branch July 30, 2020 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Needs rebase to master
Development

Successfully merging this pull request may close these issues.

4 participants