prevent unwanted retriggering

tomscript commented 4 years ago

Say project foo has an open bucket that SHA detects. SRA is triggered and we close the bucket. Let's say this bucket was actually supposed to be open, the user reopens the bucket then sets the security mark allow_admin_service_account to true so SHA ignores it. Adding the security mark is an update which causes the CSCC notification to trigger again, which calls SRA again and we close the bucket again. We need to propagate the exception security mark to SRA so updates do not cause a new execution. ETD will encounter the same issue if someone modifies a security mark.

Also, I worry about someone modifying an existing finding that was not marked as an exception. Imagine someone adds and removes a bad_ip security mark 5 times. That could cause SRA to generate 5x disk snapshots, or send 5x PagerDuty alerts. In the ETD case we could add a security mark that says "sra-was-here:ignore" then we know not to re-execute. However in the case of SHA the same finding is reused so what do we do? If we add that ignore mark, and later the project re-offends we just ignored it for life!

kieras commented 4 years ago

We need to propagate the exception security mark to SRA @tomscript, is it ok if we add the exception security mark as a configuration in SRA config file? Or do you have another idea?

tomscript commented 4 years ago

we may want to do a quick informal design doc on this. as i sorta describe above there's a few options and non really satisfy all cases. the case where we should respect sha's ignore mark is one but even then i think we can just reuse theirs. so im not sure why it would be configurable in this case when it could just match the one providedin the finding

kieras commented 4 years ago

Yes, I understand. I was just asking about one case... :-) I thought of having a list of security mark's in the config file (to not be hardcoded). If the finding contains any of the security marks in the list, it would be ignored.

Or else, we would have to parse the "ExceptionInstructions" text to find the sec. mark... I think it would be fragile...

tomscript commented 4 years ago

Or else, we would have to parse the "ExceptionInstructions" text to find the sec. mark... I think it would be fragile...

yes it would be. im open to other suggestions. we can also ask the them to add them in a structured field, would be totally fine.

but adding this mark has drawbacks as described above, we would ignore them for life which is probably not intended

kieras commented 4 years ago

It would be great if we could have this information structured. In this case, I'm not talking about adding a security mark yet, I was just thinking about the mechanism to respect the marks if they exists. I need more thought how to handle the other scenarios.

tomscript commented 4 years ago

great, ya maybe do a doc to discuss the how and why a bit. maybe we can extract via regexp for now and once we prove its valuable we can ask for a structured field

On Tue, Jan 7, 2020 at 11:20 AM Kieras notifications@github.com wrote:

It would be great if we could have this information structured. In this case, I'm not talking about adding a security mark yet, I was just thinking about the mechanism to respect the marks if they exists. I need more thought how to handle the other scenarios.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/GoogleCloudPlatform/security-response-automation/issues/159?email_source=notifications&email_token=AAXRKKVQWQ7HCJDGDV6NKK3Q4TIZXA5CNFSM4KCQ7PG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIJ66BQ#issuecomment-571731718, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAXRKKRVC2QTIX2XKZOZPRDQ4TIZXANCNFSM4KCQ7PGQ .

ghost commented 4 years ago

Say project foo has an open bucket that SHA detects. SRA is triggered and we close the bucket. Let's say this bucket was actually supposed to be open, the user reopens the bucket then sets the security mark allow_admin_service_account to true so SHA ignores it. Adding the security mark is an update which causes the CSCC notification to trigger again, which calls SRA again and we close the bucket again. We need to propagate the exception security mark to SRA so updates do not cause a new execution. ETD will encounter the same issue if someone modifies a security mark.

The ExceptionInstructions security mark (allow_admin_service_account in this example) must be added into referred Asset, not the Finding. Given that NotificationConfig filter uses Findings, SRA will not be triggered

Someone modifying an existing finding that was not marked as an exception. Imagine someone adds and removes a bad_ip security mark 5 times. That could cause SRA to generate 5x disk snapshots, or send 5x PagerDuty alerts.

