search

nccgroup / PMapper

A tool for quickly evaluating IAM permissions in AWS.
GNU Affero General Public License v3.0
1.37k stars 169 forks source link

[WIP] Check for single node sts:AssumeRole cycles #113

Closed RyanJarv closed 2 years ago

RyanJarv commented 2 years ago

When a role attempts to assume it self an explicit allow in the trust policy is not strictly necessary for access. An explicit allow in the identity policy is sufficient.

This applies to the circular access check because role revocations do not effect very recently refreshed credentials. When a role can assume it self, a bash loop calling aws sts assume-role and exporting the new credentials to the environment will bypass a revocation triggered through the web UI.

I still need to add tests, etc.. just wanted to open this in the mean time in case anyone has any thoughts on this.

Edit: Ok, just read the contributing section, I'll need to change the base and open an issue as well.

ncc-erik-steringer commented 2 years ago

In general we do not track edges where the source and destination Node are the same, which helps simplify other algorithms and eliminates redundancy for identifying effective permissions.

However, I think this would be a great idea for a new preset query. Right now the only real persistence check we have is for 2+ Node objects with edges that create a cycle between them (circular access finding from analysis):

https://github.com/nccgroup/PMapper/blob/91d2e60102bdadf346d77b60d90ddaa4a678f037/principalmapper/analysis/find_risks.py#L364

It might be nice to create a persistence preset query that goes through one or more Node objects in a Graph to identify things like when someone can reassume a role (does this drop session policies?) or chains of Edge that go back to the source Node, etc.

RyanJarv commented 2 years ago

Ok let me think about this for a bit.

Another thought I had, to avoid tracking these kind of edges, is to redo the policy checking in the circular access finding.

It might be slightly redundant, but at least it would be O(n) since you only need to check against the current node.

RyanJarv commented 2 years ago

Sorry for the delay on this. I do plan on plan on addressing your concerns as I'd like to get this merged.

I am a bit busy at the moment though, so if you'd like to close it for now just to clean up outstanding PR's I can reopen it when I've dug into this more.