- removed potential security issues

diff --git a/snapper/AppUtil.cc b/snapper/AppUtil.cc
index aa00a9f1ca3a11af80f734a16a92afcf27141640..4de51547131272a6edb7bcedb77554ccfe271ff8 100644 (file)
--- 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 7dd3a257fcd3e8a638e206817128b253c01568de..3304030a34c817763cbe26f662384e104d9efca2 100644 (file)
--- 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 6935ac413a930f4d3a97c1241ab519e9186716a3..b04b9b07e8413fbd4a4cc108f9e92260e18963f0 100644 (file)
--- 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 065f93b718ef0e999b1b0a34ed4024e51ba1e922..c4bb61ee6ecce912e0b05d89f0a44f36e318ed25 100644 (file)
--- 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 403040d3c6790c79f111d49f852e14f964701b21..4f3901175689132d2efbca1dc3fd334700596939 100644 (file)
--- 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 6669e4c515b9fbaa6bd039024e1cc7c07fa58e20..163611751a4c06931a17924df8c4b3b98829a71f 100644 (file)
--- 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 88cffca4dda2d4324f311bf5201e9a76a547c048..e0ea3cd43e6d21b89a65e33cd42b58027ef3cfe8 100644 (file)
--- 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 30ac834e9ee912cdb5a78afa4d6d936dec40f582..b3aba356d80e23f76a8544a40062bb3aa7777fed 100644 (file)
--- 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;