Yes, any update in finding will trigger to SCC Notifications and the post-process relies on the NotificationConfig filter

RATIONALE

After collecting some evidences about ETD/SHA Findings processing, one purpose is to generate a UUID (hash value) based in some attributes from each of the Finding types (SHA, ETD SCC, ETD Legacy). This set of attributes will be chosen considering which of them updates in a true situation to represent a new security occurrence to be remediated. This hash should be generated for each Finding entrance and checked against a possible security mark previously set in the Finding. If the check matches, workflow is skipped.

For example, SHA finding attribute eventTime is always updated due to a scanner detection, but when we update marks in the Finding, eventTime is not. Given that, we should consider name and eventTime attributes to generate the hash value.

For ETD SCC Findings we have another scenario. For each new occurrence, another Finding is created, so we need to check which attributes do include for comparison, excluding eventTime, createdTime, name.

Also, need to define attributes from ETD Legacy (Stackdriver)

WORKFLOW

After every remediation succeeds, this hash value is set in a security mark in the correspondent Finding, such as sra_remediated:<hash-value>. If for some reason this Finding is updated and SRA starts to process it, there will be a validation block before the Router to:

Generate the hash value based on the Finding (SHA, ETD SCC or ETD Legacy)
Check if the processing Finding has the security mark sra_remediated
If it has the security mark, extract its value to check if it's the same as the generated hash value. If true, skip the workflow

SCC Notifications configuration (NotificationConfig) filter also helps in filtering which kind of Finding we want and for example, consider only the ACTIVE ones.

If sounds reasonable, an additional check could be used to help ignoring the Finding: if the Asset has the ExceptionInstructions security mark. This will require another call to SCC API (to collect if the referred Asset has this security mark kind) and extraction of the security mark key value from the ExceptionInstructions fields in Finding SourceProperties.

CAVEATS

Considering we add a state = \"ACTIVE\" value to SCC Notifications filter, SRA will always process and remediate a Finding if the user manually changes an INACTIVE Finding into ACTIVE. Need further checking if we consider this a reasonable scenario in relation to User decisions/permissions.

tomscript commented 4 years ago

must be added into referred Asset,

awesome catch! i definitely thought it had to be attached to the finding. thanks!

so looking at the issues:

how does a user mark a sha finding as exempt

as you described, the security mark is placed on the asset. so SRA will never be re-triggered in this case. we could (if we wanted to) check the asset ourselves to make sure the mark is there. hm, may be a fair amount of effort to do. even just extracting the security mark is sorta a pain right now. although we could ask for that to be structured.

what if someone modifies security marks on an etd/sha finding? each save re-triggers.

if theres no sra_remediated then execute
hash name+eventTime and save as sra_remediated
if it re-triggers ( say someone modified a security mark) hash name+eventTime if it equals sra_remediated then the finding was not re-used bc of new activity (as sha does) but rather someone updated the finding.

do i understand correctly?

great job on the explanation,very clear. LGTM!

ghost commented 4 years ago

if it re-triggers ( say someone modified a security mark) hash name+eventTime if it equals sra_remediated then the finding was not re-used bc of new activity (as sha does) but rather someone updated the finding.

Yes that's right. For new activity only, such as when scanner runs and decides to update the finding, eventTime changes.

we could (if we wanted to) check the asset ourselves to make sure the mark is there. hm, may be a fair amount of effort to do.

Totally agree with that due the pain right now. Will keep an eye on it and use it only if, during implementation, we realize this extra check should be needed. And then we have something to justify asking for the structured field. One scenario to cover by that is the mentioned in CAVEATS section, which is something I guess will depend on user situations, such as:

what's the motivation on changing the state from INACTIVE to ACTIVE? ** Do we need to consider always processing the finding in this case (and then inactivate it again)?
usually the user will have permissions on doing that?

GoogleCloudPlatform / security-response-automation