[rdiff-backup-users] SECURITY: Not all file ops accessed via vetted RPath objects? Also a path prefixing patch

[Charles Duffy]

While trying to implement the previously-discussed feature (adding a 
prefix to all remotely-provided paths in server mode), I've come upon 
what appears to be a bug in the server-mode security model.

Tue Aug 16 00:24:51 2005  Server sending (0): None
Tue Aug 16 00:24:51 2005  Client received (0): None
Tue Aug 16 00:24:51 2005  Making directory backup-files
Tue Aug 16 00:24:51 2005  Client sending (0): ConnectionRequest: 
os.mkdir with 1 arguments
Tue Aug 16 00:24:51 2005  Client sending (0): 'backup-files'
Tue Aug 16 00:24:51 2005  Server received (0): ConnectionRequest: 
os.mkdir with 1 arguments
Tue Aug 16 00:24:51 2005  Server received (0): 'backup-files'
Vetting request (ConnectionRequest: os.mkdir with 1 arguments), 
['backup-files']
Not vetting backup-files against restricted path list

The last few lines are from my own instrumentation -- but nonetheless, 
it appears quite clearly that 'backup-files' is in this case passed as 
type str rather than as an RPath object, and thus that it never makes it 
to Security.vet_rpath(). This appears to be the case with other requests 
as well -- so far os.mkdir, os.listdir, os.chmod and C.make_file_dict, 
though I do not pretend to know of the exhaustiveness of the above list. 
This strikes me as a rather Bad Thing.

---------------

Going back to the path-prefixing feature, I have a patch that appears to 
work under *very light* testing, but it's just intended as a 
proof-of-concept -- I have very little confidence in its correctness. 
Ben, I would very much appreciate any input you could provide. I would 
like to get this deployed to my QA department quickly, and will gladly 
spend time writing code as needed to do so -- but some guidance and 
advice would be exceedingly helpful.

Thank you kindly!

diff -ru rdiff-backup-1.0.0/rdiff_backup/Globals.py 
rdiff-backup-1.0.0/rdiff_backup.new/Globals.py
--- rdiff-backup-1.0.0/rdiff_backup/Globals.py  2005-08-14 01:12:55.000000000 
-0500
+++ rdiff-backup-1.0.0/rdiff_backup.new/Globals.py      2005-08-15 
11:55:14.000000000 -0500
@@ -215,6 +215,9 @@
 # If set, exit with error instead of dropping ACLs or ACL entries.
 never_drop_acls = None
 
+# If running as a server, append this prefix to all paths used.
+path_prefix = ""
+
 
 def get(name):
        """Return the value of something in this module"""
diff -ru rdiff-backup-1.0.0/rdiff_backup/Main.py 
rdiff-backup-1.0.0/rdiff_backup.new/Main.py
--- rdiff-backup-1.0.0/rdiff_backup/Main.py     2005-08-14 01:12:55.000000000 
-0500
+++ rdiff-backup-1.0.0/rdiff_backup.new/Main.py 2005-08-15 23:55:12.000000000 
-0500
@@ -62,7 +62,7 @@
                  "exclude-filelist-stdin", "exclude-globbing-filelist=",
                  "exclude-globbing-filelist-stdin", "exclude-mirror=",
                  "exclude-other-filesystems", "exclude-regexp=",
-                 "exclude-special-files", "force", "group-mapping-file=",
+                 "exclude-special-files", "force", "force-path-prefix=", 
"group-mapping-file=",
                  "include=", "include-filelist=", "include-filelist-stdin",
                  "include-globbing-filelist=",
                  "include-globbing-filelist-stdin", "include-regexp=",
@@ -115,6 +115,7 @@
                                                                "standard 
input"))
                        select_files.append(sys.stdin)
                elif opt == "--force": force = 1
+               elif opt == "--force-path-prefix": Globals.path_prefix = 
normalize_path(arg)
                elif opt == "--group-mapping-file": group_mapping_filename = arg
                elif (opt == "--include" or
                          opt == "--include-special-files" or
diff -ru rdiff-backup-1.0.0/rdiff_backup/Security.py 
rdiff-backup-1.0.0/rdiff_backup.new/Security.py
--- rdiff-backup-1.0.0/rdiff_backup/Security.py 2005-08-14 01:12:55.000000000 
-0500
+++ rdiff-backup-1.0.0/rdiff_backup.new/Security.py     2005-08-16 
00:55:24.426435000 -0500
@@ -21,6 +21,7 @@
 
 import sys, tempfile
 import Globals, Main, rpath, log
+import os.path
 
 class Violation(Exception):
        """Exception that indicates an improper request has been received"""
@@ -177,11 +178,20 @@
 
 def vet_request(request, arglist):
        """Examine request for security violations"""
-       #if Globals.server: sys.stderr.write(str(request) + "\n")
+       #if Globals.server: sys.stderr.write("Vetting request (%s), %s [%s]\n" 
% (str(request), str(arglist), repr([type(arg) for arg in arglist])))
        security_level = Globals.security_level
+       if Globals.path_prefix:
+               for arg in arglist:
+                       if isinstance(arg, rpath.RPath):
+                               #if Globals.server: sys.stderr.write("Adding 
prefix to RPath (%s,%s)\n" % (repr(arg.base), repr(arg.path)))
+                               arg.base = os.path.join(Globals.path_prefix, 
arg.base)
+                               arg.path = os.path.join(Globals.path_prefix, 
arg.path)
+                       elif isinstance(arg, str) and request.function_string 
in ['os.mkdir', 'os.listdir', 'os.chmod', 'C.make_file_dict']:
+                               arglist[arglist.index(arg)] = 
os.path.join(Globals.path_prefix, arg)
        if Globals.restrict_path:
                for arg in arglist:
                        if isinstance(arg, rpath.RPath): vet_rpath(arg)
+                       #elif isinstance(arg, str): sys.stderr.write("Not 
vetting %s against restricted path list\n" % arg)
        if security_level == "all": return
        if request.function_string in allowed_requests: return
        if request.function_string in ("Globals.set", "Globals.set_local"):