update mode if group or owner change to keep suid bit #2967

Merged
merged 2 commits into from May 5, 2015

Conversation

Projects
None yet
6 participants
@minshallj
Contributor

minshallj commented Feb 22, 2015

On Linux updating the group or owner unsets the suid bit for security
reasons, so check for group and owner updates whether or not to set the
mode.

This is in reference to issue #2951

@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj Feb 23, 2015

Contributor

Is the appveyor failure my fault? I would doubt it because it fails very early, it it's a failure to install a gem. Plus it looks like it's on windows and all of my changes were under unix only areas.

Contributor

minshallj commented Feb 23, 2015

Is the appveyor failure my fault? I would doubt it because it fails very early, it it's a failure to install a gem. Plus it looks like it's on windows and all of my changes were under unix only areas.

@nathenharvey

This comment has been minimized.

Show comment
Hide comment
@nathenharvey

nathenharvey Feb 23, 2015

Member

The AppVeyor failure looks like an actual failure with the file_access_control_spec.rb. Can you please look into that and update your code or the tests appropriately?

Member

nathenharvey commented Feb 23, 2015

The AppVeyor failure looks like an actual failure with the file_access_control_spec.rb. Can you please look into that and update your code or the tests appropriately?

@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj Feb 24, 2015

Contributor

I can try looking into this, but I don't have a windows machine to test on, and when I bundle exec rspec ./spec/unit/file_access_control_spec.rb all tests pass. I do see the error in the test now though, so when I have time I'll try pushing some commits to fix it.

Contributor

minshallj commented Feb 24, 2015

I can try looking into this, but I don't have a windows machine to test on, and when I bundle exec rspec ./spec/unit/file_access_control_spec.rb all tests pass. I do see the error in the test now though, so when I have time I'll try pushing some commits to fix it.

@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj Feb 24, 2015

Contributor

So looking around at this failure I noticed

  1. There was a problem with my boolean logic, which I am pushing a patch for now
  2. The resource being created didn't have a user or group. I am wondering if this is only something that might happen in a test case since we are really just testing to see if the mode should be changed here.

Pushing the commit now, and it might fix this break because of the boolean logic (once it notices the suid bit is not set it will no longer check the conditions on the far right) The root cause of this failure though is that a user/group should be set when checking if a resource should have it's mode changed now.

Contributor

minshallj commented Feb 24, 2015

So looking around at this failure I noticed

  1. There was a problem with my boolean logic, which I am pushing a patch for now
  2. The resource being created didn't have a user or group. I am wondering if this is only something that might happen in a test case since we are really just testing to see if the mode should be changed here.

Pushing the commit now, and it might fix this break because of the boolean logic (once it notices the suid bit is not set it will no longer check the conditions on the far right) The root cause of this failure though is that a user/group should be set when checking if a resource should have it's mode changed now.

@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj Mar 8, 2015

Contributor

Ping.

I guess some questions that might still remain about this patch are, "Is this the best way to check for the suid bit?", "This isn't needed on macs, can we avoid doing this there", and lastly "can we suppress the output of 'changed mode from 04755 to 04755' since it seems redundant".

  1. Well, the mode as a string will always have the owner, group, and global permissions as the last three characters of the string respectively. Thus if the suid bit is set, the fourth character from the right would be something other than 0. I'm not totally sure if the mode as a string will always be 4+ characters long, if it has the possibility of being < 3 characters, the suid check might want to look like return mode_to_s(target_mode) and mode_to_s(target_mode)[-4] != '0'. I can definitely add that in if requested.
  2. This shouldn't be hard, though I'm not totally sure this isn't needed on macs, and that it won't be in the future. Something like !(/darwin/ =~ RUBY_PLATFORM) could be thrown in the if statement, again easy to accomplish if desired.
  3. No idea how to go about this. Is there some way to suppress the output?
Contributor

minshallj commented Mar 8, 2015

Ping.

I guess some questions that might still remain about this patch are, "Is this the best way to check for the suid bit?", "This isn't needed on macs, can we avoid doing this there", and lastly "can we suppress the output of 'changed mode from 04755 to 04755' since it seems redundant".

  1. Well, the mode as a string will always have the owner, group, and global permissions as the last three characters of the string respectively. Thus if the suid bit is set, the fourth character from the right would be something other than 0. I'm not totally sure if the mode as a string will always be 4+ characters long, if it has the possibility of being < 3 characters, the suid check might want to look like return mode_to_s(target_mode) and mode_to_s(target_mode)[-4] != '0'. I can definitely add that in if requested.
  2. This shouldn't be hard, though I'm not totally sure this isn't needed on macs, and that it won't be in the future. Something like !(/darwin/ =~ RUBY_PLATFORM) could be thrown in the if statement, again easy to accomplish if desired.
  3. No idea how to go about this. Is there some way to suppress the output?
lib/chef/file_access_control/unix.rb
@@ -280,6 +282,9 @@ def uid_from_resource(resource)
return nil
end
+ def suid_bit_set
+ return mode_to_s(target_mode)[-4] != '0'

This comment has been minimized.

@bahamas10

bahamas10 Mar 17, 2015

Contributor

this means the setuid, setgid, or the sticky-bit (or a combination of them) is set... if you are only concerned about the setuid bit you should check for it explicitly (also without casting to a string)

def suid_bit_set
  target_mode & 04000 > 0
end
@bahamas10

bahamas10 Mar 17, 2015

Contributor

this means the setuid, setgid, or the sticky-bit (or a combination of them) is set... if you are only concerned about the setuid bit you should check for it explicitly (also without casting to a string)

def suid_bit_set
  target_mode & 04000 > 0
end

minshallj added some commits Feb 22, 2015

update mode if group or owner change to keep suid bit
On Linux updating the group or owner unsets the suid bit for security
reasons, so check for group and owner updates whether or not to set the
mode.
@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj Mar 17, 2015

Contributor

@bahamas10 ok, made that change and squashed the non test related commits into one

Contributor

minshallj commented Mar 17, 2015

@bahamas10 ok, made that change and squashed the non test related commits into one

@minshallj

This comment has been minimized.

Show comment
Hide comment
@minshallj

minshallj May 4, 2015

Contributor

@nathenharvey, any opposition to merging this?

Contributor

minshallj commented May 4, 2015

@nathenharvey, any opposition to merging this?

@thommay

This comment has been minimized.

Show comment
Hide comment
Contributor

thommay commented May 5, 2015

@danielsdeleo

This comment has been minimized.

Show comment
Hide comment
@danielsdeleo

danielsdeleo May 5, 2015

Member

👍 as long as we fix up the indentation on merge.

Member

danielsdeleo commented May 5, 2015

👍 as long as we fix up the indentation on merge.

@thommay thommay merged commit 7204f72 into chef:master May 5, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chef chef locked and limited conversation to collaborators Nov 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.