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 188bb87eebbcec1410299e07ab251d88a3af49ff..8b57e87a33e32ebec7ed738a7ad4937b70c1b00c 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
 
    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());
    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);
 
       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
           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)
 
    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 8da832ba9f3359f689f8c42f35446b4f0c577721..1d1a1a1b8ddcc39fd81b270ec549b0f3ffccf64e 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 <string>
 #include <vector>
 #include <stdlib.h>

+#include <sys/stat.h>

 
 #include "assert.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;
 
 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)
    // normal match
    files = Glob("*.lst");
    if (files.size() != 1)