turbot / steampipe-mod-aws-compliance

Run individual controls or full compliance benchmarks for CIS, PCI, NIST, HIPAA and more across all of your AWS accounts using Powerpipe and Steampipe.
https://hub.steampipe.io/mods/turbot/aws_compliance
Apache License 2.0
372 stars 63 forks source link

Inconsistency in CIS 4.1 #272

Closed ecktom closed 3 years ago

ecktom commented 3 years ago

I just had look at https://github.com/turbot/steampipe-mod-aws-compliance/blob/main/query/cloudwatch/log_metric_filter_unauthorized_api.sql. It looks like there are some issues with this query.

1.

and filter.filter_pattern ~ 'errorCode\s*=\s*"\*{0,1}UnauthorizedOperation\*{0,1}"'
and filter.filter_pattern ~ 'errorCode\s*=\s*"\*{0,1}AccessDenied\*{0,1}"'

does not match the expected filter which is

"Filter = {(($.errorCode="*UnauthorizedOperation") || ($.errorCode="AccessDenied*")) && (($.sourceIPAddress!="delivery.logs.amazonaws.com") && ($.eventName!="HeadBucket"))}"

shouldn't it be the following (note the difference at *{0,1}) ?

and filter.filter_pattern ~ 'errorCode\s*=\s*"\*{0,1}UnauthorizedOperation"'
and filter.filter_pattern ~ 'errorCode\s*=\s*"AccessDenied\*{0,1}"'

2. Is there any reason for having the second part of the rule (($.sourceIPAddress!="delivery.logs.amazonaws.com") && ($.eventName!="HeadBucket")) commented out?

e-gineer commented 3 years ago

Thanks very much @ecktom for finding and raising this issue! It gets a bit hairy in those regular expressions :-)

Is that expected filter from AWS? I assume yes, but am wondering about the source?

Also, I wonder why we are making it optional for the * in this case instead of required? The logic seems different in that particular rule set as well.

@rajlearner17 Seems like more review is required here to check on exactly what we should be matching?

ecktom commented 3 years ago

Hi @e-gineer. I just took the expected Filter from CIS Amazon Web Services Foundations Benchmark - v1.4.0 - 05-28-2021 (should have noted that I guess ;))

I think it was made optional to also align with v1.3.0 which did not contain the * as both benchmarks in this repo are using that query, right?

rajlearner17 commented 3 years ago

@ecktom thanks for raising it, checking the same.

rajlearner17 commented 3 years ago

@ecktom thanks that an excellent observation, I think the commented lines should not be there. Unsure what might have triggered to comment at the last moment. We will uncomment and cross-check.

On the first pattern, CIS changed the pattern to add *, which is addressed by our current pattern as there could be * either 0 or 1 time. Again adding the same *{0,1} at the end was not required. Overall your recommendation looks good to be very specific as mentioned. We will cross-check these patterns in case we missed out on something in lines and fix them soon.

We are using the same query for cis 1.3 as well, new pattern should not break. We will keep an eye on these.

image

rajlearner17 commented 3 years ago

@ecktom tracking this again as nearing release, while I was investigating last week, found the filter pattern same for both CIS 1.3 and 1.4. FYI as explained below, do correct me in case missing to interpret.

The pattern in 1.3.0 (In Audit steps) "Filter = {(($.errorCode="UnauthorizedOperation") || ($.errorCode="AccessDenied")) ||(($.sourceIPAddress!="delivery.logs.amazonaws.com") || ($.eventName!="HeadBucket"))}"

Implemented as (Refer Remediation) '{ ($.errorCode = "*UnauthorizedOperation") || ($.errorCode = "AccessDenied*")($.sourceIPAddress!="delivery.logs.amazonaws.com") || ($.eventName!="HeadBucket") }'

The pattern in 1.4.0 (In Remediation steps)

'{($.errorCode = "*UnauthorizedOperation") || ($.errorCode = "AccessDenied*")|| 
 ($.sourceIPAddress!="delivery.logs.amazonaws.com") || ($.eventName!="HeadBucket") }'

We will make applicable changes as we missed out #2.