Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure => latest now broken after CVE-2022-24765 patch. #535

Closed
thepro101 opened this issue Apr 13, 2022 · 33 comments · Fixed by #549
Closed

ensure => latest now broken after CVE-2022-24765 patch. #535

thepro101 opened this issue Apr 13, 2022 · 33 comments · Fixed by #549
Assignees

Comments

@thepro101
Copy link

Describe the Bug

After applying the patch for git on both ubuntu 18.04 and ubuntu 20.04 machines, puppet runs now fail.

change from 'absent' to 'latest' failed: Path /destination/path exists and is not the desired repository. (corrective)

Expected Behavior

Successful puppet run by cloning the desired repo to the desired location

Steps to Reproduce

update git to 1:2.25.1-1ubuntu3.3 on 20.04 and rerun puppet.

Environment

  • Version v4.0.0
  • Platform Ubuntu 20.04, and Ubuntu 18.04
@thepro101
Copy link
Author

I just tried the latest release from forge v5.0.0, and the issue still exists.

@adam-vessey
Copy link

adam-vessey commented Apr 13, 2022

Poking around a bit, I... suspect it's due to a disconnect between the :owner and :user properties... in that now, git refuses to operate if they're not the same value.

... in my instance, I was specifying just the owner on the vcsrepo, and it was breaking... but if I either add or switch to user, it seems to work?

@thepro101
Copy link
Author

That actually makes a lot of sense. In our case the we are cloning the repo as a different user than the owner by specifying identity => /home/user/.ssh/id_rsa. I cannot confirm this, but will test using a single user.

@gerardkok
Copy link

gerardkok commented Apr 14, 2022

I also see this issue on Ubuntu 20.04 and git 1:2.25.1-1ubuntu3.3. I'm seeing it in the voxpupuli/puppetboard module, that indeed employs the user attribute (see https://github.com/voxpupuli/puppet-puppetboard/blob/091eaf83447523b1fecf7d069e9f4307c93a9b4e/manifests/init.pp#L138).

The problem shows itself when vcsrepo tries to work out if the working copy already exists, by running /usr/bin/git config --get remote.origin.url on the working copy (see

return git('config', '--get', "remote.#{@resource.value(:remote)}.url").chomp == default_url
). With git 1:2.25.1-1ubuntu3.2, this returns (both when running asroot and as user):

/usr/bin/git config --get remote.origin.url
https://github.com/voxpupuli/puppetboard

For git 1:2.25.1-1ubuntu3.3, the result when ran as user is the same, but when ran as root, there is no output.

My current workaround is to downgrade to git 1:2.25.1-1ubuntu3.2, and to mark the git package as 'held'. Perhaps not ideal, but at least puppet runs again.

@normelton
Copy link

normelton commented Apr 14, 2022

I seemed to have fixed this by replacing all the calls to git(...) with git_with_identity(...).

When git_with_identity was added (93815a9), some (but not all) calls to git(...) were converted. I'm happy to submit a PR with my fix, but would rather someone a little more knowledgeable speak to whether some calls were intentionally left out.

@adam-vessey
Copy link

adam-vessey commented Apr 14, 2022

@normelton : In what version? In the latest v5.0.0 git provider, there's no direct git() calls left (though there were some in v4.0.0)... everything has been changed over to use exec_git(), which does the user management bit; though, exec_git() was only introduced in v5.0.0.

... if I read correctly, in v5.0.0, the git() method no longer exists, as it was created via the has_command() business, I'm thinking; however, v5.0.0 no longer does the has_command() thing.

@adam-vessey
Copy link

To resolve for now, I've taken to rolling a monkey-patch wrapping around the exec_git() method (and destroy()) to deal with safe.directory entries in system-wide git config... approximately: https://gist.github.com/adam-vessey/83e26d17da22349dcbd6566e226ef78e

... not the most comfortable with it, but we can live with this for now...

... almost... feels like separate resources for these entries should be created in some manner, so they can be managed more explicitly? Or have some kind of tie-in? Some kind of more general vcsrepo::git Puppet class, which can manage system-wide configuration, with separate resources for these entries or something?

@normelton
Copy link

Ahh so we were back at 3.1.1. Moving to 5.0.0 fixed our problem. Thanks!!!

@gerardkok
Copy link

FYI: I upgraded to 5.0.0 too (was on 3.2.1/4.0.1), and like @thepro101 I'm still seeing the issue.

