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

[Bug] Query validation failing to capture InSet edge case with ip field types #3572

Conversation

eric-forte-elastic
Copy link
Contributor

@eric-forte-elastic eric-forte-elastic commented Apr 4, 2024

Issues

#3540

Summary

This PR addresses an issue where the eql library's inSet method does not catch a type comparison mismatch that will cause an error in Kibana over the "IP" type (see the issue for more detail). This PR wraps the LarkToEQL.in_set method with a custom in set method that supports this comparison and adds an Extended Type hint enum to provide the IP type.

Thanks @Mikaayenson for the great patches and discussion in the issue! 🙇

Testing

Modify a rule such as rules/windows/execution_scheduled_task_powershell_source.toml that has destination.address to instead read destination.ip. After doing this run view rule on this modified rule. Originally, this would not result in an error. This PR addresses the issue if this does result in an error.

Added unit test should also look to catch this.

detection_rules/ecs.py Outdated Show resolved Hide resolved
@eric-forte-elastic eric-forte-elastic self-assigned this Apr 4, 2024
@eric-forte-elastic eric-forte-elastic added bug Something isn't working python Internal python for the repository Area: DED Team: TRADE labels Apr 4, 2024
@Mikaayenson
Copy link
Contributor

It will be good to double check if this is an issue with just up types or the inSet function. I thought the original issue was that the field type have to match the value types in the set. If this is the case, the solution may be less about IPs and more about matching types.

@botelastic botelastic bot added the schema label Apr 4, 2024
Copy link
Contributor

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eric-forte-elastic can you add a unit test to demonstrate when this should / should not fail?

Copy link
Contributor

@Mikaayenson Mikaayenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

tests/test_specific_rules.py Outdated Show resolved Hide resolved
@shashank-elastic
Copy link
Contributor

🟢 Manual Test Looks Good.

  • Modified the sample rule for testing
    image
  • Ran view-rule which resulted in error
█▀▀▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄▄▄ ▄   ▄      █▀▀▄ ▄  ▄ ▄   ▄▄▄ ▄▄▄
█  █ █▄▄  █  █▄▄ █    █   █  █ █ █▀▄ █      █▄▄▀ █  █ █   █▄▄ █▄▄
█▄▄▀ █▄▄  █  █▄▄ █▄▄  █  ▄█▄ █▄█ █ ▀▄█      █ ▀▄ █▄▄█ █▄▄ █▄▄ ▄▄█

Error loading rule in /Users/shashankks/elastic_workspace/detection-rules/rules/windows/execution_scheduled_task_powershell_source.toml
Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/__main__.py", line 34, in <module>
    main()
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/__main__.py", line 31, in main
    root(prog_name="detection_rules")
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/core.py", line 1157, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/core.py", line 1078, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/core.py", line 1688, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/core.py", line 1434, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/core.py", line 783, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/click/decorators.py", line 33, in new_func
    return f(get_current_context(), *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/main.py", line 228, in view_rule
    rule = RuleCollection().load_file(rule_file)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/rule_loader.py", line 276, in load_file
    return self.load_dict(obj, path=path)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/rule_loader.py", line 253, in load_dict
    contents = TOMLRuleContents.from_dict(obj)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/mixins.py", line 109, in from_dict
    return schema.load(obj)
           ^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/marshmallow_dataclass/__init__.py", line 910, in load
    all_loaded = super().load(data, many=many, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/marshmallow/schema.py", line 722, in load
    return self._do_load(
           ^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/marshmallow/schema.py", line 884, in _do_load
    self._invoke_schema_validators(
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/marshmallow/schema.py", line 1185, in _invoke_schema_validators
    self._run_validator(
  File "/Users/shashankks/elastic_workspace/detection-rules/.venv/lib/python3.12/site-packages/marshmallow/schema.py", line 774, in _run_validator
    validator_func(output, partial=partial, many=many)
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/rule.py", line 1193, in post_conversion_validation
    data.validate_query(metadata)
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/rule.py", line 646, in validate_query
    return validator.validate(self, meta)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/shashankks/elastic_workspace/detection-rules/detection_rules/rule_validators.py", line 318, in validate
    raise ValueError(f"Error in both stack and integrations checks: {validation_checks}")
ValueError: Error in both stack and integrations checks: {'stack': EqlTypeMismatchError('Error at line:4,column:170\nUnable to compare ip to string\n [any where host.os.type == "windows" and (event.category == "library" or (event.category == "process" and event.action : "Image loaded*")) and\n  (?dll.name : "taskschd.dll" or file.name : "taskschd.dll") and process.name : ("powershell.exe", "pwsh.exe", "powershell_ise.exe")]\n [network where host.os.type == "windows" and process.name : ("powershell.exe", "pwsh.exe", "powershell_ise.exe") and destination.port == 135 and not destination.ip in ("127.0.0.1", "::1")]\n                                                                                                                                                                         ^^^^^^^^^^^\nstack: 8.9.0, beats: 8.9.0,ecs: 8.9.0, endgame: 8.4.0'), 'integrations': EqlTypeMismatchError('Error at line:4,column:170\nUnable to compare ip to string\n [any where host.os.type == "windows" and (event.category == "library" or (event.category == "process" and event.action : "Image loaded*")) and\n  (?dll.name : "taskschd.dll" or file.name : "taskschd.dll") and process.name : ("powershell.exe", "pwsh.exe", "powershell_ise.exe")]\n [network where host.os.type == "windows" and process.name : ("powershell.exe", "pwsh.exe", "powershell_ise.exe") and destination.port == 135 and not destination.ip in ("127.0.0.1", "::1")]\n                                                                                                                                                                         ^^^^^^^^^^^\nstack: 8.9.0, integration: None,ecs: 8.9.0, package: endpoint, package_version: 8.13.0')}

@eric-forte-elastic eric-forte-elastic merged commit a4a0bc6 into main May 6, 2024
14 checks passed
@eric-forte-elastic eric-forte-elastic deleted the 3540-bug-query-validation-failing-to-capture-inset-edge-case-with-ip-field-types branch May 6, 2024 11:58
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
protectionsmachine pushed a commit that referenced this pull request May 6, 2024
…ld types (#3572)

* Move test case to separate file

---------

Co-authored-by: Mika Ayenson <Mikaayenson@users.noreply.github.com>
Co-authored-by: shashank-elastic <91139415+shashank-elastic@users.noreply.github.com>

(cherry picked from commit a4a0bc6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport: auto bug Something isn't working python Internal python for the repository schema Team: TRADE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Query validation failing to capture InSet edge case with ip field types
3 participants