Async multi-schema execution may store empty {} events in temp DB due to missing guard in save_to_temp_events_only

[Gauravsharma2040]

Issue Description
Summary

While reviewing behavior observed during development of a multi-step module (CVE-2024-36401), an inconsistency was identified in the Nettacker engine event pipeline that may allow empty {} responses to be written to the temporary events database.
The async behavior itself was originally observed by @sankalp-b1401 when testing the module with multiple schemas (HTTP/HTTPS). While investigating this further, I traced the root cause to a missing guard in the engine's save_to_temp_events_only event write path.

Observed Behavior
Nettacker uses the asynchronous aiohttp library to execute module payloads concurrently.
When modules define multiple schemas (for example http and https) across multiple ports (e.g. 80, 443, 8080), requests are executed concurrently and there is no guaranteed ordering of responses.
In some cases one request may produce an empty match result while another produces a valid detection result. During this process, the engine may write an empty {} event to the temporary events database.
This behavior was initially observed while debugging a multi-step module that relies on temporary event storage.

Root Cause
The issue appears to originate in:
core/lib/base.py
Specifically in the logic responsible for writing temporary events:
save_to_temp_events_only
In this code path there is currently no guard preventing empty conditions_results from being written.
However, in the normal detection event write path, there is already a guard that prevents empty condition results from being persisted.
This results in inconsistent behavior between:
normal event writes
temporary event writes

Proposed Fix
Add a guard in the save_to_temp_events_only logic similar to the one already used in the normal detection event write path.
For example extending the condition to check:
event["response"]["conditions_results"]
before writing to the temporary event database.
This change would prevent {} responses from being stored and align the behavior of temporary event writes with the existing detection event logic.

Acknowledgement
The async execution behavior leading to {} events was initially observed by @sankalp-b1401 b1401 while testing the module; this issue documents the underlying engine condition that allows the empty events to be written.

[sankalp-b1401]

Yup, that can be done, but in my opinion an easier fix would be to check the fetched value at the time of querying the database.

dependent_on_temp_event can check the fetched value to see if it's empty {} or not. If {}, query the next index, do so until you have queried the entire list. And stop immediately if you find a non-empty value or if the list ends.

[Aarush289]

I think , this Is not an issue , writing the empty conditions to the database is intentional as far as I know because there will be an event which will be dependent the response of that "save_to_temp_event" and will wait forever incase conditions for the required event is not stored in temp database . Please correct me if I misunderstood your concern , I genuinely didn't get what are the consequences of writing empty conditions to the temp database?

[Gauravsharma2040]

Thanks for the clarification. I understand the reasoning that storing the event (even with empty conditions) ensures that dependent modules do not block indefinitely while waiting for a result.

My concern was mainly around multi-step modules that rely on the contents of conditions_results when reading from the temporary events database. If {} responses are written, a dependent step may interpret that entry as a valid event rather than a “no match” state, which could lead to incorrect branching or detection logic in modules that expect a populated result.
In practice this could affect multi-step workflows where a later step checks the stored response to decide whether to continue the detection chain. Encountering an empty {} event may require additional filtering logic in each module.
The idea behind the guard I suggested was mainly to keep the event pipeline consistent with the normal detection write path and avoid storing empty results unless they are explicitly needed for dependency tracking. From a data integrity perspective it may also make the temporary events store cleaner and easier to reason about.
That said, if the current behavior is intentional for dependency resolution, Sankalp’s suggestion of filtering {} values during dependent_on_temp_event queries would also address the module-side issue without modifying the write path.

[sankalp-b1401]

@Aarush289 I also think that {} should be allowed in save_to_temp_event.

I faced this issue when I was writing the module for CVE-2024-36401 (PR #1341).

The CVE required a sequence of payload. The first payload fetched a valid typeName which was used by the second payload to send the exploit to the server.

CVE Logic:

• it requires two payloads.
• first payload is a legitimate request that GET a valid value of typeName
• second payload uses this value of typeName to sends the actual exploit to the server

So, I leveraged Nettacker's save_to_temp_only and dependent_on_temp_event features. Payload 1 saves the value to db using save_to_temp_only and payload 2 fetches the value from the db using dependent_on_temp_event.

The issue came in the first payload. I initially used http and https as default values in the schema list. As expected two requests are sent to during the execution of first payload, one with http and other with https. Now my test server was hosted on http only, so the response corresponding to it returned legitimate values but the https request returned {}. Both values are saved in the same db, but the second payload would only fetch the value that's saved at the last. Due to Nettacker's asynchronous behaviour, we do not know which request would execute last.

Scenario 1: http request executes last, I get a legitimate value and second payload executes and everything works well.

Scenario 2: https request executes last, and i get a false negative for a vulnerable server.

So, if you'll look at my current PR, I've repeated the entire exploit separately for http and https. However it's a temporary fix and we have to eventually update the logic for a generalized solution.

This is not just limited to http/https, at any point where the requests branch out it'll happen, it can be due to multiple values of schema/port/data.

[Aarush289]

I understood the issue now , and I feel it's legit too . I don't know the best way to fix this but maybe we can implement it in such a way that it first checks whether conditions corresponding to the event are saved or not , incase it's not then we can just store it irrespective of whether it is empty or not and if not saved then we will over write it only if the conditions we have are not empty .

[sankalp-b1401]

I think only if we save_to_temp_only, then the data is saved to db at any intermediary stage otherwise it isn't anyways.

I am not sure if I understood your idea correctly, could you please elaborate.

[Aarush289]

I mean to say , that eventually for a single step of a module we need to prevent any substep with empty conditions to over write the non empty conditions from any other substep of same step , so we may follow what we said earlier by first checking whether some other substep of same step has stored the conditions or not and then proceed accordingly.

[Aarush289]

So I was thinking about it and I got a better and reliable way by iterating through all the saved data for a particular event and then run the dependent step by replacing the corresponding string with each of the stored data one by one . The main issue is that it is using the [0] which means it will only use one of the substep's conditions while ignoring others without knowing which one is useful and which ones are not . Actually till now in most of the modules using save_to_temp_events they have used only 1 port and 1 url so that only One substep is generated for the step and there is no such confusion.

[sankalp-b1401]

Yup that's what I was proposing as well. We iterate and stop when we find first non-empty value or the list ends.

[Aarush289]

Yeah , Make sense but if we just need one non empty entity then isn't it better to prevent writing multiple entities to the temp events and just restrict it to write only one entity per step which means a substep having empty conditions can first query temp database whether any entity with same event name already exists, if yes then it doesn't do anything otherwise it will write to database and if the substep is having non empty conditions then delete what is stored in database and store the current conditions , at the end this will ensure that if there is any substep with non empty conditions then we will be having non empty conditions in the database for the whole step which is needed , and thus we can skip looping through all entities to find non empty entity and will always have desired result in the database.
Sounds good ??

[Gauravsharma2040]

That's an interesting approach and it would definitely simplify the read side since the dependent step would only need to fetch a single value.
One concern I have is that this assumes a step should only ever produce one meaningful result. In cases where branching happens (multiple schemas, ports, payload variations, etc.), it's possible for multiple substeps to legitimately produce different non-empty results. With the overwrite logic we might end up discarding valid values depending on execution order.
Since the requests are async, we could still end up with race-dependent outcomes where the final stored value depends on which substep finishes last.
Because of that, it might be safer to allow storing multiple results and handle the selection logic when resolving dependent_on_temp_event. Iterating through the stored values and executing the dependent step for each non-empty result would preserve all valid outcomes and avoid ordering issues.

That would also make the behavior consistent even when modules branch across multiple schemas/ports/data combinations.

[Aarush289]

Yeah , that's fine but I proposed the approach in continuation of sankalp's approach to stop when we get the first non empty , iterating through all is the safest approach but I guess if we just use any of the non empty conditions then it will cover 99% cases and I don't know whether overhead of iterating and adding too much complexity worth considering. May be we can wait for maintainer's insights over the issue .

[Gauravsharma2040]

I did think about the potential overhead of querying and checking multiple stored results when I initially saw sankalp's approach but. However, in practice it likely wouldn’t amount to much since dependent_on_temp_event is only used by a relatively small subset of modules, and even then the number of stored results for a single step should generally remain quite small.

Because of that, the additional iteration or filtering step probably wouldn’t have a significant impact on performance in most real cases. That said, your point about complexity vs benefit is valid as well.

It might be best to wait for the maintainer’s perspective here to see which direction aligns better with the intended design of the event pipeline.