BMO Integration User is a full administrative user on Phabricator

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: claudijd, Assigned: dkl)

Tracking

(Blocks: 1 bug, {conduit-story, conduit-triaged})

Firefox Tracking Flags

(Not tracked)

Details

User Story

1) create new secure-revision policy (project) and add the phab-bot user and any admins to it.
2) phab-bot from bmo would then be able to access the new revision in order to update its policies.
3) New bmo-* group projects will have edit/view/join policies set to the secure-revision project.
4) BMO extension code will need to be updated to no longer set policies to admin and instead use secure-revision.

Things to do after code is live on BMO:

1) Manually update policies on all current bmo-* group projects to make editable by secure-revision policy members.
2) make all new revisions default to private to secure-revision policy
3) remove full admin from phab-bot in phabricator

Attachments

(1 attachment)

I cannot confirm this in prod/dev, so please keep that in mind when reviewing this, but I was reviewing the constants used on the BMO phabricator integration saw that https://github.com/mozilla-bteam/bmo/blob/master/extensions/PhabBugz/lib/Constants.pm#L22 defines the integration user as phab-bot@bmo.tld.

That user was familiar to me because I was using in my testing and I just happened to remember that that user is a full administrator of my local phabricator instance.  If this is true in say prod/dev, that could mean that a compromise of that user's credentials could result in a full compromise of phabricator.

I know these two systems need to trust each other, because Phabricator relies on BMO to provide the authentication layer for users, which will then use Phabricator to land code.  The concern I have is that the phab-bot@bmo.tld specifically might be over-trusted as an admin and that we might be better served it operated with a lower permissions set.

It could also be very possible that since the phab-bot@bmo.tld needs this higher-level permission to be able to allow the BMO auth to work and if it were compromised as a lower priv user, the same net effect might be possible where an attacker would simply make the necessary request to phabricator to authenticate an attacker as anyone they would like.

At any rate, I want to raise this as a potential concern and let someone who knows the internals a little better to give this some thought.
(Assignee)

Comment 1

2 years ago
Thoughts?

1) remove full admin from phab-bot
2) create special bmo-phabricator group (project) and add the phab-bot user to it.
3) make all new revisions default to private to bmo-phabricator group
4) phab-bot from bmo would then be able to access the new revision in order to update its policies.
5) also allow projects to be edited and created by the bmo-phabricator group which will also allow the integration to continue to work.

caveat: even though phab-bot is not longer full admin, it could still be potentially compromised and an attacker could add themselves to a security group (project) in phab. And also they could see private revisions in the short time before BMO comes in and updates the policies. Still dangerous but maybe not as much.

dkl
:dkl - that seems reasonable to me.  It at least binds your risk exposure to one account, where as with an admin account they could create a new account to do all their work there and potentially avoid incident respondors down the road.
Keywords: conduit-triaged
Summary: BMO Integration User is a full administrative user on Phabricator → [story] BMO Integration User is a full administrative user on Phabricator
Keywords: conduit-story
Summary: [story] BMO Integration User is a full administrative user on Phabricator → BMO Integration User is a full administrative user on Phabricator
(Assignee)

Updated

a year ago
User Story: (updated)
The policy shouldn't be called "secure-revision" or we should rename the existing "secure-revision" group.
Flags: needinfo?(dkl)
(Assignee)

Updated

a year ago
User Story: (updated)
Flags: needinfo?(dkl)
(Assignee)

Comment 4

a year ago
Testing has been successful and this can move forward.
(Assignee)

Updated

a year ago
User Story: (updated)
There is no patch attached to this bug but there is a commit referencing this bug in the bmo repo.
Assignee: nobody → dkl
Flags: needinfo?(dkl)
(Assignee)

Comment 6

a year ago
Posted patch 430.diffSplinter Review
Flags: needinfo?(dkl)
Attachment #8956449 - Flags: review?(dylan)
Attachment #8956449 - Flags: review?(dylan) → review+
(Assignee)

Comment 7

a year ago
Now live.
Group: conduit-security
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.