Re: Bare repositories in the working tree are a security risk

On Fri, Apr 15, 2022 at 05:41:59PM -0700, Glen Choo wrote:

To be totally honest, I am not absolutely convinced.

I agree that it's sub-optimal that Git is an attack vector for remote code execution, but I think there is significant social engineering required in order to meaningfully exploit this. Particularly because an attacker must convince their victim to:

  - clone the repository,
  - cd into the embedded bare repository, and
  - run a git command

Scripting around the output of git commands in your PS1 makes the latter more likely, so I think it's worthwhile to explore how to either prevent this type of attack or make it substantially less likely to have a user run git commands that execute parts of the config opportunistically.

That said, I think there are other approaches that we could take that would hopefully disrupt fewer existing workflows.

I think there are a handful of legitimate reasons that we might want to continue supporting this; for projects like libgit2 and Git LFS, it's useful to have repositories in a known state to execute tests in. Having bare Git repositories contained in some "test fixture" directory is a really easy way to do just that.

I'm a little late to chime into the thread, but I appreciate you summarizing some of the approaches discussed so far. Let me add my thoughts on each of these in order:

  1. Likely disrupts too many legitimate workflows for us to adopt
     without designing some way to declare an embedded bare repository
     is "safe".
  2. Ditto.
  3. This seems the most promising approach so far. Similar to (1), I
     would also want to make sure we provide an easy way to declare a
     bare repository as "safe" in order to avoid permanently disrupting
     valid workflows that have accumulated over the past >15 years.
  4. Seems like this approach is too heavy-handed.
  5. Ditto.

If there are lots of false positives, we should consider downgrading the severity of the proposed `EMBEDDED_BARE_REPO` fsck check to "info". I'm not clear if there is another reason why this patch would have a significant number of false positives (i.e., if the detection mechanism is over-zealous).

But if not, and this does detect only legitimate embedded bare repositories, we should use it as a reminder to consider how many use-cases and workflows will be affected by whatever approach we take here, if any.

Here's an alternative approach, which I haven't seen discussed thus far:

When a bare repository is embedded in another repository, avoid reading its config by default. This means that most Git commands will still work, but without the possibility of running any "executable" portions of the config. To opt-out (i.e., to allow legitimate use-cases to start reading embedded bare repository config again), the embedding repository would have to set a multi-valued `safe.embeddedRepo` configuration. This would specify a list of paths relative to the embedding repository's root of known-safe bare repositories.

I think there are a couple of desirable attributes of this approach:

  - It minimally disrupts bare repositories, restricting the change to
    only embedded repositories.
  - It allows most Git commands to continue working as expected (modulo
    reading the config), hopefully making the population of users whose
    workflows will suddenly break pretty small.
  - It requires the user to explicitly opt-in to the unsafe behavior,
    because an attacker could not influence the embedding repository's
    `safe.embeddedRepo` config.

If we were going to do something about this, I would strongly advocate for something that resembles the above. Or at the very least, some solution that captures the attributes I outlined there.

I would be happy to work together with you (or anybody!) on developing patches in that direction, so let me know if you are interested in coordinating our efforts.

No rush. Regardless of your time out-of-office, we should take advantage of the fact that this is a long-known wart to carefully craft a solution that provides some additional safety while disrupting as few existing workflows as possible.

Thanks, Taylor