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

fix: user privilege escalation bug #13976

Merged
merged 1 commit into from Dec 23, 2021

Conversation

donatello
Copy link
Member

@donatello donatello commented Dec 22, 2021

Description

The user create API endpoint was accepting a policy field. This API is used to
update a user's secret key and account status, and allows a regular user to
update their own secret key. The policy update is also applied though does not
appear to be used by any existing client side functionality.

This fix changes the accepted request body type and removes the ability to apply
policy changes as that is possible via the policy set API.

Motivation and Context

Found while reviewing code working on something else.

How to test this PR?

Tests included.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Documentation updated
  • Unit tests added/updated

@donatello
Copy link
Member Author

Depends on minio/madmin-go#56

go.sum Outdated Show resolved Hide resolved
The user create API endpoint was accepting a policy field. This API is used to
update a user's secret key and account status, and allows a regular user to
update their own secret key. The policy update is also applied though does not
appear to be used by any existing client side functionality.

This fix changes the accepted request body type and removes the ability to apply
policy changes as that is possible via the policy set API.
@minio-trusted
Copy link
Contributor

Mint Automation

Test Result
mint-large-bucket.sh ✔️
mint-fs.sh ✔️
mint-gateway-s3.sh ✔️
mint-erasure.sh ✔️
mint-dist-erasure.sh ✔️
mint-gateway-nas.sh ✔️
mint-compress-encrypt-dist-erasure.sh ✔️
mint-pools.sh ✔️
Deleting image on docker hub
Deleting image locally

@harshavardhana harshavardhana changed the title Fix user privilege escalation bug fix: user privilege escalation bug Dec 23, 2021
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@harshavardhana harshavardhana merged commit 5a96cbb into minio:master Dec 23, 2021
12 checks passed
@donatello donatello deleted the test-user-perm branch December 23, 2021 21:09
@bluikko
Copy link

bluikko commented Jan 13, 2022

The workaround may sound obvious for a seasoned MinIO developer but as an occasional user it is not exactly clear, even after looking at the documentation for a while.

Do I understand it correctly that the policy action key admin:CreateUser needs an explicit Deny set in policy for all users that I want to implement this workaround for, meaning for example a fragment added to existing policy for user needsworkaround:

    {
      "Action": [
        "admin:CreateUser"
      ],
      "Effect:" "Deny",
      "Resource": [
        "arn:aws:s3:::*"
      ]
    }

@harshavardhana
Copy link
Member

@bluikko you should upgrade and not worry about workarounds. Its been almost 2 weeks since its published GHSA-j6jc-jqqc-p6cx

I would consider it a waste of time while the release is available with a fix.

@bluikko
Copy link

bluikko commented Jan 13, 2022

@bluikko you should upgrade and not worry about workarounds. Its been almost 2 weeks since its published GHSA-j6jc-jqqc-p6cx

I would consider it a waste of time while the release is available with a fix.

Agreed, but it is not a simple matter to get this updated so I have a choice of deploying a workaround, doing nothing, or updating in a few months.

@klauspost
Copy link
Contributor

@bluikko We offer backports of critical fixes like this as part of our subscription offer. If you have critical production environments you should consider that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants