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

detect-lua: add tests #1086

Closed
wants to merge 1 commit into from

Conversation

TheKharleeci
Copy link
Contributor

Includes test for LuaMatchTest01-LuaMatchTest06

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

Previous PR: #688

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

I have pointed out some aspects that need more work on, and which I think would allow us to at least have alerts for all rules.

If ensuring that the pcap has both packets to_server isn't enough to trigger all rules, I suggest maybe using a different pcap - maybe even using one from some existing SV test. :)

Would be great if we could manage to have the alert checks also check the packet number, but not sure how easy that would be - I tried a bit, but the pcaps I tried didn't have pcap_cnt fields in the alert outputs, so maybe we can ignore it by now...

alert tcp any any -> any any (flow:to_server; lua:test2.lua; sid:2;)
alert tcp any any -> any any (flow:to_server; lua:test3.lua; sid:3;)
alert http any any -> any any (flow:to_server; lua:test4.lua; sid:4;)
alert http any any -> any any (flow:to_server; lua:test4.lua; sid:5;)
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this rule is using test4.lua instead of test5.lua

- HAVE_LUA

args:
- -k none
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove space in the beginning of the line.
For a pcap like this one, we should also add - --set stream.midstream=true

@@ -0,0 +1 @@
Test http buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see more details here, like listing the different buffers that are mentioned to be tested by the unittests, and which scripts are covering what.

types:
- alert
- flow

Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness, I think it's good to add http here, too.

end

function match(args)
a = ScFlowvarGet(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that in the unittests the a versions of the tests were testing the SC variant of the lua functions, and currently we're losing this with these SV tests, so we must tackle this, too, somehow.

Comment on lines +6 to +11
pkts += Ether(dst='ff:ff:ff:ff:ff:ff', src='00:01:02:03:04:05')/ \
Dot1Q(vlan=6)/ \
IP(dst='192.168.1.1', src='192.168.1.5')/TCP(sport=53, dport=80, flags='P''A')/"POST / HTTP/1.1\r\nHost: www.emergingthreats.net\r\n\r\n"
pkts += Ether(dst='ff:ff:ff:ff:ff:ff', src='00:01:02:03:04:05')/ \
Dot1Q(vlan=6)/ \
IP(dst='192.168.1.1', src='192.168.1.5')/TCP(sport=80, dport=53, flags='P''A')/"POST / HTTP/1.1\r\nHost: www.openinfosecfoundation.org\r\n\r\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the unittests, we have two requests to the server, while here we see packets to different directions.

@inashivb inashivb mentioned this pull request Apr 18, 2023
@inashivb
Copy link
Member

Replaced w #1165

@inashivb inashivb closed this Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants