zeek/spicy: Built-in spicy modules cannot be replaced.

[jlucovsky]

When Zeek is built with built-in spicy modules and a custom spicy module is loaded that uses the replaces keyword for an existing spicy module, the existing spicy module is not replaced -- it's left loaded along with the custom spicy module which requested the replacement.

[rsmmr]

Replacement was originally meant for replacing Zeek's traditional analyzers, not Spicy analyzers, so not totally surprised. Do you have a short example showing the problem (specifically, where exactly are the "builtin-in Spicy modules" coming from / loaded from?)

[bbannier]

FTR, I tried to reproduce this, but there seems to be an issue setting up builtin Spicy analyzers with the default build, see zeek/zeek#2442. A proper reproducer would certainly help.

[sethhall]

Sorry, the original ticket is explained slightly wrong.

It's intending to say that with a builtin spicy-plugin, spicy analyzers don't do the replaces behavior correctly. The existing analyzer they're supposed to replace isn't disabled.

[bbannier]

Unless I misunderstand what you are describing, I do not see this when installing a Spicy analyzer in the upstream image docker.io/zeekurity/zeek which has spicy-plugin as a builtin plugin:

• Initial state of image:

  $ zeek -NN | grep 'PE '
  Zeek::PE - Portable Executable analyzer (built-in)
      [File Analyzer] PE (ANALYZER_PE, enabled)

• Install & use https://github.com/zeek/spicy-pe with zkg:

  $ zkg install spicy-pe
  $ zeek -NN | grep 'PE '
  Zeek::PE - Portable Executable analyzer (built-in)
      [File Analyzer] PE (ANALYZER_PE, disabled)
      [File Analyzer] spicy_PE (ANALYZER_SPICY_PE, enabled)
  

I also looked into concrete logs. If I e.g., run https://github.com/zeek/spicy-pe/blob/main/tests/traces/ftp-pe.pcap through zeek I see the expected analyzers in files.log.

Could you provide a reproducer that shows the problem?

[bbannier]

@jlucovsky shared his analyzer with me privately, and I can confirm the issue.

To reproduce:

$ zkg create --features spicy-analyzer --packagedir=teredo --user-var name=Teredo --user-var namespace=foo

Adjust teredo/analyzer/foo.evt to replace the builin Teredo analyzer:

protocol analyzer spicy::Teredo over TCP:
    parse with foo::Teredo,
    replaces Teredo, 
    port 8080/tcp;

Install the analyzer with zkg install --skiptests teredo.

I then see both the builtin Teredo analyzer and the just installed one enabled at the same time:

$ zeek -NN | grep -i 'Teredo '
    [Analyzer] spicy_Teredo (ANALYZER_SPICY_TEREDO, enabled)
Zeek::Teredo - Teredo packet analyzer (built-in)
    [Packet Analyzer] Teredo (ANALYZER_TEREDO)

I tried drilling down more where this came from and an important factors seem to be that replaced analyzer has a name after Zeek::Spicy when sorted in lexicographical order (probably why my test replacing PE failed); I don't seem to see this issue if I attempt to replace e.g., HTTP.

Since nothing in spicy-plugin cares about the order of registered analyzers and it instead calls Zeek APIs (see e.g., here or here I suspect this is an issue in Zeek proper.

[bbannier]

With above reproducer the smoking gun is in HILIT's zeek debug log:

$ HILTI_DEBUG=zeek zeek -N >/dev/null
[zeek] Registering TCP protocol analyzer spicy_Teredo with Zeek
[zeek]   Scheduling analyzer for port 8080/tcp
[zeek] Done with post-script initialization

If the analyzer actually replaced another analyzer we'd expect mention of that in the output,

spicy-plugin/src/plugin.cc

Lines 114 to 118 in ab743fb

	if ( auto tag = ::zeek::analyzer_mgr->GetAnalyzerTag(replaces.c_str()) ) {
	ZEEK_DEBUG(hilti::rt::fmt(" Replaces existing protocol analyzer %s", replaces));
	info.replaces = tag;
	::zeek::analyzer_mgr->DisableAnalyzer(tag);
	}

.

The issue here seems to be that spicy-plugin uses Manager::GetAnalyzerTag which via ComponentManager::GetComponentTag and ComponentManager::Lookup searches ComponentManager::components_by_name. At that point the components have only been added via Plugin::AddComponent (which adds to ComponentManager::components only), but not registered via one of the Component::RegisterComponent which gets triggered by Component::Initialize methods and actually populates components_by_name.

It looks like one fix for this might be to initialize components eagerly, but it is unclear to me what other unintended side effects that can have.

[bbannier]

Another possible fix with probably would be to defer adding of spicy-plugin analyzers to a later point, e.g., in InitPostscript. This has the downside that it might prevent other components seeing spicy-plugin analyzers.

[rsmmr]

I'll move this to spicy-plugin.

[rsmmr]

On closer inspection, I think the problem is a different one: that reproducer attempts to replace packet analyzer with a protocol analyzer. That can't work semantically, but the real problem is that packet analyzers currently do not support replaces anyways: only protocol and file analyzers do. The code says:

::spicy::zeek::compat::AnalyzerTag plugin::Zeek_Spicy::Plugin::tagForPacketAnalyzer(
    const ::spicy::zeek::compat::AnalyzerTag& tag) {
    // Don't have a replacement mechanism currently.
    return tag;
}

I changed the reproducer to use replaces Telnet, and that seems to work fine, the original Telnet analyzer ends up being disabled then:

[Analyzer] Telnet (ANALYZER_TELNET, disabled)

[rsmmr]

On the upside, Zeek just learned to disable packet analyzers, so I believe we should be able to implement this now. zeek/zeek#2443

[rsmmr]

Ok, I can now also reproduce the original problem: the key is to have both (1) spicy-plugin built into Zeek, and (2) load the HLTO through ZEEK_SPICY_MODULE_PATH rather than the command-line. Will look into what's going on.