` URIs in Diffusion
ClosedPublic
Actions

Authored by epriestley on Jun 13 2014, 1:57 PM.

Details

Reviewers

btrahan
chad

Commits

Restricted Diffusion Commit
rPd401036bd82b: Prevent the use of `file://` URIs in Diffusion

Summary

Via HackerOne. There are two attacks here:

Configuring mirroring to a file:// URI to place files on disk or overwrite another repository. This is not particularly severe.
Configuring cloning from a file:// URI to read repositories you should not have access to. This is more severe.

Historically, repository creation and editing explicitly supported file:// URIs to deal with use cases where you had something else managing repositories on the same machine. Since there were no permissions, repository management was admin-only, and you couldn't mirror, this was fine.

As we've evolved, this use case is a tiny minority use case and the security implications of file:// URIs overwhelm the utility it provides. Prevent the use of file:// URIs. Existing configured repositories won't stop working, you just can't add any new ones.

Also prevent localPath from being set via Conduit (see T4039).

Test Plan

Tried to create a file:// repository.
Tried to create a file:// mirror.
Tried to create a file:// repository via Conduit.
Created a non-file:// repository.
Created a non-file:// mirror.
Created a non-file:// repository via Conduit.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley updated this revision to Diff 22794.Jun 13 2014, 1:57 PM

epriestley retitled this revision from to Prevent the use of `file://` URIs in Diffusion.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added reviewers: chad, btrahan.

Herald added a subscriber: epriestley. · View Herald TranscriptJun 13 2014, 1:57 PM

Harbormaster completed remote builds in B1052: Diff 22794.Jun 13 2014, 1:59 PM

chad accepted this revision.Jun 13 2014, 2:02 PM

chad edited edge metadata.

This revision is now accepted and ready to land.Jun 13 2014, 2:02 PM

Closed by commit rPd401036bd82b (authored by @epriestley).

I just tried to import another file:// repo, and since it failed, i found this Diff.

Actually, this is a real problem at our site. For reasons not under my control, all our repos are file based. We could not have tried out phabricator in the first place if I wasn't able to import file:// repos. I really need to import another one, though. I suggest to make this a configurable option.

Quoting the description:

this use case is a tiny minority use case

there may actually be more windows-based environments without Git servers out there.

If this is not an option, can I just revert this commit in a local branch at our install?

This option is insecure and can not reasonably be made secure, so we do not currently plan to ever restore support in the upstream. You can try reverting this, but over time that's likely to require maintenance. (In other cases where installs were using file://, they have usually been able to switch without any issues.)

Changing the situation here to use a git server is out of my jurisdiction, and will probably not be done except for very good reasons. Trying to make Phabricator a good-enough reason unfortunately requires more file:// repos right now, so that is a kind of chicken-and-egg problem for me. I will try to revert it in the meantime, or is there a way to import repositories with the command line tools?

Hmm. I feel like there's something I'm missing, here? I can't add a new file:// repo, so instead I set the user owning Phabricator to be able to ssh to itself, and add the repo using an ssh:// URL. How does that help?

On its own, that's not any different. In many setups, this change has no practical security benefits (because policies are not used, or only users with access to everything have permission to create repositories).

If you later decided to create a hosted repository, anyone who has permission to create repositories could read it using a file:// URI. With the other repositories using ssh://, they could not.

747endetta added a subscriber: 747endetta.Sep 1 2014, 4:23 PM