Skip to content

Commit

Permalink
Fix Privilege Escalation (#108)
Browse files Browse the repository at this point in the history
When assigning roles to groups, the server does not check the role ID
can actually be granted by the user leading to a situation where it's
possible, if you know the role ID, even if not advertised, to add it to
your own group.
  • Loading branch information
spjmurray authored Aug 20, 2024
1 parent eddf49e commit efecaf5
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 2 deletions.
4 changes: 2 additions & 2 deletions charts/identity/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ description: A Helm chart for deploying Unikorn's IdP

type: application

version: v0.2.30
appVersion: v0.2.30
version: v0.2.31
appVersion: v0.2.31

icon: https://raw.githubusercontent.com/unikorn-cloud/assets/main/images/logos/dark-on-light/icon.png

Expand Down
9 changes: 9 additions & 0 deletions pkg/handler/groups/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/unikorn-cloud/identity/pkg/handler/organizations"
"github.com/unikorn-cloud/identity/pkg/middleware/authorization"
"github.com/unikorn-cloud/identity/pkg/openapi"
"github.com/unikorn-cloud/identity/pkg/rbac"

kerrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -141,6 +142,14 @@ func (c *Client) generate(ctx context.Context, organization *organizations.Meta,

return nil, errors.OAuth2ServerError("failed to validate role ID").WithError(err)
}

// Check that the user is allowed to grant the role, this closes a security
// hole where a user can cause privilige escalation by just knowing the
// elevated role ID. As these are typically generated by hashing the name
// guessing them is pretty trivial.
if err := rbac.AllowRole(ctx, &resource, organization.ID); err != nil {
return nil, errors.HTTPForbidden("requested role cannot be granted").WithError(err)
}
}

// TODO: validate groups exist.
Expand Down

0 comments on commit efecaf5

Please sign in to comment.