From: Arvin Schnell <aschnell@suse.de>
Date: Fri, 8 Feb 2013 09:47:57 +0000 (-0500)
Subject: - removed potential security issues
X-Git-Tag: v0.1.3~33
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9ca50ded88738422851227d43f717104bdd4f762;p=thirdparty%2Fsnapper.git

- removed potential security issues
---

diff --git a/snapper/AppUtil.cc b/snapper/AppUtil.cc
index aa00a9f1..4de51547 100644
--- a/snapper/AppUtil.cc
+++ b/snapper/AppUtil.cc
@@ -162,6 +162,24 @@ namespace snapper
     }
 
 
+    string
+    dirname(const string& name)
+    {
+	string::size_type pos = name.find_last_of('/');
+	if (pos == string::npos)
+	    return string(".");
+	return string(name, 0, pos == 0 ? 1 : pos);
+    }
+
+
+    string
+    basename(const string& name)
+    {
+	string::size_type pos = name.find_last_of('/');
+	return string(name, pos + 1);
+    }
+
+
     bool
     getMtabData(const string& mount_point, bool& found, MtabData& mtab_data)
     {
diff --git a/snapper/AppUtil.h b/snapper/AppUtil.h
index 7dd3a257..3304030a 100644
--- a/snapper/AppUtil.h
+++ b/snapper/AppUtil.h
@@ -55,6 +55,9 @@ namespace snapper
 
     string stringerror(int errnum);
 
+    string dirname(const string& name);
+    string basename(const string& name);
+
 
     struct MtabData
     {
diff --git a/snapper/Btrfs.cc b/snapper/Btrfs.cc
index 6935ac41..b04b9b07 100644
--- a/snapper/Btrfs.cc
+++ b/snapper/Btrfs.cc
@@ -533,10 +533,16 @@ namespace snapper
 
 	if (status & (CONTENT | PERMISSIONS | USER | GROUP))
 	{
-	    status &= ~(CONTENT | PERMISSIONS | USER | GROUP);
-	    // TODO use of fullname might be insecure
 	    // TODO check for content sometimes not required
-	    status |= cmpFiles(processor->dir1.fullname(), processor->dir2.fullname(), name);
+	    status &= ~(CONTENT | PERMISSIONS | USER | GROUP);
+
+	    string dirname = snapper::dirname(name);
+	    string basename = snapper::basename(name);
+
+	    SDir subdir1 = SDir::deepopen(processor->dir1, dirname);
+	    SDir subdir2 = SDir::deepopen(processor->dir2, dirname);
+
+	    status |= cmpFiles(SFile(subdir1, basename), SFile(subdir2, basename));
 	}
 
 	return status;
@@ -799,12 +805,16 @@ namespace snapper
 	    processor->deleted(from);
 	    processor->created(to);
 
-	    // TODO use of fullname might be insecure
-	    if (checkDir(processor->dir1.fullname() + "/" + from))
+	    string dirname = snapper::dirname(from);
+	    string basename = snapper::basename(from);
+
+	    struct stat buf;
+	    SDir tmpdir1 = SDir::deepopen(processor->dir1, dirname);
+	    if (tmpdir1.stat(basename, &buf, AT_SYMLINK_NOFOLLOW) == 0 && S_ISDIR(buf.st_mode))
 	    {
-		SDir dir_from(processor->dir1.fullname() + "/" + from);
-		vector<string> entries = dir_from.entries_recursive();
+		SDir tmpdir2(tmpdir1, basename);
 
+		vector<string> entries = tmpdir2.entries_recursive();
 		for (vector<string>::const_iterator it = entries.begin(); it != entries.end(); ++it)
 		{
 		    processor->deleted(from + "/" + *it);
diff --git a/snapper/Compare.cc b/snapper/Compare.cc
index 065f93b7..c4bb61ee 100644
--- a/snapper/Compare.cc
+++ b/snapper/Compare.cc
@@ -207,14 +207,8 @@ namespace snapper
 
 
     unsigned int
-    cmpFiles(const string& base_path1, const string& base_path2, const string& name)
+    cmpFiles(const SFile& file1, const SFile& file2)
     {
-	SDir dir1(base_path1);
-	SDir dir2(base_path2);
-
-	SFile file1(dir1, name); // TODO not secure
-	SFile file2(dir2, name); // TODO not secure
-
 	struct stat stat1;
 	int r1 = file1.stat(&stat1, AT_SYMLINK_NOFOLLOW);
 
diff --git a/snapper/Compare.h b/snapper/Compare.h
index 403040d3..4f390117 100644
--- a/snapper/Compare.h
+++ b/snapper/Compare.h
@@ -39,9 +39,9 @@ namespace snapper
     typedef std::function<void(const string& name, unsigned int status)> cmpdirs_cb_t;
 
 
-    /* TODO */
+    /* Compares the two files. */
     unsigned int
-    cmpFiles(const string& base_path, const string& base_path2, const string& name);
+    cmpFiles(const SFile& file1, const SFile& file2);
 
     /* Compares the two directories. All file-operations use the openat
        et.al. functions. */
diff --git a/snapper/File.cc b/snapper/File.cc
index 6669e4c5..16361175 100644
--- a/snapper/File.cc
+++ b/snapper/File.cc
@@ -162,7 +162,19 @@ namespace snapper
     File::getPreToSystemStatus()
     {
 	if (pre_to_system_status == (unsigned int)(-1))
-	    pre_to_system_status = cmpFiles(file_paths->pre_path, file_paths->system_path, name);
+	{
+	    SDir dir1(file_paths->pre_path);
+	    SDir dir2(file_paths->system_path);
+
+	    string dirname = snapper::dirname(name);
+	    string basename = snapper::basename(name);
+
+	    SDir subdir1 = SDir::deepopen(dir1, dirname);
+	    SDir subdir2 = SDir::deepopen(dir2, dirname);
+
+	    pre_to_system_status = cmpFiles(SFile(subdir1, basename), SFile(subdir2, basename));
+	}
+
 	return pre_to_system_status;
     }
 
@@ -171,7 +183,19 @@ namespace snapper
     File::getPostToSystemStatus()
     {
 	if (post_to_system_status == (unsigned int)(-1))
-	    post_to_system_status = cmpFiles(file_paths->post_path, file_paths->system_path, name);
+	{
+	    SDir dir1(file_paths->post_path);
+	    SDir dir2(file_paths->system_path);
+
+	    string dirname = snapper::dirname(name);
+	    string basename = snapper::basename(name);
+
+	    SDir subdir1 = SDir::deepopen(dir1, dirname);
+	    SDir subdir2 = SDir::deepopen(dir2, dirname);
+
+	    post_to_system_status = cmpFiles(SFile(subdir1, basename), SFile(subdir2, basename));
+	}
+
 	return post_to_system_status;
     }
 
diff --git a/snapper/FileUtils.cc b/snapper/FileUtils.cc
index 88cffca4..e0ea3cd4 100644
--- a/snapper/FileUtils.cc
+++ b/snapper/FileUtils.cc
@@ -29,6 +29,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <stdlib.h>
+#include <assert.h>
 #include <algorithm>
 
 #include "snapper/FileUtils.h"
@@ -65,6 +66,9 @@ namespace snapper
     SDir::SDir(const SDir& dir, const string& name)
 	: base_path(dir.base_path), path(dir.path + "/" + name)
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	dirfd = ::openat(dir.dirfd, name.c_str(), O_RDONLY | O_NOFOLLOW | O_NOATIME | O_CLOEXEC);
 	if (dirfd < 0)
 	{
@@ -77,6 +81,7 @@ namespace snapper
 	if (!S_ISDIR(buf.st_mode))
 	{
 	    y2err("not a directory path:" << dir.fullname(name));
+	    close(dirfd);
 	    throw IOErrorException();
 	}
     }
@@ -118,6 +123,17 @@ namespace snapper
     }
 
 
+    SDir
+    SDir::deepopen(const SDir& dir, const string& name)
+    {
+       string::size_type pos = name.find('/');
+       if (pos == string::npos)
+	   return SDir(dir, name);
+
+       return deepopen(SDir(dir, string(name, 0, pos)), string(name, pos + 1));
+    }
+
+
     string
     SDir::fullname(bool with_base_path) const
     {
@@ -229,6 +245,9 @@ namespace snapper
     int
     SDir::stat(const string& name, struct stat* buf, int flags) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::fstatat(dirfd, name.c_str(), buf, flags);
     }
 
@@ -236,6 +255,9 @@ namespace snapper
     int
     SDir::open(const string& name, int flags) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::openat(dirfd, name.c_str(), flags);
     }
 
@@ -243,6 +265,9 @@ namespace snapper
     int
     SDir::open(const string& name, int flags, mode_t mode) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::openat(dirfd, name.c_str(), flags, mode);
     }
 
@@ -250,6 +275,9 @@ namespace snapper
     int
     SDir::readlink(const string& name, string& buf) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	char tmp[1024];
 	int ret = ::readlinkat(dirfd, name.c_str(), tmp, sizeof(tmp));
 	if (ret >= 0)
@@ -261,6 +289,9 @@ namespace snapper
     int
     SDir::mkdir(const string& name, mode_t mode) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::mkdirat(dirfd, name.c_str(), mode);
     }
 
@@ -268,6 +299,9 @@ namespace snapper
     int
     SDir::unlink(const string& name, int flags) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::unlinkat(dirfd, name.c_str(), flags);
     }
 
