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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Detections] Updating prepackaged rules overwrites existing exceptions #80417

Closed
spong opened this issue Oct 13, 2020 · 4 comments
Closed

[Security Solution][Detections] Updating prepackaged rules overwrites existing exceptions #80417

spong opened this issue Oct 13, 2020 · 4 comments
Assignees
@FrankHassanabad
Labels
bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine fixed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0

Comments

@spong
Copy link
Member

spong commented Oct 13, 2020 •
edited
Loading

Kibana version:
7.9

When updating prepackaged rules any existing exceptions on the rules being updated will be overridden with whatever exceptions are on the prepackaged rules. To mitigate this, we can merge any existing exceptions_list items from the rulesFromFileSystem with those coming from the prepackagedRules.

Flow:

Get rules to update:

kibana/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.ts

Line 65 in 21156d6

const rulesToUpdate = getRulesToUpdate(rulesFromFileSystem, prepackagedRules);

Add logic for merging existing exceptions

kibana/x-pack/plugins/security_solution/server/lib/detection_engine/routes/rules/add_prepackaged_rules_route.ts

Line 87 in 21156d6

await updatePrepackagedRules(alertsClient, savedObjectsClient, rulesToUpdate, signalsIndex);

Patch in update_prepacked_rules.ts will now include any pre-existing exceptions and will no longer overwrite:

kibana/x-pack/plugins/security_solution/server/lib/detection_engine/rules/update_prepacked_rules.ts

Line 63 in d6c7128

exceptions_list: exceptionsList,

Steps to recreate:

  • Add prepackaged rules
  • Add exception to prepackaged rule
  • Increment version of the rule that the exception was added to
  • Update prepackaged rules
  • Observe exception added is no longer present
@spong spong added bug Fixes for quality problems that affect the customer experience Team:SIEM v7.10.0 Feature:Detection Rules Security Solution rules and Detection Engine Team:Detections and Resp Security Detection Response Team labels Oct 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@FrankHassanabad FrankHassanabad mentioned this issue Oct 14, 2020
Merged
1 task
FrankHassanabad added a commit that referenced this issue Oct 15, 2020
@FrankHassanabad
[Security Solutions][Detection Engine] Fixes pre-packaged rules which…
e1aec17 
… contain exception lists to not overwrite user defined lists (#80592)

## Summary

Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417

* Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost.
* Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists
* Refactored some of the code in the `get_rules_to_update.ts`
* Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests
* Added unit tests to exercise the bug and then the fix.
* Added integration tests that fail this logic and then fixed the logic

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this issue Oct 15, 2020
@FrankHassanabad
[Security Solutions][Detection Engine] Fixes pre-packaged rules which…
d18854d 
… contain exception lists to not overwrite user defined lists (elastic#80592)

## Summary

Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: elastic#80417

* Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost.
* Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists
* Refactored some of the code in the `get_rules_to_update.ts`
* Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests
* Added unit tests to exercise the bug and then the fix.
* Added integration tests that fail this logic and then fixed the logic

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security Solution)

FrankHassanabad added a commit that referenced this issue Oct 15, 2020
@FrankHassanabad
[Security Solutions][Detection Engine] Fixes pre-packaged rules which…
733e5b7 
… contain exception lists to not overwrite user defined lists (#80592) (#80734)

## Summary

Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417

* Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost.
* Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists
* Refactored some of the code in the `get_rules_to_update.ts`
* Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests
* Added unit tests to exercise the bug and then the fix.
* Added integration tests that fail this logic and then fixed the logic

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
FrankHassanabad added a commit that referenced this issue Oct 15, 2020
@FrankHassanabad
[Security Solutions][Detection Engine] Fixes pre-packaged rules which…
08009fb 
… contain exception lists to not overwrite user defined lists (#80592) (#80733)

## Summary

Fixes a bug where when you update your pre-packaged rules you could end up removing any existing exception lists the user might have already added. See: #80417

* Fixes the merge logic so that any exception lists from pre-packaged rules will be additive if they do not already exist on the rule. User based exception lists will not be lost.
* Added new backend integration tests for exception lists that did not exist before including ones that test the functionality of exception lists
* Refactored some of the code in the `get_rules_to_update.ts`
* Refactored some of the integration tests to use helper utils of `countDownES`, and `countDownTest` which simplify the retry logic within the integration tests
* Added unit tests to exercise the bug and then the fix.
* Added integration tests that fail this logic and then fixed the logic

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@peluja1012 peluja1012 added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Oct 28, 2020
@MadameSheema
Copy link
Member

Tested and working on 7.10BC4

@ghost
Copy link

ghost commented Nov 25, 2020

Bug Conversion:

This ticket requires Dev validation

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@FrankHassanabad FrankHassanabad

Labels
bug Fixes for quality problems that affect the customer experience Feature:Detection Rules Security Solution rules and Detection Engine fixed impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@peluja1012 @FrankHassanabad @MindyRS @spong @elasticmachine @MadameSheema