Access Control Easy to Circumvent

[0x4007]

The bot clearly states that the assignee does not have access to set the Priority label, thus funding the bounty. However, it appears that they were able to edit an invalid label that was already on the issue, and rename it to a valid label to generate a payment permit.

Proposed Solution

• If anybody but an admin or billing_manager updates pricing related labels, then an admin or billing_manager must manually re-allow automatic payment permit generation. This state should be saved in our database.

• The bot will check the last updated time of a dependent (pricing related) label and if it was between the time the issue was started and completed. This implies that one of the dependent labels was tampered with while a task was being worked on.

Example:

• Issue has Priority: 4 (Urgent) and Time: <1 Hour
• In our database we have the updated column for Priority: 4 (Urgent)
• The issue was started on by an assignee on 1 October.
• The Priority: 4 (Urgent) label was last modified, by an unauthorized contributor, on 2 October.
• The issue is marked as complete.
• Due to the dependent label Priority: 4 (Urgent) being modified after the task was started, the bot refuses to generate the permit and explains the situation.

Notes

• this should only be concerned with pricing related labels defined in our config
• In this scenario, if it is still not authorized, then the admin or billing_manager should change the label back from Y to X and the bot should re-allow permits to be generated.
• when it is authorized, perhaps we should make a command like /approve for admins, and billing_managers which will "approve" in the database see below.
• Our access control should accomodate for these as well. For example if I wrote /allow set-priority @seprintour true and if seprintour changed the priority label, it should be approved implicitly.

Unauthorized label changes table

1. id, int8
2. created, timestamptz
3. updated, timestamptz
4. user id, int8
5. repository id, int8
6. label from, string
7. label to, string
8. authorized, bool

@seprintour, You are not allowed to add Priority: 3 (High)

Originally posted by @ubiquibot[bot] in ubiquity/ubiquibot-telegram#4 (comment)

Task checklist

1. create "label changes" table
2. intercept label change event and log if they dont have appropriate permissions
3. create /approve command (i feel like this part of the proposal can be refined... rfc!)

[seprintour]

/start

[ubiquibot]

Deadline	Sun, 10 Sep 2023 21:56:35 UTC
Registered Wallet	0x3623338046b101ecEc741De9C3594CC2176f39E5

Tips:

• Use /wallet 0x0000...0000 if you want to update your registered payment wallet address @user.
• Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
• Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the bounty.

[0x4007]

@seprintour curious to know your input on the proposed solution. I didn't do research on this and I am not confident that it is the best approach.

rfc @ubiquity/software-development

[kamaalsultan]

I wonder if the bot should automatically resets non-admin's label update.

[0x4007]

@seprintour I think we should figure out a good strategy before you start coding.

[seprintour]

@seprintour I think we should figure out a good strategy before you start coding.

Yea, not coding yet.. Trying to hotfix the forum issue.

Sent a DM

[rndquu]

1. Could somebody explain to me how is this possible? There is a special condition that checks who can generate permits when issue is closed:

• repository collaborators
• organization members
• users with admin or billing_manager organization roles

I haven't found @seprintour to be in any of them.

2. @seprintour How come that you can edit labels if you don't have any permissions in the repo or org?

[0x4007]

1. Could somebody explain to me how is this possible? There is a special condition that checks who can generate permits when issue is closed:

• repository collaborators
• organization members
• users with admin or billing_manager organization roles

I haven't found @seprintour to be in any of them.

2. @seprintour How come that you can edit labels if you don't have any permissions in the repo or org?

I temporarily added @seprintour as a collaborator on that specific repo to push some hotfixes to the CI on development directly.

Although collaborators are more trusted positions, projects should still be able to set fine grained permissions i.e. like our /allow command; but that as we see can be easily circumvented.

[ubiquibot]

Do you have any updates @seprintour? If you would like to release the bounty back to the DevPool, please comment /stop
Last activity time: Mon Sep 11 2023 18:41:07 GMT+0000 (Coordinated Universal Time)