@@ -275,6 +309,9 @@ namespace snapper
     int
     SDir::chmod(const string& name, mode_t mode, int flags) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::fchmodat(dirfd, name.c_str(), mode, flags);
     }
 
@@ -282,6 +319,9 @@ namespace snapper
     int
     SDir::chown(const string& name, uid_t owner, gid_t group, int flags) const
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
+
 	return ::fchownat(dirfd, name.c_str(), owner, group, flags);
     }
 
@@ -289,6 +329,12 @@ namespace snapper
     int
     SDir::rename(const string& oldname, const string& newname) const
     {
+	assert(oldname.find('/') == string::npos);
+	assert(oldname != "..");
+
+	assert(newname.find('/') == string::npos);
+	assert(newname != "..");
+
 	return ::renameat(dirfd, oldname.c_str(), dirfd, newname.c_str());
     }
 
@@ -332,6 +378,8 @@ namespace snapper
     SFile::SFile(const SDir& dir, const string& name)
 	: dir(dir), name(name)
     {
+	assert(name.find('/') == string::npos);
+	assert(name != "..");
     }
 
 
diff --git a/snapper/FileUtils.h b/snapper/FileUtils.h
index 30ac834e..b3aba356 100644
--- a/snapper/FileUtils.h
+++ b/snapper/FileUtils.h
@@ -46,6 +46,8 @@ namespace snapper
 	SDir& operator=(const SDir&);
 	~SDir();
 
+	static SDir deepopen(const SDir& dir, const string& name);
+
 	int fd() const { return dirfd; }
 
 	string fullname(bool with_base_path = true) const;