Fix for bug 14209 (security violation with --restrict)

git-svn-id: http://svn.savannah.nongnu.org/svn/rdiff-backup@625 2b77aa54-bcbc-44c9-a7ec-4f6cf2b41109

trunk/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)
 --------------------------

trunk/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,48 +126,35 @@ 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",
+	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":
-			allowed_requests.extend(
-				["fs_abilities.get_fsabilities_readonly",
+	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"])
-		elif sec_level == "update-only":
-			allowed_requests.extend(
-				["log.Log.open_logfile_local", "log.Log.close_logfile_local",
+	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",
@@ -162,18 +166,31 @@ def set_allowed_requests(sec_level):
 				  "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",
+		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))
+

trunk/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)

trunk/rdiff-backup/testing/securitytest.py

-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()
 		

trunk/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()