If I run puppet with --debug, the logs show:

Debug: Executing: 'git config --get remote.origin.url'
Debug: Executing: 'git config --get remote.origin.url'
Error: Path /srv/puppetboard/puppetboard exists and is not the desired repository.
Error: /Stage[main]/Puppetboard/Vcsrepo[/srv/puppetboard/puppetboard]/ensure: change from 'absent' to 'present' failed: Path /srv/puppetboard/puppetboard exists and is not the desired repository.

mergwyn added a commit to mergwyn/control-repo that referenced this issue Apr 15, 2022
mergwyn added a commit to mergwyn/puppet-puppetboard that referenced this issue Apr 15, 2022
mergwyn added a commit to mergwyn/control-repo that referenced this issue Apr 15, 2022
@snaki4
Copy link

snaki4 commented Apr 19, 2022

Workaround of @mergwyn wont work if you have different user to run git from and actual owner / group.

Example when it fails :

	user		=> 'root',
	identity	=> '/root/.ssh/id_rsa_git',
	owner		=> 'www-XXX-com',
	group		=> 'www-XXX-com',

So, basically, problem persist when you tries to checkout as root, but repo should have different user/group ownership

PeterJCLaw added a commit to PeterJCLaw/srcomp-puppet that referenced this issue Apr 19, 2022
This was to test if it fixed puppetlabs/puppetlabs-vcsrepo#535
for us. It doesn't, but should be fine otherwise.
PeterJCLaw added a commit to PeterJCLaw/srcomp-puppet that referenced this issue Apr 19, 2022
Rather than checking them out as root and then changing their
ownership later. This avoids extra work and also works around
puppetlabs/puppetlabs-vcsrepo#535.
@mpdude
Copy link

mpdude commented Apr 20, 2022

I think that this has nothing to do with ensure => latest, since I am checking out dedicated commits/revisions.

In my case, the checked-out directory belongs to a dedicated user account and I can confirm that using user (instead of having a file resource setting ownership) makes the problem go away.

@UiP9AV6Y
Copy link

git 2.35.2 introduced a remidy for a CVE. as such you cannot execute git commands in a local worktree if its filesystem ownership is different than the current user.

to resolve this, each local worktree directory matching this condition must be whitelisted explicitly using the multi-value git setting safe.directory

i have not looked into the git source, but it seems this settings receives a stricter treatment than other git configuration values, i.e. you cannot configure it on the commandline or the config of a local worktree. the error message suggest you set it on the global scope, which is the users home directory.

the problem with this, is, that Puppet::Util::Execution.execute clears out certain environment variables when running commands; HOME being one of them.
git config --global cannot work without this variable being set and as such, whitelisting directories in this file is futile.

to fix this issue, two things need to be done:

  • define HOME in git_exec.

    this is already done, when the git commands are run under a different user. however, if you simply use the owner attribute of the defined type, the command user and current user are the same, hence HOME is not defined

  • whitelist the local working tree

    since you cannot do this on the commandline, you have to manage the safe.directory git config directive separatly; this is IMO not the responsibility of the VCS module

    maybe add a note to the README about further steps necessary when managing VCS repos with git >= 2.35.2

TL;DR

git 2.35.2 introduced a security feature, which requires a config setting to make this puppet module to continue to work in all configuration scenarios. the location of this setting is currently unreachable as it relies on the HOME environment variable being set, which puppet explicitly undefines.

@maxadamo
Copy link

resolves voxpupuli/puppet-puppetboard#355

@smortex
Copy link
Collaborator

smortex commented Apr 24, 2022

I also see this issue on Ubuntu 20.04 and git 1:2.25.1-1ubuntu3.3. I'm seeing it in the voxpupuli/puppetboard module, that indeed employs the user attribute (see voxpupuli/puppet-puppetboard@091eaf8/manifests/init.pp#L138).

@gerardkok you are referring to this commit voxpupuli/puppet-puppetboard@1935f97 but it was released only recently (2 days ago), probably you where not using the code that use user.

@yakatz
Copy link

yakatz commented Apr 25, 2022

We might want to highlight this as I and a whole bunch of other people missed it the first few times reading this:

The new safe.directory setting can not be used as it relies on the HOME environment variable being set, which puppet explicitly undefines.

@adam-vessey
Copy link

adam-vessey commented Apr 25, 2022

safe.directory can be set system-wide, with git config --system [...], instead of git config --global [...], and thereby side-step the need for HOME to be defined.

@david-peters-aitch2o
Copy link

are there any fixes for this? the safe.directory did not work for me

@yakatz
Copy link

yakatz commented May 25, 2022

are there any fixes for this? the safe.directory did not work for me

Where did you put it? We had to put it in the system config, not the global (root user) config.

@david-peters-aitch2o
Copy link

david-peters-aitch2o commented May 25, 2022

# cat /usr/local/etc/gitconfig 
[safe]
        directory = /home/www/
#

what's interesting is first run of puppet is ok, it creates the /home/www and brings the data in, any subsequent runs of puppet fails with: change from 'absent' to 'latest' failed: Path /home/www exists and is not the desired repository. (corrective)

@yakatz
Copy link

yakatz commented May 25, 2022

On our systems (RedHat and Ubuntu), it is just /etc/gitconfig, not sure what OS you are running.

If you run git config --system -l, does it show the values you set? Otherwise, does it should show an error message with the location of where it expects the file to be.

What we are doing:

[root@host ~]# git config --system -l
include.path=/etc/gitconfig.d/puppet
[root@host ~]# cat /etc/gitconfig.d/puppet
# This file is managed by Puppet. DO NOT EDIT.
[safe]
        directory = /var/www/sites/wiki.example.com/code/mediawiki-1.35-LTS
        directory = /var/www/sites/wiki.example.com/sites
        directory = /var/www/sites/www.example.com/cms
        directory = /var/www/sites/www.example.com/local-cgi-bin
[root@host ~]#

Our Puppet manifests: https://gist.github.com/yakatz/1df458814dcbc6f2604a5a9d9f2073d4

@yakatz
Copy link

yakatz commented May 25, 2022

# cat /usr/local/etc/gitconfig 
[safe]
        directory = /home/www/
#

I actually see your problem (and the manifest I mentioned above would help you) - don't use a trailing slash

@david-peters-aitch2o
Copy link

I have removed the trailing slash but still have the same error

