detect: guard rate filter callback registration

[kenifor]

Ticket: 8560

Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.

Contribution style:

• I have read the contributing guide lines at
  https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

• I have signed the Open Information Security Foundation contribution agreement at
  https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

• I have updated the User Guide (in doc/userguide/) to reflect the changes made
• I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
  (including schema descriptions)
• I have created a ticket at
  https://redmine.openinfosecfoundation.org/projects/suricata/issues

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

Describe changes:

• Guard SCDetectEngineRegisterRateFilterCallback() against a NULL return from DetectEngineGetCurrent().
• Avoid a NULL pointer dereference when this public extension API is called while no suitable current detect engine is available.
• This API is documented in the devguide and used by the bundled library examples, so callers should not be able to crash Suricata due to a missing internal detect-engine context.

Provide values to any of the below to override the defaults.

• To use a Suricata-Verify or Suricata-Update pull request,
  link to the pull request in the respective _BRANCH variable.
• Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=

[victorjulien]

I'm really curious how we would get here with a NULL de_ctx. The only call site:

            if (de_ctx->RateFilterCallback && original_action != pa->action) {
                pa->action = de_ctx->RateFilterCallback(p, sid, gid, rev, original_action,
                        pa->action, de_ctx->rate_filter_callback_arg);

So there a detect engine when this is called. It's also a full engine as this is only called on a signature match AFAICS.

[catenacyber]

It is also called in plugins see

examples/lib/custom/main.c:    SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);
examples/lib/live/main.c:    SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);

cc @jasonish

[jasonish]

Is this actually a scenario a plugin could end up in?

[jasonish]

And if it can, where should a plugin attempt to reregister these callbacks again?

[kenifor]

Yes. SCDetectEngineRegisterRateFilterCallback() is a documented public
extension API. A plugin author following the bundled examples
(examples/lib/custom/main.c, examples/lib/live/main.c) will naturally
call it during plugin/library initialization — before DetectEngineCtxInit()
has run and registered an engine as current. In that window,
DetectEngineGetCurrent() returns NULL and the original code dereferences it
unconditionally. In multi-tenant or delayed-detect configurations the window
is even wider.

The runtime call-site that @victorjulien points to (inside the signature-match
path) is indeed safe, because there a live engine is guaranteed. But this
function is also a registration entry-point called by external code that has
no visibility into the engine lifecycle — that is the path Svace flags.

Where should a plugin reregister?

That is a fair follow-up. The current fix (guard + log + early return) prevents
the crash, but since the function is void the caller has no way to detect
that registration was silently dropped.

Two options, in order of invasiveness:

1. Change the return type to bool/int so the caller knows the
   registration failed and can retry after the engine is ready. This is a
   small API change but requires updating the devguide and the two bundled
   examples.

2. Keep void, document the constraint — add a note to the devguide
   stating that this API must be called after the detect engine is initialized.
   The guard then acts purely as a defensive belt-and-suspenders measure.