s3 rule added to monitor potential defacement attempts #563
Conversation
| def rule(event): | ||
|
|
||
| # Quick exit if event contains ignored value | ||
| if event.get("operation") in OPERATIONS: |
There was a problem hiding this comment.
I am not sure this is doing what you want.
>>> OPERATIONS = ["REST.GET.", "REST.HEAD.", "S3.EXPIRE.OBJECT"]
>>> "REST.GET.POLICY_STATUS" in OPERATIONS
False
| @@ -0,0 +1,44 @@ | |||
| # Ignore certain operations, users, user agents and specifc iam roles | |||
| OPERATIONS = ["REST.GET.", "REST.HEAD.", "S3.EXPIRE.OBJECT"] | |||
There was a problem hiding this comment.
I suggest marking these as EXCLUDE_OPERATIONS, etc
There was a problem hiding this comment.
I think that these exclude/include rules aren't behaving as intended. I suggest checking out this detection as a cross-reference.
I believe you'll want to implement both the looping for the constants and to bring in fnmatch to do the matching.
Additionally, this detection should bring a positive and negative test case for each of the early-exit/exclusion checks to demonstrate how those evaluations behave.
|
Thank you for the suggestions, Ed! I am working on improving the logic now. |
|
Let me know your thoughts when you have a moment Ed! Im sure there is a way to simplify this detection but its not immediately obvious to me at the moment. |
|
@edyesed - any suggestions for this rule? |
| if fnmatch(event.get("key"), item): | ||
| return True | ||
|
|
||
| return True |
There was a problem hiding this comment.
default should return False . When the default is True, lines 31-39 are unnecessary processing
| - TA0040:T1491 | ||
| Severity: Low | ||
| Tests: | ||
| - ExpectedResult: true |
There was a problem hiding this comment.
This detection should include test cases that show
- S3 Get operation which does not match the EXCLUDE_* rules -> false
- At least one test case demonstrating that the EXCLUDE_* functions -> false
- a s3 put operation which returns -> true. You'll need to mock the INCLUDED_BUCKET_NAMES object. I'm happy to help with that if you would like 👍
There was a problem hiding this comment.
Hey hey! I made some changes and pushed them. However, it doesn't appear to pass linting:
rules/aws_s3_rules/aws_s3_bucket_defacement_attempt.py:22:0: R1260: 'rule' is too complex. The McCabe rating is 15 (too-complex)
rules/aws_s3_rules/aws_s3_bucket_defacement_attempt.py:22:0: R0911: Too many return statements (7/6) (too-many-return-statements)
rules/aws_s3_rules/aws_s3_bucket_defacement_attempt.py:22:0: R0912: Too many branches (14/12) (too-many-branches)
Are those thresholds arbitrary?
There was a problem hiding this comment.
Those thresholds are intentional ( arbitrary is a word that might be universally applied to any style/lint guidelines ). There's ~1 detection that's disabling these by intention. I can take a look in here and see if there's a way to logically simplify the detection
…ity checks and restructure tests to logical boundaries (#619)
Background
As part of Project Odin, we are looking to provide more out-of-the-box coverage against MITRE ATT&CK. Using Ed's findings in this Asana task, I put together this rule. Please let me know how it could be improved!
Changes
Testing