Error: /Stage[main]/app_base/Vcsrepo[/home/www ensure: change from 'absent' to 'latest' failed: Path /home/www exists and is not the desired repository. (corrective)

code used

  vcsrepo { '/home/www':
  ensure   => latest,
  provider => git,
  source   => 'ssh://git@host:repo.git',
  identity => '/usr/home/www/.ssh/id_rsa',
  user     => 'www',
  owner    => 'www',
  group    => 'www',
  revision => 'master',
  }

first time puppet runs it creates the dir with all data in it as expected, subsequent runs fail with above error

@yakatz
Copy link

yakatz commented May 26, 2022

What is the output of git config --system -l run as root?

What is the output of git config --system -l run as www (using sudo)?

What if you go to that directory and run git fetch manually as root and then as www?

The output from those steps should tell you exactly what is wrong.

To confirm, does your gitconfig now look like this?

# cat /usr/local/etc/gitconfig 
[safe]
        directory = /home/www   <--- no trailing slash
#

@david-peters-aitch2o
Copy link

so both have the same output (as root and as www)

# git config --system -l
safe.directory=/home/www
safe.directory=/home/www/versions
#

I think I found the problem, I use vcsrepo to clone two different repo's in the same /home/www

repo1 to /home/www
repo2 to /home/www/versions

does vcsrepo support this ?

@yakatz
Copy link

yakatz commented Jun 1, 2022

@david-peters-aitch2o I just tested that and also had no issues. When you run as your www user, can you run a git fetch?

@chelnak
Copy link

chelnak commented Jun 10, 2022

Hey everyone 👋 .

I'm going to be investigating this issue and will hopefully get a resolution soon. It's been great reading through all of the suggestions/investigation that everyone has been doing.

I think this is a tricky one given that the issue was caused by a CVE remediation. We need to be mindful that we do not re-introduce a vulnerability again (e.g blanket allow).

I see that there are a few suggestions for using the system wide gitconfig given some limitations with exec_git... I'd like to see if we can address these to scope the safe.directory config to the user that is invoking git.

I'll do some more research and hopefully will update next week!

@chelnak chelnak self-assigned this Jun 10, 2022
@yakatz
Copy link

yakatz commented Jun 10, 2022

I see that there are a few suggestions for using the system wide gitconfig given some limitations with exec_git... I'd like to see if we can address these to scope the safe.directory config to the user that is invoking git.

As long as the git commands are run with no HOME directory, there is no user global .gitconfig to load.

For us, Puppet is cloning most repositories as root on machines with no direct user access, so it doesn't make a huge difference.

Separately, this has caused us to reevaluate whether things should really be pushed out with vcsrepo or would be better off as packages (rpm, pip, etc). By changing a number of our repos to packages, we have gotten a significant agent run speed improvement.

@chelnak
Copy link

chelnak commented Jun 13, 2022

We can easily add HOME in to the custom_environment for standard executions which will solve part of the problem.. but I wonder if having another resource to manage the safe.directory config is worth testing. I'm not 100%.. might be tricky due to the multi-provider implementation.

@yakatz
Copy link

yakatz commented Jun 13, 2022

If managing safe.directory is built in, it would probably make sense for it to be a parameter of the main resource (i.e. add_safe_directory => true)

@chelnak
Copy link

chelnak commented Jun 16, 2022

Hey everyone! Just wanted to post a quick update.

Here is a very early PR with a fix for this issue: #549

@yakatz I liked your suggestion of adding a new parameter. I liked that it was an explicit choice for the user.

There are a couple of things with the git versions that I need to think about. For example, the patched version of git that introduces the CVE mitigation is 2.35.1 however on ubuntu this has been back-ported in to 1:2.25.1-1ubuntu3.4.

This means that we can't just do a version compare and apply config if git is above a certain version.

Also I would have liked to have introduced a new property for this given that it is a measurable "thing" on the system.

However, the type has ensurable values that call exists? explicitly which means that git is invoked before most things are evaluated. I'm actually going to spend a bit more time with the property option but need to find a balance between the value of the fix and the amount of unrelated code being changed.

@chelnak
Copy link

chelnak commented Jun 16, 2022

In addition to the above, the team has been talking about the possibilities of moving git back out in to its own module.

There are a few of benefits here, mainly:

  • The new module would be built with the resource api
  • Reduced complexity.
  • The module would be responsible for one domain (git).

There is an archived git module which has some cool stuff we could lean from.

Let us know what you think!

chelnak added a commit that referenced this issue Jun 17, 2022
Prior to this commit, users running newer versions of Git and setting
the `owner` parameter on a resource would encounter an error during
puppet runs.

This commit fixes the issue by allowing users to add the path of the
resources to Gits global `safe.directoy` configuration.

This can be achieved by specifying `safe_directory => true` on a resource.
chelnak added a commit that referenced this issue Jun 17, 2022
This commit adds a section to the README that briefly describes the CVE
and our mitigation to errors caused by it's remediation in later Git
versions.
chelnak added a commit that referenced this issue Jun 17, 2022
Prior to this commit, users running newer versions of Git and setting
the `owner` parameter on a resource would encounter an error during
puppet runs.

This commit fixes the issue by allowing users to add the path of the
resources to Gits global `safe.directoy` configuration.

This can be achieved by specifying `safe_directory => true` on a resource.
chelnak added a commit that referenced this issue Jun 17, 2022
This commit adds a section to the README that briefly describes the CVE
and our mitigation to errors caused by it's remediation in later Git
versions.
chelnak added a commit that referenced this issue Jun 22, 2022
@bill-mcgonigle
Copy link

This CVE patch was released on Debian Buster in the past few days, causing breakage there on system puppet (5.5.10-4). There doesn't seem to be a patch available for vcsrepo 3.2.1, the latest with Puppet 5 support. I'm not sure why my Bullseye machines don't seem to be affected. As a workaround, this WFM:

once:

  case $::facts['os']['distro']['codename'] {
    'buster' : {
      concat { '/etc/gitconfig' :
        owner   => 'root',
        group   => 'root',
        mode    => '0644',
      }
    }
  }

and then in my project-handling class:

  case $::facts['os']['distro']['codename'] {
    'buster' : {
      concat::fragment { "gitconfig_$project" :
        target  => '/etc/gitconfig',
        content => "[safe]\n\tdirectory = /usr/local/repos/$project\n\n",
        before  => Vcsrepo["/usr/local/repos/$project"],
      }
    }
  }

@bill-mcgonigle
Copy link

This CVE patch was released on Debian Buster in the past few days

Now affecting Bullseye (-security) as well - same workaround is effective as of:

2023-01-30 04:47:13 upgrade git:arm64 1:2.30.2-1 1:2.30.2-1+deb11u1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.