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

Permissions should also be needed for RNTO #150

Closed
giampaolo opened this issue May 28, 2014 · 9 comments
Closed

Permissions should also be needed for RNTO #150

giampaolo opened this issue May 28, 2014 · 9 comments
Assignees
Labels
bug Component-Library imported imported from old googlecode site and very likely outdated Security

Comments

@giampaolo
Copy link
Owner

From alexande...@googlemail.com on January 12, 2011 12:41:21

What steps will reproduce the problem?  
1. rename file /tmp/file 

What is the expected output?  


What do you see instead?  
That command will move the file from the users directory to a directory outside 
the users scope (/tmp) as long as it is on the same physical filesystem. When 
permissions were needed then ftp2fs would be called first. What version of 
pyftpdlib are you using? On what operating system? Which Python version? trunk 
r807 , Linux, Python 2.5 

Please provide any additional information below.  
I also need this, because I implemented an authorizer that gives permissions on 
a file-by-file basis. And currently has_perm() is not called for the 
destination file, so I can't check whether the user is allowed to write to that 
filename. (It would be nice if RNTO gets another letter than 'f', so I can 
differentiate between a file that is read and deleted and a file that will be created.)

Original issue: http://code.google.com/p/pyftpdlib/issues/detail?id=150

@giampaolo giampaolo self-assigned this May 28, 2014
@giampaolo
Copy link
Owner Author

From g.rodola on January 12, 2011 05:20:02

I think there's nothing wrong with requiring "f" permission also for RNTO other 
than just RNFR:

-    'RNTO' : dict(perm=None, ...
+    'RNTO' : dict(perm='f', ...

I don't think RNTO alone is worth a separate permission letter though. 
"f" for both RNFR and RNTO is just fine.

Status: Accepted
Labels: -Type-Defect Type-Enhancement Version-0.5.2 Milestone-0.6.0
Usability

@giampaolo
Copy link
Owner Author

From alexande...@googlemail.com on January 12, 2011 05:41:06

How can I then differentiate between RNTO and RNFR? From my authorizer's point 
of view they are entirely different operations (delete vs. create). Maybe 
has_perm() could also get the original FTP command, so a more fine grained 
control without additional letters is possible?

@giampaolo
Copy link
Owner Author

From g.rodola on January 14, 2011 11:45:00

It's not clear to me why you need to differentiate between the 2 commands.
Could you explain your use case better?

> How can I then differentiate between RNTO and RNFR? 

As for now you can do this by overriding FTPHandler.process_command method.
Not very elegant but...

@giampaolo
Copy link
Owner Author

From g.rodola on January 14, 2011 12:54:05

RNTO now requires 'f' permission as r809 .
This should be enough for fixing the path traversal issue.

Status: FixedInSVN
Labels: -Type-Enhancement -Usability Type-Defect Security

@giampaolo
Copy link
Owner Author

From alexande...@googlemail.com on January 14, 2011 14:19:00

> It's not clear to me why you need to differentiate between the 2 commands.
> Could you explain your use case better?

I'm building on top of an already existing access control system. And this 
system differentiates between creating a file, creating a directory, writing a 
file, deleting a file, deleting a directory and much more. So there I need that 
distinction.

There are also illegal filenames: filenames with certain characters that are 
not allowed to be created. But if they somehow exist, it should be possible to 
delete them (but not to modify them). So there I also need to differentiate 
between both operations.

@giampaolo
Copy link
Owner Author

From g.rodola on January 14, 2011 14:40:24

Ok understood. 
But again, I don't think this use case worths a separate permission letter.
Plus, it would also be a bit confusing (what if user grants permission for RNFR 
but not for RNTO?).
RNFR and RNTO should be seen as a single command, like "rm".
The fact that in FTP protocol they are implemented as two separate commands 
should not affect the upper API, in my opinion.
If you need such a level of control you must operate against the handler (and 
given this kind of customization it's likely you already have your own 
FTPHandler subclass):


from pyftpdlib import ftpserver

def is_valid_path(path):
    # logic goes here
    return False


class CustomizedHandler(ftpserver.FTPHandler):

    def process_command(self, cmd, *args, **kwargs):
        if cmd == "RNTO":
            path = args[0]
            if not is_valid_path(path):
                self.respond("550 not a valid destination path")
                return
        ftpserver.FTPHandler.process_command(self, cmd, args, kwargs)

@giampaolo
Copy link
Owner Author

From g.rodola on January 14, 2011 14:43:24

> RNFR and RNTO should be seen as a single command, like "rm".

Obviously I meant "mv".

@giampaolo
Copy link
Owner Author

From alexande...@googlemail.com on January 14, 2011 15:10:20

Okay, thanks.

I think I will implement my own FTPHandler that'll put its own set of letters 
into the proto_cmds dictionary. So I'll have the interface to the access 
control system in one place (the authorizer) and don't need to check RNTO and 
RNFR a second time, it's much more cleaner that way. (And can also easily 
differentiate between DELE and RMD.)

@giampaolo
Copy link
Owner Author

From g.rodola on January 23, 2011 12:59:22

Implemented in version 0.6.0.

Status: Fixed
Owner: g.rodola

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Component-Library imported imported from old googlecode site and very likely outdated Security
Projects
None yet
Development

No branches or pull requests

1 participant