The Puppet Labs Issue Tracker has Moved: https://tickets.puppetlabs.com

Bug #1806

supplementary groups are not reset

Added by Till Maas almost 6 years ago. Updated over 4 years ago.

Status:ClosedStart date:12/08/2008
Priority:NormalDue date:
Assignee:James Turnbull% Done:

0%

Category:plumbing
Target version:0.25.2
Affected Puppet version:0.24.6 Branch:
Keywords:

We've Moved!

Ticket tracking is now hosted in JIRA: https://tickets.puppetlabs.com

This issue is currently not available for export. If you are experiencing the issue described below, please file a new ticket in JIRA. Once a new ticket has been created, please add a link to it that points back to this Redmine ticket.


Description

I noticed that puppetmasterd does not reset it’s supplementary groups when switching to a different user. Therefore, if puppetmasterd is started manually by root, e.g. with “service puppetmaster start” on Fedora or CentOS, then it keeps the supplementary groups. This may allow puppet to access files, that it should not, i.e. files that are only readable by members of the group “root”. Also it may lead to a situation where puppet cannot access a file, because it can be only accessed for users in a certain group, that is not the primary group of puppet.

Attached are two patches, the first fixes puppetmasterd itself. I copied it into an older release of puppet, where it worked. The second patch is completely untested.

0001-Support-supplementary-groups.patch Magnifier - support for supplementary groups (2.82 KB) Till Maas, 05/15/2009 09:48 am

History

#1 Updated by Till Maas almost 6 years ago

  • File 0002-initialize-supplementary-groups-with-initgroups-wi.patch added

#2 Updated by James Turnbull almost 6 years ago

  • Category set to plumbing
  • Status changed from Unreviewed to Needs More Information
  • Assignee set to Luke Kanies

Luke?

#3 Updated by Luke Kanies almost 6 years ago

Seems like a bug to me, and the code looks right but I wouldn’t merge it without a good bit of testing.

#4 Updated by Luke Kanies almost 6 years ago

  • Status changed from Needs More Information to Accepted

#5 Updated by Luke Kanies almost 6 years ago

  • Status changed from Accepted to In Topic Branch Pending Review

#6 Updated by Scott Dodson over 5 years ago

I’ve applied the patches cleanly but now I get the following which I believe is due to a typo introduced on line 32, where I believe it’s missing a comma. After adding a comma it runs but tells me that it cannot change to user 100, puppet’s uid on my system.

Starting puppetmaster: /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `gem_original_require': /usr/lib/ruby/site_ruby/1.8/puppet/util/suidmanager.rb:32: syntax error (SyntaxError)
            self.initgroups(convert_xid :gid new_uid) if new_uid
                                                    ^
/usr/lib/ruby/site_ruby/1.8/puppet/util/suidmanager.rb:69: syntax error
/usr/lib/ruby/site_ruby/1.8/puppet/util/suidmanager.rb:76: syntax error from /usr/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:27:in `require'
        from /usr/lib/ruby/site_ruby/1.8/puppet.rb:17
        from /usr/sbin/puppetmasterd:83:in `require'
        from /usr/sbin/puppetmasterd:83

#7 Updated by James Turnbull over 5 years ago

  • Status changed from In Topic Branch Pending Review to Code Insufficient
  • Target version set to 0.25.0

#8 Updated by James Turnbull over 5 years ago

  • Target version changed from 0.25.0 to 2.6.0

Bumped since no progress and no tests.

#9 Updated by Till Maas over 5 years ago

  • File deleted (0001-Initialize-initgroups-before-user-switching.patch)

#10 Updated by Till Maas over 5 years ago

  • File deleted (0002-initialize-supplementary-groups-with-initgroups-wi.patch)

#11 Updated by Till Maas over 5 years ago

Here is a reworked patch, that should work better. It allows me to start puppetmaster and it has to correct groups afterwards. I did not yet test more. It also makes the error message if chuser fails verbose, it is now similiar to the warning when changing the group fails.

#12 Updated by James Turnbull over 5 years ago

  • Status changed from In Topic Branch Pending Review to Needs Decision

Luke? Thoughts? Still no tests obviously but your feedback on the patch would be good. I’ll send to the list for comment.

#13 Updated by Till Maas over 5 years ago

Btw. if you need some test procedure to understand why the current beheaviour is bad, use this one:

Steps to Reproduce: 1. # service puppetmaster start 2. # cat /proc/$(ps —User puppet -o pid | tail -n 1)/status | grep Group

Actual results: The output matches “id -G root”.

Expected results: The output should match “id -G puppet”, i.e. the process should run with the supplementary groups of puppet.

The default supplementary groups of root include the group disk, which e.g. provides raw read access on disk devices.

This works on a Fedora or CentOS system. Fedora bug report: https://bugzilla.redhat.com/show_bug.cgi?id=475201

#14 Updated by micah - about 5 years ago

This works on a Fedora or CentOS system. Fedora bug report: https://bugzilla.redhat.com/show_bug.cgi?id=475201

Works on Debian as well:

% cat /proc/$(ps —User puppet -o pid | tail -n 1)/status | grep Group
Groups: 0

Looks like Fedora released an update for this, perhaps it will get a CVE?

#15 Updated by Luke Kanies about 5 years ago

  • Status changed from Needs Decision to In Topic Branch Pending Review
  • Assignee deleted (Luke Kanies)

#16 Updated by Till Maas almost 5 years ago

I just wrote a mail to mitre to get a CVE number assigned.

#17 Updated by Till Maas almost 5 years ago

I got this number for this issue: CVE-2009-3564

#18 Updated by James Turnbull almost 5 years ago

Till – can you rebase your latest patch please against master.

#19 Updated by James Turnbull almost 5 years ago

Till – ignore that I’ve sent your updated patch to the puppet-dev list.

#20 Updated by James Turnbull almost 5 years ago

  • Status changed from In Topic Branch Pending Review to Closed

#21 Updated by James Turnbull almost 5 years ago

  • Status changed from Closed to Needs More Information
  • Assignee set to James Turnbull

Waiting some feedback on the -dev list.

#22 Updated by James Turnbull almost 5 years ago

  • Status changed from Needs More Information to Closed
  • Target version changed from 2.6.0 to 0.25.2

Pushed in commit:e32f980fd7c6291abc2841ede397c962798d9a9c in branch 0.25.x.

Also available in: Atom PDF