Fix insecure file permissions when using FileFd with OpenMode::Atomic

Commit 7335eebea6dd43581d4650a8818b06383ab89901 introduced a bug
that caused FileFd to create insecure permissions when FileFd::Atomic
is used. This commit fixes the permissions and adds a test.

The bug is most likely caused by the confusing "Perm" parameter
that is passed to Open() - its not the file permissions but intead
the "mode" part of open/creat.

diff --git a/apt-pkg/contrib/fileutl.cc b/apt-pkg/contrib/fileutl.cc
index 188bb87..8b57e87 100644 (file)
--- a/apt-pkg/contrib/fileutl.cc
+++ b/apt-pkg/contrib/fileutl.cc
@@ -1067,6 +1067,10 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
    if_FLAGGED_SET(Exclusive, O_EXCL);
    #undef if_FLAGGED_SET
 
+   // there is no getumask() so we read it by setting it and reset
+   mode_t current_umask = umask(0);
+   umask(current_umask);
+
    if ((Mode & Atomic) == Atomic)
    {
       char *name = strdup((FileName + ".XXXXXX").c_str());
@@ -1080,11 +1084,11 @@ bool FileFd::Open(string FileName,unsigned int const Mode,APT::Configuration::Co
       TemporaryFileName = string(name);
       free(name);
 
-      if(Perms != 600 && fchmod(iFd, Perms) == -1)
+      if(Perms != 600 && fchmod(iFd, Perms & ~current_umask) == -1)
           return FileFdErrno("fchmod", "Could not change permissions for temporary file %s", TemporaryFileName.c_str());
    }
    else
-      iFd = open(FileName.c_str(), fileflags, Perms);
+      iFd = open(FileName.c_str(), fileflags, Perms & ~current_umask);
 
    this->FileName = FileName;
    if (iFd == -1 || OpenInternDescriptor(Mode, compressor) == false)

diff --git a/test/libapt/fileutl_test.cc b/test/libapt/fileutl_test.cc
index 8da832b..1d1a1a1 100644 (file)
--- a/test/libapt/fileutl_test.cc
+++ b/test/libapt/fileutl_test.cc
@@ -6,13 +6,44 @@
 #include <string>
 #include <vector>
 #include <stdlib.h>
+#include <sys/stat.h>
 
 #include "assert.h"
 
+// regression test for permission bug LP: #1304657
+static bool
+TestFileFdOpenPermissions(mode_t a_umask, mode_t ExpectedFilePermission)
+{
+   FileFd f;
+   struct stat buf;
+   static const char* fname = "test.txt";
+
+   umask(a_umask);
+   f.Open(fname, FileFd::ReadWrite|FileFd::Atomic);
+   f.Close();
+   if (stat(fname, &buf) < 0)
+   {
+      _error->Errno("stat", "failed to stat");
+      _error->DumpErrors();
+      return false;
+   }
+   unlink(fname);
+   equals(buf.st_mode & 0777, ExpectedFilePermission);
+   return true;
+}
+
 int main()
 {
    std::vector<std::string> files;
 
+   if (TestFileFdOpenPermissions(0002, 0664) == false ||
+       TestFileFdOpenPermissions(0022, 0644) == false ||
+       TestFileFdOpenPermissions(0077, 0600) == false ||
+       TestFileFdOpenPermissions(0026, 0640) == false)
+   {
+      return 1;
+   }
+
    // normal match
    files = Glob("*.lst");
    if (files.size() != 1)