[seprintour]

Do you have any updates @seprintour? If you would like to release the bounty back to the DevPool, please comment /stop
Last activity time: Mon Sep 11 2023 18:41:07 GMT+0000 (Coordinated Universal Time)

📌

[seprintour]

@pavlovcik I am going to do some research and write it down here as it comes to mind

• I think adding a label that means a label was edited is a possible solution, let's assume we have a label Label Edited and once this label is on an issue.. It's disabled from permit generation pending someone with enough permission to get rid of the label or maybe we can create a command that can only be called by an admin or billing_manager.

Once the bot sees that the issue once has the label and it was removed by someone who's not one of these two roles.. It rejects permit generation any way.

rfc @rndquu

[seprintour]

@pavlovcik

[rndquu]

@pavlovcik I am going to do some research and write it down here as it comes to mind

• I think adding a label that means a label was edited is a possible solution, let's assume we have a label Label Edited and once this label is on an issue.. It's disabled from permit generation pending someone with enough permission to get rid of the label or maybe we can create a command that can only be called by an admin or billing_manager.

Once the bot sees that the issue once has the label and it was removed by someone who's not one of these two roles.. It rejects permit generation any way.

rfc @rndquu

Can't we simply reject editing labels for all roles unless the role is:

• admin
• billing_manager
• explicitly allowed to edit labels via the /allow command (we don't have this permission right now)

?

[seprintour]

Can't we simply reject editing labels for all roles unless the role is:

We can't really directly intercept the Github UI actions and reject it, all we can do is create a reaction after the webhook is triggered that would permission-gate the action that is forbidden for the bot.

[rndquu]

Can't we simply reject editing labels for all roles unless the role is:

We can't really directly intercept the Github UI actions and reject it, all we can do is create a reaction after the webhook is triggered that would permission-gate the action that is forbidden for the bot.

I still don't understand what exactly you edited here

As far as I understand here is what happened:

1. You removed the Priority: 3 (High) label from the issue and added it again
2. The bot removed the Priority: 3 (High) label (because you don't have "set priority" permission) which you added and added it back
3. You reopened and closed the issue
4. Permit was generated because at that time pavlovcik added you to the repo's collaborators (which is an expected behavior)

Could you describe what label exactly did you edit and what was the exact flow?

[seprintour]

Could you describe what label exactly did you edit and what was the exact flow?

I was already a collaborator

1. Labels were changed, for ex. Priority: 2 (High) became Priority: 3 (High), so it could not generate a permit
2. I tried to replace Priority: 2 (High) with the newly generated Priority: 3 (High) label using the Github UI, it rejected it (expected behavior from the /allow feature)
3. So i decided to delete the Priority: 3 (High) and quickly renamed the outdated Priority: 2 (High) to Priority: 3 (High)
4. I then reopened and closed the issue and it generated the permit

[whilefoo]

This is possible only if you're a member or collaborator, so this is not that big of an issue I guess. Normally we should be able to trust members.

[seprintour]

This is possible only if you're a member or collaborator, so this is not that big of an issue I guess. Normally we should be able to trust members.

Same comment I made when the issue was raised, it's not really a vulnerability but if its a feature needed here.. Only way to go around it is a reaction once an unauthorized collaborator edits a label

[0x4007]

Although collaborators are more trusted positions, projects should still be able to set fine grained permissions

#743 (comment)

This doesn't seem like a very elegant solution, but it is fairly secure:

All future permits on that repository will throw an error explaining the situation, ideally explaining that the collaborator changed from X to Y label (only concerning pricing related labels defined in our config) and require some type of manual approval (i.e. command invocation) from an admin. /allow-edit-labels @seprintour true

In this scenario, if it is still not authorized, then the admin should change the label back from Y to X and the bot should re-allow permits to be generated.

[seprintour]

This doesn't seem like a very elegant solution, but it is fairly secure:

Elegant solutions will be to directly intercept this event but unfortunately we can't do that, is there any other way we can do this that doesn't involve attaching a label that the bot acts on?

Rfc

[0x4007]

Well I was thinking that the bot will check the last updated time of a dependent label and if it was between the time the issue was started and completed. This implies that one of the dependent labels was tampered with while a task was being worked on.

Example:

• Issue has Priority: 4 (Urgent) and Time: <1 Hour
• In our database we have the updated column for Priority: 4 (Urgent) (or can we just query this directly from the GitHub API?)
• The issue was started on by an assignee on 1 October.
• The Priority: 4 (Urgent) label was last modified, by an unauthorized contributor, on 2 October.
• The issue is marked as complete
• Due to the dependent label Priority: 4 (Urgent) being modified after the task was started, the bot refuses to generate the permit and explains the situation.

---

I just asked ChatGPT and we might be able to leverage the audit log.

• Requires it to be an org not personal repo.
• Required permissions

But I have a feeling we will need to manually track these things with our bot and the database.

[seprintour]

The Priority: 4 (Urgent) label was last modified, by an unauthorized contributor, on 2 October.

We can see when last a label was edited but it doesn't keep track of who edited.. Perhaps we can save that to the db when the event is triggered and do the check when the issue is closed

[seprintour]

Unauthorized label changes table

1. id, int8
2. created, timestamptz
3. updated, timestamptz
4. user id, int8
5. label from, string
6. label to, string
7. approved, bool

I think there's supposed to be a field for repository

[seprintour]

3. create /approve command (i feel like this part of the proposal can be refined... rfc!)

Using this command (callable by admin) would just approve the change for the specific repo and enable all permits been held by the recent change to be generated

Should we limit the approve command to just admin? and what if the admin want to decline the request and roll back.

Maybe we can make it (/labelEdit approve and /labelEdit decline) or something.. rfc @pavlovcik

We can also pass the id of the change like this /labelEdit approve 24 so i can approve a label change even before a permit generation fail

[0x4007]

Default state is declined so I don't see the need to specify state if it's a one bit operation.

Admin and billing_manager are both responsible for finances by default.

Also for the access control we should let those with delegated access change the types of labels. See point 4 under Notes

We can also pass the id of the change like this /labelEdit approve 24

Normal users aren't meant to have access to the database so I don't see how they can find out the id of the change

[seprintour]

Normal users aren't meant to have access to the database so I don't see how they can find out the id of the change

I'd still leave the id, because this event occurs completely outside and issue or a comment.. It cannot just send a random comment anywhere and it cannot reply.

So to work without the ID, it needs to have an issue where it failed to generate a permit, and perhaps among the reason for failure.. I can attach the id of the change that triggered the failure

[seprintour]

Or we can just search for changes by label. using the label on the issue that was just closed.. would be a better approach

[0x4007]

Just needs to check at permit generation time inside of the issue for any unauthorized changes.

When an issue is closed as complete inside of that closed event context we have the labels and the issue.

From there we check the labels and see if any were tampered with.

[seprintour]

@pavlovcik check this out

Seprintour-Test/test#43 (comment)

Does the error message work?

[seprintour]

Ready for review @pavlovcik

QA: Seprintour-Test/test#44

[ubiquibot]

Task Assignee Reward

[ CLAIM 600 WXDAI ]

0x36233380...2176f39E5

If you've enjoyed your experience in the DevPool, we'd appreciate your support. Follow Ubiquity on GitHub and star this repo. Your endorsement means the world to us and helps us grow!

We are excited to announce that the DevPool and UbiquiBot are now available to partners! Our ideal collaborators are globally distributed crypto-native organizations, who actively work on open source on GitHub, and excel in research & development. If you can introduce us to the repository maintainers in these types of companies, we have a special bonus in store for you!