'Permission denied' on get with use_sudo=True

[OddBloke]

I am using Fabric against a fresh Ubuntu host, and trying to pull down /var/log in its entirety. I am hitting a 'Permission denied' error when calling get('/var/log', '...', use_sudo=True) because of a slightly convoluted Unix permissions problem.

/var/log/syslog is owned by syslog:adm, and has permissions of 640. When get copies it to its temporary location, it changes its permissions to 404 (but doesn't change ownership at all).

When a file has permissions of 404 (-r-----r--), it seems that being in the group that owns the file but not the user that owns the file means that you can't read the file (even though "others" can). (Presumably each permission is checked in turn, "group can't read" must result in a "permission denied" without even looking at the "others" permission). I have fabric paused in a debugger here and am looking at the actual temporary file:

$ whoami
ubuntu
$ groups
ubuntu adm dialout cdrom floppy sudo audio dip video plugdev netdev
$ ls -lah 9c689b24596429d276c11545d27fe2a17dd9299a
-r-----r-- 1 syslog adm 46K Dec  3 11:19 9c689b24596429d276c11545d27fe2a17dd9299a
$ head -n1 9c689b24596429d276c11545d27fe2a17dd9299a 
head: cannot open '9c689b24596429d276c11545d27fe2a17dd9299a' for reading: Permission denied
$ sudo -u www-data head -n1 9c689b24596429d276c11545d27fe2a17dd9299a 
Dec  3 11:18:48 ubuntu rsyslogd: [origin software="rsyslogd" swVersion="7.4.4" x-pid="1057" x-info="http://www.rsyslog.com"] start

I reckon this could be fixed by either (a) making the temporary files group-readable, or (b) changing the ownership of the temporary files to the user performing the operation.

[bitprophet]

I remember a very similar issue with put+sudo but I am not sure the exact permission wrinkle was the same. So there's probably two things we can look at here:

• See if the code patched in to address that put version of the issue, has just not been applied to the get side of things.
• If it has, then yes, we need to figure out what further workaround is needed (leaning towards the user change, since user info doesn't make sense to preserve during transit but mode does - so making the mode change would require an undo post-download. not hard, just more complicated).
  • This is a complicated feature area so any solution needs to be done with care lest we just add more bugs for another use case.

[cmattoon]

On my first attempt, I could not reproduce this error, however it was reproducible on a fresh Ubuntu 14.04 server install for certain files.

When a user ubuntu (who is in the adm group) moves a file with 640 permissions, access is granted based on the group. Setting the permissions to 0404 revokes group access and makes the file unavailable to the user, and appears to have been introduced here. The comment reads:

Only root and the user has the right to read the file

Even with the correct permissions of 0400 (unless I'm missing something), the file would end up out of reach, since cp -p preserves ownership by default. Since this section of the code is working with a temp file and not the source, it seems safe to change the ownership to env.user. I'm not overly familiar with the code, so I'm not sure if env.user is safe by itself (it's what I've always accessed in scripting).

I've attached a patch, just in case.

Debug Info:

(on a new Ubuntu 14.04 VM)

ubuntu@test-host:~$ whoami
ubuntu
ubuntu@test-host:~$ groups
ubuntu adm dialout cdrom floppy sudo audio dip video plugdev netde
ubuntu@test-host:~# ll /var/|grep log
drwxrwxr-x  8 root syslog 4096 Dec 12 20:45 log

While iterating through the files, the first 'permission denied' error is raised with dmesg.0:

-rw-r-----  1 root      adm         0 Oct 17 16:23 dmesg.0

The next file was /var/log/fsck/checkfs:

ubuntu@test-host:~$ ll /var/log/|grep fsck
drwxr-xr-x  2 root      root     4096 Apr 17  2014 fsck/
ubuntu@test-host:~$ ll /var/log/fsck/
total 8
drwxr-xr-x 2 root root   4096 Apr 17  2014 ./
drwxrwxr-x 8 root syslog 4096 Dec 12 20:45 ../
-rw-r----- 1 root adm       0 Apr 18  2014 checkfs
-rw-r----- 1 root adm       0 Apr 18  2014 checkroot
ubuntu@test-host:~$ 

In the second instance (fsck), the traceback was:

Traceback (most recent call last):
  File "/usr/local/lib/python2.7/dist-packages/Fabric-1.10.0-py2.7.egg/fabric/operations.py", line 574, in get
    result = ftp.get_dir(remote_path, local_path, use_sudo, temp_dir)
  File "/usr/local/lib/python2.7/dist-packages/Fabric-1.10.0-py2.7.egg/fabric/sftp.py", line 228, in get_dir
    result.append(self.get(rpath, lpath, use_sudo, True, rremote, temp_dir))
  File "/usr/local/lib/python2.7/dist-packages/Fabric-1.10.0-py2.7.egg/fabric/sftp.py", line 181, in get
    getter(remote_path, local_path)
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 676, in get
    size = self.getfo(remotepath, fl, callback)
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 639, in getfo
    fr = self.file(remotepath, 'rb')
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 245, in open
    t, msg = self._request(CMD_OPEN, filename, imode, attrblock)
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 689, in _request
    return self._read_response(num)
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 736, in _read_response
    self._convert_status(msg)
  File "/usr/lib/python2.7/dist-packages/paramiko/sftp_client.py", line 764, in _convert_status
    raise IOError(errno.EACCES, text)
IOError: [Errno 13] Permission denied

[bitprophet]

Github doesn't parse refs in headers so just noting that #1235 is the PR for this.

[bitprophet]

Re: your PR, my only worry is that if you/we (I can't keep all this stuff straight anymore XD) are incorrect about the scope of the s/0404/0400/ change, this might break the original use case which caused the original change.

One of us needs to go find the PR that added this to see if there's more info about how to replicate that use case - presumably whoever added it found that it did function correctly in some situation(s).

[cmattoon]

Here's the commit I found, and it looks like it was added as part of the overall task.

I might be wrong here, so I'll explain my thinking in case anyone sees a problem:

Setting 0404 simply bypasses the intent of "only root and the user" accessing the file. It's readable by user and world, which doesn't do what the comment says. Being more permissive with the permissions makes me wonder if it was a typo, or if there are instances where env.user doesn't match up with the user that's running the process. I changed it to '0400' to match the intent of the comment, based on some assumptions:

• env.user is an accurate source of information about the user
• The process is running as env.user
• No setuid-type weirdness occurs that would ever cause the process to be running under someone other than env.user

With ownership otheruser:somegroup, chmod 0400 would lock the process out potentially. (I don't know off the top of my head how changing permissions with an open file descriptor or something would work, but it definitely seems to have potential to cause problems!) By changing the ownership to user:user before changing permissions to 0400, the process/user retains access while blocking everyone else.

[bitprophet]

That commit seems to have come out of the original #700 which doesn't really list anything interesting besides "make it work". I guess that's something.

Your analysis looks sound to me, I was simply being paranoid about somebody going "hi yes this only works if I make the file world readable". But barring that I agree it's most secure (and consistent) to use 0400.

I'm gonna try whipping up an integration test for some of these cases (all we seem to have is the ones for put()) and test out the PR with those in place.

[bitprophet]

I've identified one problem with the fix, not all usernames are necessarily reflected as groups, that's just happenstance on some systems. E.g. when testing against a work-related VM, where users do not have their own groups, I get an error when the PR's code tries to do chown jforcier:jforcier.

I don't see a huge benefit to chowning the group as well, honestly (just user should be sufficient to work around the original problem reported by @OddBloke), and not doing so would skip this problem, so I'll try tweaking it that way.

[bitprophet]

With that change in, the test I wrote which failed prior to merging #1235, passes \o/

[bitprophet]

Couple of non-integration tests for same functionality fail, updated them to account for new sub-call (honestly...not a fan of this kind of test and clearly I didn't add them...but not worth removing now) and things look good on my end. Merging to 1.10 and letting travis have a crack.

[bitprophet]

https://travis-ci.org/fabric/fabric/builds/44377867

[cmattoon]

Awesome! Yeah, I guess the group isn't relevant with 400 permissions..d'oh! Third time's a charm, they say...