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

Security problem: upload_template () ==> world readable file in the home folder #1341

Closed
indera opened this Issue Jun 11, 2015 · 3 comments

Comments

Projects
None yet
4 participants
@indera

indera commented Jun 11, 2015

I wast running the upload_template() function from the fabric.contrib.files package and found an unexpected behavior.
If the "destination" parameter points to an invalid path then you end up with a world-readable file in your home folder:

-rw-r--r-- 1 me users 623 Jun 11 12:02 dbdc6b14139b9aaf18cfcd2cb1244440dbf08136

If the file happens to contain a password there is a chance of the somebody reading it.

@indera

This comment has been minimized.

indera commented Jun 11, 2015

I would expect that if
upload_template()
function can't complete the action then it should delete the temporary file in the home folder or set the permission to something like 600.

@banjocat

This comment has been minimized.

banjocat commented Nov 8, 2016

I was able to reproduce this with the below (fabric 1.12.0)

import sys

from fabric.api import hosts, env, run
from fabric.contrib.files import upload_template

env.user = 'root'


@hosts('localhost')
def tryit():
    run('ls')
    try:
        upload_template('./test.j2', '/lol/.', use_sudo=True)
    except:
        run('ls')
        print("Error %s", sys.exc_info()[0])

It created a file called 20c0b8892060448bc203c84a79c9299b72002a8e. The contents of the file was the template file. It did throw an exception
The use_sudo=True was required to see this error. Without it, it would fail as expected.
I tested it with user=root and user=banjocat. Problem existed in both.

Full output of fabric

[localhost] Executing task 'tryit'
[localhost] run: ls
[localhost] out: Desktop  wakeup~  wakeuy~  wakeuz~
[localhost] out: 

[localhost] put: <file obj> -> /lol/.

Fatal error: sudo() received nonzero return code 1 while executing!

Requested: mv "20c0b8892060448bc203c84a79c9299b72002a8e" "/lol/."
Executed: sudo -S -p 'sudo password:'  /bin/bash -l -c "mv \"20c0b8892060448bc203c84a79c9299b72002a8e\" \"/lol/.\""

============================================================================ Standard output ============================================================================

mv: cannot move '20c0b8892060448bc203c84a79c9299b72002a8e' to '/lol/.': No such file or directory

==========================================================================================================================================================================

Aborting.
[localhost] run: ls
[localhost] out: 20c0b8892060448bc203c84a79c9299b72002a8e  Desktop  wakeup~  wakeuy~  wakeuz~
[localhost] out: 

('Error %s', <type 'exceptions.SystemExit'>)

Done.
Disconnecting from localhost... done.
@UkuLoskit

This comment has been minimized.

UkuLoskit commented Apr 1, 2017

The issue is not specific to upload_template, but to fabric.sftp.SFTP.put which is used by upload_template.
You can replace upload_template with put (that is operations.put) in @banjocat 's example, and the very same thing will happen. The issue only applies to use-cases with use_sudo=True

The issue appears to be here:
https://github.com/fabric/fabric/blob/master/fabric/sftp.py#L286-L290

For an invalid destination path, the mv command will fail and the temporary file will never be removed.

I propose the following approach which will clean up after itself in case of failure

diff --git a/fabric/sftp.py b/fabric/sftp.py
index c530f4a1..252745de 100644
--- a/fabric/sftp.py
+++ b/fabric/sftp.py
@@ -287,7 +287,11 @@ class SFTP(object):
             # Temporarily nuke 'cwd' so sudo() doesn't "cd" its mv command.
             # (The target path has already been cwd-ified elsewhere.)
             with settings(hide('everything'), cwd=""):
-                sudo("mv \"%s\" \"%s\"" % (remote_path, target_path))
+                try:
+                    sudo("mv \"%s\" \"%s\"" % (remote_path, target_path))
+                except BaseException:
+                    sudo("rm -f \"%s\"" % remote_path)
+                    raise
             # Revert to original remote_path for return value's sake
             remote_path = target_path
         return remote_path

@bitprophet what do you think of this approach?

UkuLoskit added a commit to UkuLoskit/fabric that referenced this issue Apr 7, 2017

@bitprophet bitprophet closed this Nov 27, 2018

bitprophet added a commit that referenced this issue Nov 27, 2018

ploxiln added a commit to ploxiln/fab-classic that referenced this issue Nov 28, 2018

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