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

Dependent actions allow broader permissions with wildcard #382

Open
knastase opened this issue Dec 29, 2021 · 2 comments
Open

Dependent actions allow broader permissions with wildcard #382

knastase opened this issue Dec 29, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@knastase
Copy link
Contributor

knastase commented Dec 29, 2021

I am not sure if this was a conscious design decision or not. Dependent actions are always added with wildcards (*) and may not be the expected behavior.

Example

When generating a policy document for write actions on a KMS key ARN, the dependent actions are added as wildcards which could lead to granting more permissions than anticipated.

kms:ReplicateKey is a write action that is added when I specify a key ARN (as designed)
Two dependent actions of it are kms:CreateKey and kms:PutKeyPolicy

I understand kms:CreateKey has no resource requirements and is added as a wildcard. kms:PutKeyPolicy on the other hand CAN take a key ARN as a resource constraint but is indiscriminately added as a wildcard action as well. This leads to the generated policy being able to put a key policy on any KMS key (if a key has the default key policy or an overly permissive one)

Observed behavior

- Sid: MultMultNone
  Effect: Allow
  Action:
  - kms:CreateKey
  - kms:PutKeyPolicy <--
  - kms:TagResource
  Resource:
  - '*' <--

Expected behavior

- Sid: KmsWriteKey
  Effect: Allow
  Action:
  ...
  - kms:Verify
  - kms:PutKeyPolicy <--
  - kms:TagResource
  Resource:
  - arn:aws:kms:us-east-1:123456789012:key/shaq <--
- Sid: MultMultNone
  Effect: Allow
  Action:
  - kms:CreateKey
  Resource:
  - '*'

Possible remediation

I attempted to add a bit of code to writing/sid_group.py to add the dependent action to the same SID as the other actions if they share the same resource constraint (but part of a different access_level).

if len(dependent_actions) > 0:
  for dep_action in dependent_actions:
      # If a dependent action has the same resource constraint, add it to actions 
      # instead of * which could grant more access than anticipated.
      for level in ["Read", "Write", "List", "Tagging", "Permissions management"]:
          if dep_action in get_actions_with_arn_type_and_access_level(
              service_prefix, resource_type_name, level):
                if not dep_action in actions:
                  actions.append(dep_action)
      if not dep_action in actions:
          self.add_action_without_resource_constraint(dep_action)
@knastase knastase changed the title Dependent actions allow broader permissions with wildcard Dependant actions allow broader permissions with wildcard Dec 29, 2021
@knastase knastase changed the title Dependant actions allow broader permissions with wildcard Dependent actions allow broader permissions with wildcard Dec 29, 2021
@kmcquade
Copy link
Collaborator

@knasty51 - right on! Would you mind sending that remediation in as a PR?

thanks so much for all of your contributions recently - you rock!

@knastase
Copy link
Contributor Author

knastase commented Feb 6, 2022

1st attempt - #395

@kmcquade kmcquade added the bug Something isn't working label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants