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

HTTP analyzer does not produce any log for traffic on non-standard port #88

Closed
bbannier opened this issue Sep 30, 2021 · 3 comments · Fixed by #89
Closed

HTTP analyzer does not produce any log for traffic on non-standard port #88

bbannier opened this issue Sep 30, 2021 · 3 comments · Fixed by #89
Assignees

Comments

@bbannier
Copy link
Contributor

bbannier commented Sep 30, 2021

@mmguero reported the following issue on Slack:

I generated a sample PCAP and put together a simple test and noticed that the http.log generated by Zeek for that PCAP is only generated when using Zeek's built-in HTTP analyzer (either through Spicy::disable_protocol_analyzer(Analyzer::ANALYZER_SPICY_HTTP); or by doing zkg remove spicy/spicy-analyzers). When using the zeek/spicy-analyzers HTTP analyzer, the HTTP traffic doesn't get detected at all.

I looked into this and was able to reproduce the issue with a zeek-4.1.1 from Homebrew, and the most recent releases of spicy, spicy-plugin and spicy-analyzers. When I rebuild a zeek-4.1.1 from source (in DEBUG mode for -B dpd support), the issue went away.

We should look into this. It might be that e.g., Zeek-builtin DPDP for HTTP doesn't trigger for our HTTP analyzer which claims to replaces HTTP.

@bbannier bbannier self-assigned this Sep 30, 2021
@bbannier
Copy link
Contributor Author

The issue seems to be due to how replaces works. While its name suggests that we'd swap one analyzer for another one, this isn't really what happens: instead we disable the old analyzer and add and activate another one in its place. This replacement analyzer e.g., needs to have another name since the old analyzer is still there (but deactivated).

Zeek's builtin HTTP analyzer gets activated for this traffic even on a non-standard port since it matches a builtin DPD signature for HTTP traffic. DPD signatures explicitly hardcode the name of an analyzer to enable on match, so e.g., Zeek's HTTP signatures enable the builtin HTTP analyzer. When we replace it in spicy-plugin, we do not add any new activation rules or update existsing ones. I'll have a look at Zeek's DPD logic whether we can on analyzer registration update existing signatures to also trigger the replacement analyzer.

bbannier added a commit that referenced this issue Sep 30, 2021
When specifying `replaces` in the analyzer glue we currently deactivate
the replaced analyzer before activating the replacement. We do however
not update the names referenced in DPD signatures, see e.g.,
zeek/spicy-plugin#69.

In order to activate our replacement analyzers in the same scenarios
builtin analyzers would have been activated by DPD, this patch copies
over the zeek-4.1.1 signatures of all replacement analyzers where the
replaced analyzers could have been activated with DPD in Zeek (HTTP and
DHCP).

Closes #88.
@rsmmr
Copy link
Member

rsmmr commented Sep 30, 2021

This sounds like a bug (or oversight) then: the replaces is supposed to take over the existing activation rules as well so that enable "http" now triggers the new analyzer.

@bbannier
Copy link
Contributor Author

This sounds like a bug (or oversight) then: the replaces is supposed to take over the existing activation rules as well so that enable "http" now triggers the new analyzer.

Ah, I see that we pass replaces into Zeek, but for some reason DPD doesn't pick up the new name. I'll continue digging in Zeek to figure out what's going on there and will potentially file a bug there.

@rsmmr rsmmr closed this as completed in cdfa13b Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants