From 420600f59943b8fd20a3cc6853b1d6e7a1bb2bb1 Mon Sep 17 00:00:00 2001 From: bescoto Date: Sat, 20 Aug 2005 06:06:07 +0000 Subject: Fix for bug 14209 (security violation with --restrict) git-svn-id: http://svn.savannah.nongnu.org/svn/rdiff-backup/trunk@625 2b77aa54-bcbc-44c9-a7ec-4f6cf2b41109 --- rdiff-backup/CHANGELOG | 5 + rdiff-backup/rdiff_backup/Security.py | 166 +++++++++++++++++++-------------- rdiff-backup/testing/regressiontest.py | 4 +- rdiff-backup/testing/securitytest.py | 16 +++- rdiff-backup/testing/server.py | 4 +- 5 files changed, 121 insertions(+), 74 deletions(-) diff --git a/rdiff-backup/CHANGELOG b/rdiff-backup/CHANGELOG index b721602..00e75af 100644 --- a/rdiff-backup/CHANGELOG +++ b/rdiff-backup/CHANGELOG @@ -15,6 +15,11 @@ Kevin Spicer). fsync_directories defaults to None, to avoid errors in testing (suggestion by Charles Duffy). +bug#14209: Security bug with --restrict-read-only and +--restrict-update-only allowed file statting and directory listing +outside path. Bug with --restrict option allowed writes outside path. +(Reported by Charles Duffy.) + New in v1.0.0 (2005/08/14) -------------------------- diff --git a/rdiff-backup/rdiff_backup/Security.py b/rdiff-backup/rdiff_backup/Security.py index 57e3d2f..efd8f04 100644 --- a/rdiff-backup/rdiff_backup/Security.py +++ b/rdiff-backup/rdiff_backup/Security.py @@ -19,7 +19,7 @@ """Functions to make sure remote requests are kosher""" -import sys, tempfile +import sys, tempfile, types import Globals, Main, rpath, log class Violation(Exception): @@ -35,6 +35,23 @@ allowed_requests = None # set on the server. disallowed_server_globals = ["server", "security_level", "restrict_path"] +# Some common file commands we may want to check to make sure they are +# in the right directory. Any commands accessing files that could be +# added to allowed_requests must be here. A few others have also been +# added---this is not a intended to be a complete list of course, just +# some support for the depreciated --restrict option when the +# security_level is "all" so you don't accidentally step out of the +# directory. +# +# The keys are files request, the value is the index of the argument +# taking a file. +file_requests = {'os.listdir':0, 'C.make_file_dict':0, 'os.chmod':0, + 'os.chown':0, 'os.remove':0, 'os.removedirs':0, + 'os.rename':0, 'os.renames':0, 'os.rmdir':0, 'os.unlink':0, + 'os.utime':0, 'os.lchown':0, 'os.link':1, 'os.symlink':1, + 'os.mkdir':0, 'os.lchown':0} + + def initialize(action, cmdpairs): """Initialize allowable request list and chroot""" global allowed_requests @@ -109,71 +126,71 @@ def set_security_level(action, cmdpairs): def set_allowed_requests(sec_level): """Set the allowed requests list using the security level""" global allowed_requests - if sec_level == "all": return - allowed_requests = ["VirtualFile.readfromid", "VirtualFile.closebyid", - "Globals.get", "Globals.is_not_None", - "Globals.get_dict_val", - "log.Log.open_logfile_allconn", - "log.Log.close_logfile_allconn", - "Log.log_to_file", - "FilenameMapping.set_init_quote_vals_local", - "SetConnections.add_redirected_conn", - "RedirectedRun", - "sys.stdout.write", - "robust.install_signal_handlers"] - if sec_level == "minimal": pass - elif sec_level == "read-only" or sec_level == "update-only": - allowed_requests.extend( - ["C.make_file_dict", - "rpath.ea_get", - "rpath.acl_get", - "rpath.setdata_local", - "log.Log.log_to_file", - "os.getuid", - "os.listdir", - "Time.setcurtime_local", - "rpath.gzip_open_local_read", - "rpath.open_local_read", - "Hardlink.initialize_dictionaries", - "user_group.uid2uname", - "user_group.gid2gname"]) - if sec_level == "read-only": - allowed_requests.extend( - ["fs_abilities.get_fsabilities_readonly", - "fs_abilities.get_fsabilities_restoresource", - "restore.MirrorStruct.set_mirror_and_rest_times", - "restore.MirrorStruct.initialize_rf_cache", - "restore.MirrorStruct.close_rf_cache", - "restore.MirrorStruct.get_diffs", - "backup.SourceStruct.get_source_select", - "backup.SourceStruct.set_source_select", - "backup.SourceStruct.get_diffs"]) - elif sec_level == "update-only": - allowed_requests.extend( - ["log.Log.open_logfile_local", "log.Log.close_logfile_local", - "log.ErrorLog.open", "log.ErrorLog.isopen", - "log.ErrorLog.close", - "backup.DestinationStruct.set_rorp_cache", - "backup.DestinationStruct.get_sigs", - "backup.DestinationStruct.patch_and_increment", - "Main.backup_touch_curmirror_local", - "Main.backup_remove_curmirror_local", - "Globals.ITRB.increment_stat", - "statistics.record_error", - "log.ErrorLog.write_if_open", - "fs_abilities.get_fsabilities_readwrite"]) + l = ["VirtualFile.readfromid", "VirtualFile.closebyid", + "Globals.get", "Globals.is_not_None", "Globals.get_dict_val", + "log.Log.open_logfile_allconn", "log.Log.close_logfile_allconn", + "Log.log_to_file", "FilenameMapping.set_init_quote_vals_local", + "SetConnections.add_redirected_conn", "RedirectedRun", + "sys.stdout.write", "robust.install_signal_handlers"] + if (sec_level == "read-only" or sec_level == "update-only" or + sec_level == "all"): + l.extend(["C.make_file_dict", "os.listdir", "rpath.ea_get", + "rpath.acl_get", "rpath.setdata_local", + "log.Log.log_to_file", "os.getuid", "Time.setcurtime_local", + "rpath.gzip_open_local_read", "rpath.open_local_read", + "Hardlink.initialize_dictionaries", "user_group.uid2uname", + "user_group.gid2gname"]) + if sec_level == "read-only" or sec_level == "all": + l.extend(["fs_abilities.get_fsabilities_readonly", + "fs_abilities.get_fsabilities_restoresource", + "restore.MirrorStruct.set_mirror_and_rest_times", + "restore.MirrorStruct.set_mirror_select", + "restore.MirrorStruct.initialize_rf_cache", + "restore.MirrorStruct.close_rf_cache", + "restore.MirrorStruct.get_diffs", + "restore.ListChangedSince", + "restore.ListAtTime", + "backup.SourceStruct.get_source_select", + "backup.SourceStruct.set_source_select", + "backup.SourceStruct.get_diffs"]) + if sec_level == "update-only" or sec_level == "all": + l.extend(["log.Log.open_logfile_local", "log.Log.close_logfile_local", + "log.ErrorLog.open", "log.ErrorLog.isopen", + "log.ErrorLog.close", + "backup.DestinationStruct.set_rorp_cache", + "backup.DestinationStruct.get_sigs", + "backup.DestinationStruct.patch_and_increment", + "Main.backup_touch_curmirror_local", + "Main.backup_remove_curmirror_local", + "Globals.ITRB.increment_stat", + "statistics.record_error", + "log.ErrorLog.write_if_open", + "fs_abilities.get_fsabilities_readwrite"]) + if sec_level == "all": + l.extend(["os.mkdir", "os.chown", "os.lchown", "os.rename", + "os.unlink", "os.remove", "os.chmod", + "backup.DestinationStruct.patch", + "restore.TargetStruct.get_initial_iter", + "restore.TargetStruct.patch", + "restore.TargetStruct.set_target_select", + "regress.Regress"]) if Globals.server: - allowed_requests.extend( - ["SetConnections.init_connection_remote", - "log.Log.setverbosity", - "log.Log.setterm_verbosity", - "Time.setprevtime_local", - "Globals.postset_regexp_local", - "Globals.set_select", - "backup.SourceStruct.set_session_info", - "backup.DestinationStruct.set_session_info", - "user_group.init_user_mapping", - "user_group.init_group_mapping"]) + l.extend(["SetConnections.init_connection_remote", + "log.Log.setverbosity", "log.Log.setterm_verbosity", + "Time.setprevtime_local", "Globals.postset_regexp_local", + "Globals.set_select", "backup.SourceStruct.set_session_info", + "backup.DestinationStruct.set_session_info", + "user_group.init_user_mapping", + "user_group.init_group_mapping"]) + allowed_requests = {} + for req in l: allowed_requests[req] = None + +def raise_violation(request, arglist): + """Raise a security violation about given request""" + raise Violation("\nWarning Security Violation!\n" + "Bad request for function: %s\n" + "with arguments: %s\n" % (request.function_string, + arglist)) def vet_request(request, arglist): """Examine request for security violations""" @@ -182,15 +199,14 @@ def vet_request(request, arglist): if Globals.restrict_path: for arg in arglist: if isinstance(arg, rpath.RPath): vet_rpath(arg) - if security_level == "all": return + if request.function_string in file_requests: + vet_filename(request, arglist) + if security_level == "override": return if request.function_string in allowed_requests: return if request.function_string in ("Globals.set", "Globals.set_local"): if Globals.server and arglist[0] not in disallowed_server_globals: return - raise Violation("\nWarning Security Violation!\n" - "Bad request for function: %s\n" - "with arguments: %s\n" % (request.function_string, - arglist)) + raise_violation(request, arglist) def vet_rpath(rpath): """Require rpath not to step outside retricted directory""" @@ -207,3 +223,13 @@ def vet_rpath(rpath): "Request to handle path %s\n" "which doesn't appear to be within " "restrict path %s.\n" % (normalized, restrict)) + +def vet_filename(request, arglist): + """Check to see if file operation is within the restrict_path""" + i = file_requests[request.function_string] + if len(arglist) <= i: raise_violation(request, arglist) + filename = arglist[i] + if type(filename) is not types.StringType: + raise_violation(request, arglist) + vet_rpath(rpath.RPath(Globals.local_connection, filename)) + diff --git a/rdiff-backup/testing/regressiontest.py b/rdiff-backup/testing/regressiontest.py index 18ec1a3..427ed83 100644 --- a/rdiff-backup/testing/regressiontest.py +++ b/rdiff-backup/testing/regressiontest.py @@ -12,7 +12,9 @@ testfiles Globals.set('change_source_perms', 1) Globals.counter = 0 -log.Log.setverbosity(7) +Globals.security_level = "override" +log.Log.setverbosity(3) + def get_local_rp(extension): return rpath.RPath(Globals.local_connection, "testfiles/" + extension) diff --git a/rdiff-backup/testing/securitytest.py b/rdiff-backup/testing/securitytest.py index f8796ff..cd83635 100644 --- a/rdiff-backup/testing/securitytest.py +++ b/rdiff-backup/testing/securitytest.py @@ -1,4 +1,4 @@ -import os, unittest, time +import os, unittest, time, traceback, sys from commontest import * import rdiff_backup.Security as Security @@ -12,7 +12,10 @@ class SecurityTest(unittest.TestCase): problem. """ - assert isinstance(exc, Security.Violation), exc + if not isinstance(exc, Security.Violation): + type, value, tb = sys.exc_info() + print "".join(traceback.format_tb(tb)) + raise exc #assert str(exc).find("Security") >= 0, "%s\n%s" % (exc, repr(exc)) def test_vet_request_ro(self): @@ -188,6 +191,15 @@ class SecurityTest(unittest.TestCase): extra_args = '-r now', success = 0) + def test_restrict_bug(self): + """Test for bug 14209 --- mkdir outside --restrict arg""" + Myrm('testfiles/output') + self.secure_rdiff_backup('testfiles/various_file_types', + 'testfiles/output', 1, + '--restrict foobar', success = 0) + output = rpath.RPath(Globals.local_connection, 'testfiles/output') + assert not output.lstat() + if __name__ == "__main__": unittest.main() diff --git a/rdiff-backup/testing/server.py b/rdiff-backup/testing/server.py index b5d265d..4dc786f 100755 --- a/rdiff-backup/testing/server.py +++ b/rdiff-backup/testing/server.py @@ -23,10 +23,12 @@ if len(sys.argv) > 2: try: if len(sys.argv) == 2: sys.path.insert(0, sys.argv[1]) import rdiff_backup.Globals + import rdiff_backup.Security from rdiff_backup.connection import * except (OSError, IOError, ImportError): print_usage() raise -#Log.setverbosity(9) +#log.Log.setverbosity(5) +rdiff_backup.Globals.security_level = "override" PipeConnection(sys.stdin, sys.stdout).Server() -- cgit v1.2.1