Possible unexpected behaviours in subspace permissions #800
Assignees
Labels
kind/bug
Something isn't working
Comments
mergify bot
pushed a commit
that referenced
this issue
Apr 4, 2022
## Description This PR adds permissions sanitation before storing the permissions while handling a `MsgSetUserGroupPermissions` or `MsgSetUserPermissions`. Special review required by @manu0466 since he was the one to report the bug (and suggest a solution). Closes: #800 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [x] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Bug description
To store permission that an user have in a subspace we use
uint32and then we check the less significative 6 bits to extract the user permissions. Since we don't sanitize the receiveduint32to make sure that only the less significative 6 bit are set a malicius admin or simply by error can gave themself or other users unwanted permissions in case of an extenions of the current permissions system.Steps to reproduce
Let's assume that the admin Bob have the
PermissionSetPermissionsand gives to Alice the following permission:0b11111111111111111111111111000001that is considered from our current implementation a validPermissionWriteand stored as is.Now let's assume that we introduce a new
FancyPermissionthat have bit mask0b1000000then now we have that Alice inherit this permission even if Bob din't gave to her.Expected behavior
The permissions bit mask should be sanitize to make sure that we only store permissions that we are currently supporting, to do so we can do bitwise and operation with the mask
0b111111to set to 0 the unused bits before storing the data.The text was updated successfully, but these errors were encountered: