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

[suricata] don't modify shared mutable param lists #4723

Closed
joegallo opened this issue Nov 28, 2022 · 2 comments · Fixed by #4731
Closed

[suricata] don't modify shared mutable param lists #4723

joegallo opened this issue Nov 28, 2022 · 2 comments · Fixed by #4731
Labels
bug Something isn't working, use only for issues Integration:suricata Suricata

Comments

@joegallo
Copy link
Contributor

This code leaks a shared mutable reference:

In this context, k is something like type and the associated v is an ArrayList straight out of the params object. But the params object is 'static' (allocated once per script processor, not once per script processor execution) so it's a shared object. Later on in the same pipeline, there are some append processors that operate on the resulting 'type', like this one:

- append:
field: event.type
value: "{{{suricata.eve.alert.action}}}"
if: "ctx?.suricata?.eve?.alert?.action != null"

Which means that if multiple Elasticsearch indexing threads are pushing documents through this pipeline simultaneously, then it's possible (albeit rare!) for one of them to throw a ConcurrentModificationException. As a very subtle point, note that (arguably) it's acceptable to leak the shared reference as long as there is no later modification of the leaked list.

Here's a commit that I think will fix the issue, I tried to model it off the similarly-purposed code in #3492:

main...joegallo:integrations:suricata-copy-param-lists

elastic/elasticsearch#91977 is the upstream issue in Elasticsearch -- it shouldn't be necessary to make a defensive copy like this, but at least in the context of the current behavior of Elasticsearch, the defensive copy is necessary.

/cc @andrewkroh @P1llus @masseyke

@joegallo joegallo added bug Something isn't working, use only for issues Integration:suricata Suricata labels Nov 28, 2022
@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6
Copy link
Contributor

efd6 commented Nov 29, 2022

This defensive copy has been done elsewhere that this pattern exists (crowdstrike).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working, use only for issues Integration:suricata Suricata
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants