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.

[jasonish]

I think the only time this could be a problem is if a plugin/library application registered this, called when detection is disabled. This function should probably return a bool to indicator failure to register, but more importantly, the plugin examples should probably be updated to be something like:

-    SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);
+    if (DetectEngineEnabled()) {
+        SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);
+    }

[kenifor]

I think the only time this could be a problem is if a plugin/library application registered this, called when detection is disabled. This function should probably return a bool to indicator failure to register, but more importantly, the plugin examples should probably be updated to be something like:

-    SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);
+    if (DetectEngineEnabled()) {
+        SCDetectEngineRegisterRateFilterCallback(RateFilterCallback, NULL);
+    }

Updated in cbce462:

• Changed return type to bool as suggested — callers now know if registration was dropped.
• Wrapped the calls in both bundled examples (examples/lib/custom/main.c, examples/lib/live/main.c) with DetectEngineEnabled().

[jasonish]

I would look a warning here, and in the other plugin as well on failure, just for completeness of the examples.

[jasonish]

One more comment made here..

Please follow our contribution guidelines with respect to pushing changes and followup pull requests.

In your followup pull request, either do one clean commit. Or one with the change minus the example usage, and then a second commit with the examples updated.

[victorjulien]

For reference, this is also flagged by gcc (using 16) with the -fanalyzer option:

detect-engine.c: In function ‘SCDetectEngineRegisterRateFilterCallback’:
detect-engine.c:5209:32: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference]
 5209 |     de_ctx->RateFilterCallback = fn;
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
  ‘SCDetectEngineRegisterRateFilterCallback’: events 1-2
    │
    │ 5206 | void SCDetectEngineRegisterRateFilterCallback(SCDetectRateFilterFunc fn, void *arg)
    │      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │      |      |
    │      |      (1) entry to ‘SCDetectEngineRegisterRateFilterCallback’
    │ 5207 | {
    │ 5208 |     DetectEngineCtx *de_ctx = DetectEngineGetCurrent();
    │      |                               ~~~~~~~~~~~~~~~~~~~~~~~~
    │      |                               |
    │      |                               (2) calling ‘DetectEngineGetCurrent’ from ‘SCDetectEngineRegisterRateFilterCallback’
    │
    └──> ‘DetectEngineGetCurrent’: event 3
           │
           │ 3943 | DetectEngineCtx *DetectEngineGetCurrent(void)
           │      |                  ^~~~~~~~~~~~~~~~~~~~~~
           │      |                  |
           │      |                  (3) entry to ‘DetectEngineGetCurrent’
           │
         ‘DetectEngineGetCurrent’: event 4
           │
           │ 3963 |     return NULL;
           │      |            ^~~~
           │      |            |
           │      |            (4) using NULL here
           │
    <──────┘
    │
  ‘SCDetectEngineRegisterRateFilterCallback’: events 5-7
    │
    │ 5208 |     DetectEngineCtx *de_ctx = DetectEngineGetCurrent();
    │      |                               ^~~~~~~~~~~~~~~~~~~~~~~~
    │      |                               |
    │      |                               (5) returning to ‘SCDetectEngineRegisterRateFilterCallback’ from ‘DetectEngineGetCurrent’
    │ 5209 |     de_ctx->RateFilterCallback = fn;
    │      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │      |           |                    |
    │      |           (6) ‘de_ctx’ is NULL (7) ⚠️  dereference of NULL ‘de_ctx’
    │

@kenifor could you submit a new PR?

[catenacyber]

Replaced by #15553